Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,18 +284,23 @@ impl StepDescription {
}
}

fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) {
fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) -> Option<()> {
if pathsets.iter().any(|set| self.is_excluded(builder, set)) {
return;
return Some(());
}

// Determine the targets participating in this rule.
let targets = if self.only_hosts { &builder.hosts } else { &builder.targets };
if targets.is_empty() && self.only_hosts {
return None;
}

for target in targets {
let run = RunConfig { builder, paths: pathsets.clone(), target: *target };
(self.make_run)(run);
}

Some(())
}

fn is_excluded(&self, builder: &Builder<'_>, pathset: &PathSet) -> bool {
Expand Down Expand Up @@ -328,10 +333,12 @@ impl StepDescription {
);
}

let mut hosts_check = Vec::new();
if paths.is_empty() || builder.config.include_default_paths {
for (desc, should_run) in v.iter().zip(&should_runs) {
if desc.default && should_run.is_really_default() {
desc.maybe_run(builder, should_run.paths.iter().cloned().collect());
hosts_check
.push(desc.maybe_run(builder, should_run.paths.iter().cloned().collect()));
}
}
}
Expand All @@ -345,7 +352,7 @@ impl StepDescription {
paths.retain(|path| {
for (desc, should_run) in v.iter().zip(&should_runs) {
if let Some(suite) = should_run.is_suite_path(&path) {
desc.maybe_run(builder, vec![suite.clone()]);
hosts_check.push(desc.maybe_run(builder, vec![suite.clone()]));
return false;
}
}
Expand All @@ -360,10 +367,19 @@ impl StepDescription {
for (desc, should_run) in v.iter().zip(&should_runs) {
let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind);
if !pathsets.is_empty() {
desc.maybe_run(builder, pathsets);
hosts_check.push(desc.maybe_run(builder, pathsets));
}
}

// 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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, there are three cases:

  1. ./x.py is invoked with no host parameter. This will make the host value default to this machine.
  2. ./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.
  3. ./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? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

builder.kind.as_str()
);
crate::detail_exit(1);
}

if !paths.is_empty() {
eprintln!("error: no `{}` rules matched {:?}", builder.kind.as_str(), paths,);
eprintln!(
Expand Down
32 changes: 32 additions & 0 deletions src/bootstrap/builder/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,16 @@ fn test_exclude() {
assert!(cache.contains::<test::RustdocUi>());
}

#[test]
fn test_with_empty_host_and_empty_target() {
let config = configure("test", &[], &[]);
let cache = run_build(&[], config);

assert!(!cache.contains::<test::Tidy>());
assert!(!cache.contains::<test::Cargotest>());
assert!(!cache.contains::<test::RustdocUi>());
}

#[test]
fn test_exclude_kind() {
let path = PathBuf::from("src/tools/cargotest");
Expand Down Expand Up @@ -430,6 +440,16 @@ mod dist {
);
}

#[test]
fn dist_with_empty_host_and_empty_target() {
let config = configure(&[], &[]);
let mut cache = run_build(&[], config);

assert!(cache.all::<dist::Docs>().is_empty());
assert!(cache.all::<dist::Mingw>().is_empty());
assert!(cache.all::<dist::Std>().is_empty());
}

#[test]
fn dist_with_same_targets_and_hosts() {
let mut cache = run_build(&[], configure(&["A", "B"], &["A", "B"]));
Expand Down Expand Up @@ -541,6 +561,18 @@ mod dist {
);
}

#[test]
fn build_with_empty_host_and_empty_target() {
let config = configure(&[], &[]);
let build = Build::new(config);
let mut builder = Builder::new(&build);
builder.run_step_descriptions(&Builder::get_step_descriptions(Kind::Build), &[]);

assert!(builder.cache.all::<compile::Std>().is_empty());
assert!(builder.cache.all::<compile::Assemble>().is_empty());
assert!(builder.cache.all::<compile::Rustc>().is_empty());
}

#[test]
fn test_with_no_doc_stage0() {
let mut config = configure(&["A"], &["A"]);
Expand Down