Added sanity check to to catch empty `host` parameter in `./x.py test --host=''` #107858

madsravn · 2023-02-09T20:24:59Z

I have added this test to prematurely exit the bootstrapper before it running all its logic. This solves #77906

rustbot · 2023-02-09T20:25:10Z

(rustbot has picked a reviewer for you, use r? to override)

compiler-errors · 2023-02-09T20:30:29Z

Just a side-note that you don't need to put the issue number in the title, just a "fixes ####" in the description will make sure it's linked correctly.

compiler-errors · 2023-02-09T20:31:20Z

(Though also, PR titles should be more about what they do, so this deserves a better title I think?)

Maybe "error if x test is run with empty --host string" is a good title?

madsravn · 2023-02-09T20:37:20Z

(Though also, PR titles should be more about what they do, so this deserves a better title I think?)

Maybe "error if x test is run with empty --host string" is a good title?

I think I changed the title while you were editting this comment. Is it OK or should I change to your suggestion? I'm not heavily invested in mine.

compiler-errors · 2023-02-09T20:38:47Z

Your title is totally fine.

Mark-Simulacrum · 2023-02-10T00:13:41Z

Looks like tests are failing. We also pass this flag exactly like this, though it may not be strictly necessary,

rust/src/ci/docker/host-x86_64/test-various/Dockerfile

Line 57 in 4887cb1

    
           ENV WASM_SCRIPT python3 /checkout/x.py --stage 2 test --host='' --target $WASM_TARGETS \

- and I think there are valid use cases for a target-only test run.

madsravn · 2023-02-10T11:49:29Z

@Mark-Simulacrum

I fixed my failling tests and created some new ones. There are valid use cases for target only (thanks @compiler-errors for pointing to the issue this was introduced) and I have thought that into my fix.

I might be a little unsure whether or not my solution is elegant enough. A mutable vector catching all the return values seems a little non-idiomatic. What do you think?

src/bootstrap/builder.rs

Mark-Simulacrum · 2023-02-12T20:03:35Z

src/bootstrap/builder.rs

+        // sanity checks on hosts
+        if !hosts_check.contains(&Some(())) {
+            eprintln!(
+                "`x.py {}` run with empty `host` parameter. Either set it or leave it out for default value.",


This error message seems to believe that this can only happen if we have hosts as an empty parameter, but I'm not convinced by skimming the code above that's the only way to hit this. Can we document the rationale for that conclusion in a comment, plus why we don't validate host='' at flag parsing time?

As far as I understand, there are three cases:

./x.py is invoked with no host parameter. This will make the host value default to this machine.

./x.py is invoked with an explicitely set empty host parameter, but also an explicitely set non-empty target parameter. This will set the host value to the target value.

./x.py is invoked with an explicitely set empty host parameter and no target parameter.

Case 3 is what we are fixing here. I think I achieved this by the check on line 294. This also takes case 1 and 2 into consideration and does not treat those as bad cases.

plus why we don't validate host='' at flag parsing time?

When is that - or where is that? :)

Flag parsing is in flags.rs and config.rs, I believe. (Maybe we can put this check there, but I suspect that it's not that simple - it is sometimes valid to pass --host with an empty string. Not 100% confident about that though.

Would you mind us keeping this check here ? This fixes the issue at hand and is tied together with the other logic of testing for empty host and target parameters.

I am still not following the logic here, or exactly why you think this code specifically targets the case you mention. But the code here (https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L1005) seems like it's able to directly detect that we're setting host + target to empty sets and error based on that.

If you would rather have it there, I can try to look into that. It was just mentioned in the issue ( #77906 ) that this would be the spot to fix it. And now it is fixed there.

I'm not sure why @jyn514 suggested fixing #77906 here. It seems to me that if a fix is necessary at all, we would want to fix this in a more general manner -- for example, if nothing is run, we error rather than saying "all is successful", without trying to determine why nothing was run (e.g., exclude rules excluding all targets, etc.)

I'm not sure why @jyn514 suggested fixing #77906 here. It seems to me that if a fix is necessary at all, we would want to fix this in a more general manner -- for example, if nothing is run, we error rather than saying "all is successful", without trying to determine why nothing was run (e.g., exclude rules excluding all targets, etc.)

I don't know why this place in the code was picked, but to me it seems like a fine place. We can check what we need to check, emit errors with a return code 1 and it fits some of the other checks that is being done here (and could be done). But how do we determine this? This is my second pull request for this project, so I don't imagine my opinion matters too much.

for example, if nothing is run, we error rather than saying "all is successful", without trying to determine why nothing was run (e.g., exclude rules excluding all targets, etc.)

I am not sure I understand this. If nothing is run due to empty host or exclude rules or something third, I think it is important to tell the user that the build was unsuccessful and tell them why. The why is important because that gives them a path forward - a way to fix the issue. Do we disagree on this?

I am not sure I understand this. If nothing is run due to empty host or exclude rules or something third, I think it is important to tell the user that the build was unsuccessful and tell them why. The why is important because that gives them a path forward - a way to fix the issue. Do we disagree on this?

I don't think we agree that providing arguments that lead to a no-op build necessarily should error, but it doesn't seem bad to error in that case. Regardless, this PR is giving a very specific error message about providing empty hosts but isn't touching the code in a way that I can quickly say "that is the only way to reach this error message". Telling the user that they have empty hosts when that may not be the case is very confusing, and I cannot easily follow the conditions to in fact confirm that is the case for this error.

Mark-Simulacrum · 2023-02-12T20:05:05Z

A mutable vector catching all the return values seems a little non-idiomatic. What do you think?

Yes, as noted on the review I think probably we want a bool as the return type and maybe an Option<bool> for the tracking if we can't get away with just a bool there.

madsravn · 2023-02-22T19:30:34Z

Ping @Mark-Simulacrum - What do you think of it now? :)

Mark-Simulacrum · 2023-02-25T14:52:56Z

src/bootstrap/builder.rs

-    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) {
+    /// This might run the builder. Returns false if no targets can be determined indicating that
+    /// builder was not run
+    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) -> bool {


Given that we return true if we did determine targets but they were excluded, I feel like the comment should also note that case - it otherwise also falls under "didn't run".

I am having a hard time giving a nice description to this function. Instead of committing my current iteration, I will paste it here and then we can look at it?

/// This might run the builder. If the any of the `pathsets` is excluded by `self.is_excluded` /// it will not run but will return true. /// If no target can be determined it will not run and will return false. /// Else it will run and return true. fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) -> bool {

At least we should adjust -- this might run self, not the Builder. Builder is essentially configuration, not something you run.

It seems a little odd that we return true if we are excluded -- that implies that e.g. x.py test --host='' --exclude src/test/ui would not cause the error this PR is trying to add, right? That doesn't seem logical to me.

It seems a little odd that we return true if we are excluded

I agree, but that is current behaviour. I would like to fix that in a future issue. Normally when I grab an issue I keep my fix contained to that issue. Would that plan of attack work for you?

But going forward I think we need to align on what is the most critical error - or should we emit both? If we both exclude the only path we are testing and give an empty host.

I agree, but that is current behaviour. I would like to fix that in a future issue. Normally when I grab an issue I keep my fix contained to that issue. Would that plan of attack work for you?

In general with bootstrap a targeted fix may not be the best one, because we don't want to add to confusion by adding checks that make little sense by themselves. This code isn't necessarily great and cleaning it up as we go is the main way we can make progress on changing that.

But going forward I think we need to align on what is the most critical error - or should we emit both? If we both exclude the only path we are testing and give an empty host.

I think my confusion and maybe the miscommunication here is that "empty host" isn't the thing we're trying to give an error on -- at least not directly -- if that was the targeted fix being pursued, then a check in config.rs would be the right place. This PR is adding code interleaved with the decision of how to execute a specific part of the build (Step impl), and executes many times for every build.

src/bootstrap/builder.rs

Mark-Simulacrum · 2023-02-25T14:59:49Z

src/bootstrap/builder.rs

+        // sanity checks on hosts
+        if !hosts_check.contains(&Some(())) {
+            eprintln!(
+                "`x.py {}` run with empty `host` parameter. Either set it or leave it out for default value.",


I am still not following the logic here, or exactly why you think this code specifically targets the case you mention. But the code here (https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L1005) seems like it's able to directly detect that we're setting host + target to empty sets and error based on that.

madsravn · 2023-03-02T20:42:45Z

Ping @Mark-Simulacrum

Any way you could accept the check in the place I put it?

The issue I solve here is a wrong "build succesfull" message when nothing was build. The same issue exists when you doesn't run anything due to exclude paths. I wish to solve that issue afterwards and that check also resides here.

Mark-Simulacrum · 2023-03-02T23:35:55Z

I typically only have significant review bandwidth once a week, so I will respond over the weekend.

Mark-Simulacrum

Sorry for the long delays etc here. I remain unconvinced that this is the right approach or even that fixing #77906 is all that necessary, but hopefully these comments can move us in the right direction toward getting to agreement on the right approach forward.

src/bootstrap/builder.rs

Mark-Simulacrum · 2023-03-04T16:44:11Z

src/bootstrap/builder.rs

-    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) {
+    /// This might run the builder. Returns false if no targets can be determined indicating that
+    /// builder was not run
+    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) -> bool {


At least we should adjust -- this might run self, not the Builder. Builder is essentially configuration, not something you run.

It seems a little odd that we return true if we are excluded -- that implies that e.g. x.py test --host='' --exclude src/test/ui would not cause the error this PR is trying to add, right? That doesn't seem logical to me.

src/bootstrap/builder.rs

Mark-Simulacrum · 2023-03-04T16:50:29Z

src/bootstrap/builder.rs

+        // sanity checks on hosts
+        if !hosts_check.contains(&Some(())) {
+            eprintln!(
+                "`x.py {}` run with empty `host` parameter. Either set it or leave it out for default value.",


I'm not sure why @jyn514 suggested fixing #77906 here. It seems to me that if a fix is necessary at all, we would want to fix this in a more general manner -- for example, if nothing is run, we error rather than saying "all is successful", without trying to determine why nothing was run (e.g., exclude rules excluding all targets, etc.)

Solving the issue

755ea9f

rustbot assigned Mark-Simulacrum Feb 9, 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 Feb 9, 2023

compiler-errors changed the title ~~Solving issue 77906: ./x.py test --host='' does nothing and succeeds~~ ./x.py test --host='' does nothing and succeeds Feb 9, 2023

compiler-errors changed the title ~~./x.py test --host='' does nothing and succeeds~~ Solving ./x.py test --host='' does nothing and succeeds Feb 9, 2023

madsravn changed the title ~~Solving ./x.py test --host='' does nothing and succeeds~~ Added sanity check to to catch empty host parameter in ./x.py test --host='' Feb 9, 2023

Fixing formatting

62f67bf

This comment has been minimized.

Sign in to view

Fixing placement of test and adding tests

0118a8b

Mark-Simulacrum reviewed Feb 12, 2023

View reviewed changes

Mads Ravn and others added 2 commits February 16, 2023 20:10

Simplifying to booleans instead of Options

a874d7d

Adding comment to method

89b37c9

Mark-Simulacrum reviewed Feb 25, 2023

View reviewed changes

Fixing comment for function

707e11f

Mark-Simulacrum reviewed Mar 4, 2023

View reviewed changes

Fixing assignments to BitOrAssign and small change to comment

48f755e

This comment has been minimized.

Sign in to view

Formatting issue

8491691

Mark-Simulacrum 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 Mar 18, 2023

madsravn closed this Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added sanity check to to catch empty `host` parameter in `./x.py test --host=''` #107858

Added sanity check to to catch empty `host` parameter in `./x.py test --host=''` #107858

madsravn commented Feb 9, 2023 •

edited by compiler-errors

Loading

rustbot commented Feb 9, 2023

compiler-errors commented Feb 9, 2023

compiler-errors commented Feb 9, 2023 •

edited

Loading

madsravn commented Feb 9, 2023

compiler-errors commented Feb 9, 2023

Mark-Simulacrum commented Feb 10, 2023 •

edited by jyn514

Loading

This comment has been minimized.

madsravn commented Feb 10, 2023

Mark-Simulacrum Feb 12, 2023

madsravn Feb 12, 2023

Mark-Simulacrum Feb 12, 2023

madsravn Feb 12, 2023

Mark-Simulacrum Feb 25, 2023

madsravn Feb 25, 2023

Mark-Simulacrum Mar 4, 2023

madsravn Mar 4, 2023

Mark-Simulacrum Mar 4, 2023

Mark-Simulacrum commented Feb 12, 2023 •

edited

Loading

madsravn commented Feb 22, 2023

Mark-Simulacrum Feb 25, 2023

madsravn Feb 25, 2023

Mark-Simulacrum Mar 4, 2023

madsravn Mar 4, 2023

Mark-Simulacrum Mar 4, 2023

Mark-Simulacrum Feb 25, 2023

madsravn commented Mar 2, 2023

Mark-Simulacrum commented Mar 2, 2023

Mark-Simulacrum left a comment

Mark-Simulacrum Mar 4, 2023

Mark-Simulacrum Mar 4, 2023

This comment has been minimized.

Added sanity check to to catch empty host parameter in ./x.py test --host='' #107858

Added sanity check to to catch empty host parameter in ./x.py test --host='' #107858

Conversation

madsravn commented Feb 9, 2023 • edited by compiler-errors Loading

rustbot commented Feb 9, 2023

compiler-errors commented Feb 9, 2023

compiler-errors commented Feb 9, 2023 • edited Loading

madsravn commented Feb 9, 2023

compiler-errors commented Feb 9, 2023

Mark-Simulacrum commented Feb 10, 2023 • edited by jyn514 Loading

This comment has been minimized.

madsravn commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Feb 12, 2023 • edited Loading

madsravn commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsravn commented Mar 2, 2023

Mark-Simulacrum commented Mar 2, 2023

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Added sanity check to to catch empty `host` parameter in `./x.py test --host=''` #107858

Added sanity check to to catch empty `host` parameter in `./x.py test --host=''` #107858

madsravn commented Feb 9, 2023 •

edited by compiler-errors

Loading

compiler-errors commented Feb 9, 2023 •

edited

Loading

Mark-Simulacrum commented Feb 10, 2023 •

edited by jyn514

Loading

Mark-Simulacrum commented Feb 12, 2023 •

edited

Loading