Skip to content
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

bootstrap: make --exclude consistent with --skip in libtest #111894

Closed
jyn514 opened this issue May 24, 2023 · 5 comments · Fixed by #114001
Closed

bootstrap: make --exclude consistent with --skip in libtest #111894

jyn514 opened this issue May 24, 2023 · 5 comments · Fixed by #114001
Assignees
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented May 24, 2023

Right now, there are two ways to ignore tests:

  1. Pass x test --exclude tests/ui/weird-exprs.rs to bootstrap
  2. Pass x test --test-args --skip=tests/ui/weird-exprs.rs to compiletest

It's weird that these use different arguments. --skip is public-facing, so we should rename --exclude to --skip in bootstrap.

I don't have a strong opinion on whether to keep --exclude around as an alias for --skip, but --skip should be the one we document.

Note that --exclude is slightly different from --skip because it can also applies to whole Steps, not just tests, and doesn't support substring matching. But I don't think it's different enough to be worth having a different name.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels May 24, 2023
@Mark-Simulacrum
Copy link
Member

It's worth noting that they also have pretty different semantics. Skipping a test is reliable (we wouldn't ever run it) where exclude in bootstrap only applies to the defaults for that command, something ensure'ing that step will still run it. (keep-stage has closer semantics to skip, in some sense).

I'm not sure if it makes sense to rename here, the different names don't feel like they cause confusion to me?

@jyn514
Copy link
Member Author

jyn514 commented May 24, 2023

Hmm that's fair - I'm more worried about discoverability than confusion, but I do see how the new name could be confusing.

For context, I thought of this after someone looked for --skip in bootstrap's help output and didn't find it: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/UI.20test.20fails.20with.20clang.20as.20a.20linker

@meysam81
Copy link
Contributor

Hey guys,
I'm a first-time contributor, enthusiast, and admirer of the Rust lang.
Is there anything I can do here?
Regards.

@albertlarsan68
Copy link
Member

Hello @meysam81,

The bare minimum would be to change this line: https://github.com/rust-lang/rust/blob/master/src/bootstrap/flags.rs#L71
You can either just rename this flag, or duplicate it to keep the current command lines compatible.
Then, as the end goal is to change the option, rename the struct member there:
https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L133
Following the compiler errors, you should end at this point:
https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L1116
There, if you decided to rename the command-line flag, you should rename both sides of this line, else you should change the left side of this assignment, and duplicate the line to also change the right side to the new flag name.
What is left to do is to fix the remaining compiler errors, then you have a PR ready to be reviewed!

@albertlarsan68 albertlarsan68 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jul 23, 2023
@meysam81
Copy link
Contributor

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants