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

Simplify --exclude syntax and make it more useful #103201

Closed
jyn514 opened this issue Oct 18, 2022 · 3 comments · Fixed by #112297
Closed

Simplify --exclude syntax and make it more useful #103201

jyn514 opened this issue Oct 18, 2022 · 3 comments · Fixed by #112297
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. 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 Oct 18, 2022

I think we should have a holistic conversation about what excludes should look like. The current syntax is currently quite baroque and AFAIK @pietroalbini's CI scripts are the only ones using most of the features. Slapping git-style ignores on top seems like it will make things more confusing, not less.

That said, I agree the behavior is confusing and there's room for improvement; I'd love to get rid of the doc:: prefixes somehow and make the various paths unambiguous.

See https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/excludes for the discussion; this issue is just a placeholder so it doesn't get lost.

Originally posted by @jyn514 in #102706 (review)

@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 18, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 5, 2023

That said, I agree the behavior is confusing and there's room for improvement; I'd love to get rid of the doc:: prefixes somehow and make the various paths unambiguous.

I wonder if we could do this by making it so that --exclude only applies to the subcommand that was run; so for example x test --exclude library/std would behave the same as x test --exclude test::library/std today. I don't think there's a use case for x test --exclude doc::library/std, that will cause tests to fail anyway.

@jyn514 jyn514 added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Mar 5, 2023
@pietroalbini
Copy link
Member

I don't think there's a use case for x test --exclude doc::library/std, that will cause tests to fail anyway.

So, I just checked and in Ferrocene we don't use --exclude test::library/std (or any kind:: prefix) anymore after a CI refactor (linkchecker runs in a separate job than tests now), so I don't think I'm a blocker for this anymore.

The behavior you mentioned would still fix #91794, and I don't see any extra reasoning in #91965 that would require a doc:: prefix when running tests.

@jyn514
Copy link
Member Author

jyn514 commented Mar 11, 2023

Mentoring instructions:

  1. Change is_excluded to always return false if step_description.kind != builder.config.kind:
    fn is_excluded(&self, builder: &Builder<'_>, pathset: &PathSet) -> bool {
  2. Remove TaskPath and replace all uses with Path directly:
    #[derive(Clone, PartialOrd, Ord, PartialEq, Eq)]
    pub struct TaskPath {
    pub path: PathBuf,
    pub kind: Option<Kind>,
  3. Verify that x test --stage 1 --exclude library/core linkchecker fails before and passes afterwards, like in Linkchecker tool reports a huge number of broken links #91794 (comment).

@jyn514 jyn514 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 11, 2023
@bors bors closed this as completed in 04e41dd Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. 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.

2 participants