Removed bootstrap asserts by GentBinaku · Pull Request #107483 · rust-lang/rust

GentBinaku · 2023-01-30T20:00:18Z

No description provided.

…inaku/rust-1 into removed_bootstrap_asserts

rustbot · 2023-01-30T20:00:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ozkanonur (or someone else) soon.

Please see the contribution instructions for more information.

GentBinaku · 2023-01-30T20:01:03Z

r? @jyn514

Noratrieb

Maybe it would make sense to create a exit_error! macro that wraps the eprintln and detail_exit?

Noratrieb · 2023-01-30T20:46:40Z

src/bootstrap/job.rs

+    if job.is_null() {
+        eprintln!("{}", io::Error::last_os_error());
+    }
    assert!(!job.is_null(), "{}", io::Error::last_os_error());


This seems redundant

onur-ozkan

I agree suggestion of @Nilstrieb, abstracting those eprintln, crate::detail_exit duplications can be better for future changes.

onur-ozkan · 2023-01-30T21:03:52Z

src/bootstrap/builder.rs

+        match self.kind == Kind::Setup || !self.builder.src.join(alias).exists() {
+            true => {}
+            false => {
+                eprintln!("use `builder.path()` for real paths: {:?}", alias)
+            }
+        };


if statement would better fit here

this is not a user-facing error message, this only happens if there's a bug in a Step. so I think an assert like before is better.

onur-ozkan · 2023-01-30T21:04:11Z

src/bootstrap/config.rs

+                    match s == "if-available" {
+                        false => {
+                            eprintln!("unknown option `{}` for download-ci-llvm", s);
+                            crate::detail_exit(1);
+                        }
+                        true => {}
+                    };


Same here as well

jyn514 · 2023-01-30T23:12:07Z

r? @ozkanonur

…emoved_bootstrap_asserts

jyn514

I've only glanced over this, but at a first glance it doesn't look right, it's changed every assert to an error message. I want to instead distinguish user errors from internal errors. I'm happy to give guidance on the difference but it may be a while before I can go line by line through the PR.

As a general rule:

If the panic can only happen because bootstrap's code itself was changed, it's an internal error (e.g. the Step checking in builder.rs)
If the panic can only happen because of a config.toml option or flags passed to bootstrap, before any commands are run, it's a user error
If the panic happens because of an external command it's ambiguous, but I would prefer to default to panicking so people report the bug.
If the panic happens because of an I/O error it's likely an internal error.
If the panic should never happen (e.g. "path traversal attack") it's an internal error

rust-log-analyzer · 2023-02-05T10:56:59Z

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:aa723573e04016ede7da6c5d7b029e72cb8a05a3)
Complete job name: PR (x86_64-gnu-llvm-13, false, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: x86_64-gnu-llvm-13
---
test builder::tests::test_intersection ... ok
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/deps/bootstrap-ee2956ab5ba3b811 --test-threads=1` (exit status: 1)

jyn514 · 2023-02-05T17:29:30Z

Maybe a simpler rule is "would it be helpful to show a backtrace of where the error is emitted?" If so it should be a panic, if not it should use eprintln.

onur-ozkan · 2023-02-05T17:46:04Z

Maybe a simpler rule is "would it be helpful to show a backtrace of where the error is emitted?" If so it should be a panic, if not it should use eprintln.

IMO having backtrace on errors always nicer than not having it. There should be a huge cost(which I can't see any in this PR's case) for avoiding such need.

jyn514 · 2023-02-05T18:03:19Z

I don't agree, "option X can't be combined with download-ci-llvm" is clear on its own and doesn't need a backtrace. And using panics for expected errors makes it more likely that people will ignore real bugs that we do want them to report.

onur-ozkan · 2023-02-05T19:07:01Z

using panics for expected errors makes it more likely that people will ignore real bugs that we do want them to report.

I didn't look from this perspective. Seems reasonable and makes sense.

bors · 2023-02-15T00:44:53Z

☔ The latest upstream changes (presumably #108056) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2023-03-14T06:02:22Z

@GentBinaku
ping from triage - can you post your status on this PR?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

jyn514 · 2023-04-05T01:02:33Z

I'm going to close this since it's been a while - feel free to reopen if you have time to work on it again.

GentBinaku added 2 commits January 30, 2023 20:53

Removed asserts and panic

aa73ef7

Merge branch 'removed_bootstrap_asserts' of personal.github.com:GentB…

c03db81

…inaku/rust-1 into removed_bootstrap_asserts

rustbot assigned onur-ozkan Jan 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 30, 2023

rustbot assigned jyn514 and unassigned onur-ozkan Jan 30, 2023

Noratrieb reviewed Jan 30, 2023

View reviewed changes

onur-ozkan suggested changes Jan 30, 2023

View reviewed changes

onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2023

rustbot assigned onur-ozkan and unassigned jyn514 Jan 30, 2023

This comment has been minimized.

Sign in to view

GentBinaku added 2 commits February 4, 2023 10:55

Merge branch 'master' of personal.github.com:GentBinaku/rust-1 into r…

9244474

…emoved_bootstrap_asserts

Merge masters

a7f110e

jyn514 suggested changes Feb 4, 2023

View reviewed changes

jyn514 closed this Apr 5, 2023

jyn514 added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Apr 5, 2023

jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 5, 2023

Uh oh!

Conversation

GentBinaku commented Jan 30, 2023

Uh oh!

rustbot commented Jan 30, 2023

Uh oh!

GentBinaku commented Jan 30, 2023

Uh oh!

Noratrieb left a comment

Choose a reason for hiding this comment

Uh oh!

Noratrieb Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

jyn514 Feb 4, 2023

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

jyn514 commented Jan 30, 2023

Uh oh!

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

rust-log-analyzer commented Feb 5, 2023

Uh oh!

jyn514 commented Feb 5, 2023

Uh oh!

onur-ozkan commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Feb 5, 2023

Uh oh!

onur-ozkan commented Feb 5, 2023

Uh oh!

bors commented Feb 15, 2023

Uh oh!

JohnCSimon commented Mar 14, 2023

Uh oh!

jyn514 commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

onur-ozkan commented Feb 5, 2023 •

edited

Loading