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

Add more granular --exclude in x.py #91965

Merged
merged 2 commits into from
Jan 22, 2022

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Dec 15, 2021

x.py has support for excluding some steps from the current invocation, but unfortunately that's not granular enough: some steps have the same name in different modules, and that prevents excluding only some of them.

As a practical example, let's say you need to run everything in ./x.py test except for the standard library tests, as those tests require IPv6 and need to be executed on a separate machine. Before this commit, if you were to just run this:

./x.py test --exclude library/std

...the invocation would eventually fail, as that would not only exclude running the tests for the standard library (library/std in the test module), it would also exclude generating its documentation (library/std in the doc module), breaking linkchecker.

This commit adds support to the --exclude flag for prefixing paths with the name of the module their step is defined in, allowing the user to choose which module to exclude from:

./x.py test --exclude test::library/std

This maintains backward compatibility with existing invocations, while allowing more ganular exclusion. Examples of the behavior:

--exclude Docs Tests
library/std Skipped Skipped
doc::library/std Skipped Run
test::library/std Run Skipped

Note that this PR only changes the --exclude flag, and not in other x.py arguments or flags yet.

In the implementation I tried to limit the impact this would have with rustbuild as a whole as much as possible. The module name is extracted from the step by parsing the result of std::any::type_name(): unfortunately that output can change at any point in time, but IMO it's better than having to annotate all the existing and future Step implementations with the module name. I added a test to ensure the parsing works as expected, so hopefully if anyone makes changes to the output of std::any::type_name() they'll also notice they have to update rustbuild.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2021
@the8472
Copy link
Member

the8472 commented Dec 15, 2021

Isn't the problem one of dependency management rather than path includes/excludes?

Either linkchecker has to force library/std docs to be generated even when they're excluded or excluding them must also cause linkchecker to skip checking the std docs because a dependency is not met.

@camelid camelid added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Dec 15, 2021
@pietroalbini
Copy link
Member Author

To clarify, in theory linkchecker could pass even if the standard library docs were skipped: the problem is that other documentation links to the standard library docs, so if the standard library docs are not there linkchecker will identify those links as broken.

Changing the linkchecker step to always build all docs would indeed solve the specific problem I'm facing when excluding the standard library tests, but would also prevent other use cases: at least I have occasionally done ./x.py test src/tools/linkchecker --exclude $foo to get a list of all links pointing to the $foo documentation, and such change would prevent that. I can imagine there are also other use cases I'm not thinking about.

@petrochenkov
Copy link
Contributor

cc #91794

@Mark-Simulacrum
Copy link
Member

I'm not actually sure this is the right implementation strategy. The module is not really a user-visible thing; and eventually some Steps may work for multiple different modules (Kind, in bootstrap terminology).

You should be able to adjust the match arms here to pass in the kind to StepDescription::from in some way, I think? Not sure if that'll work too easily, may need some refactoring. Ideally the strategy pursued should take into account the possibility for one struct implementing Step to have multiple different "kinds", though that might be painful -- let's try to avoid doing something that prevents it from happening in the future, at least, if possible.

@the8472
Copy link
Member

the8472 commented Jan 1, 2022

To clarify, in theory linkchecker could pass even if the standard library docs were skipped: the problem is that other documentation links to the standard library docs, so if the standard library docs are not there linkchecker will identify those links as broken.

Changing the linkchecker step to always build all docs would indeed solve the specific problem I'm facing when excluding the standard library tests, but would also prevent other use cases: at least I have occasionally done ./x.py test src/tools/linkchecker --exclude $foo to get a list of all links pointing to the $foo documentation, and such change would prevent that. I can imagine there are also other use cases I'm not thinking about.

I still think --exclude is the wrong thing to modify here. Maybe linkchecker itself needs an option to inform it what should be excluded from checking? Via --test-args for example.

@Mark-Simulacrum
Copy link
Member

The motivation (in my eyes) is not necessarily to fix/prevent the link check problem - I agree that has a different solution, likely with an error on excluding "too much/too little" for required dependencies like this. But the desired error there may want to suggest either excluding link check or limiting the standard library exclude, I think - though not sure.

@Mark-Simulacrum 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 Jan 4, 2022
@pietroalbini
Copy link
Member Author

Changed the PR to use Kind and rebased to fix the merge conflict.

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum 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 Jan 10, 2022
@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2022
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

r=me with commits squashed (particularly the last two); I was going to do this myself but it looks like the push permissions aren't setup such that I can do that.

@Mark-Simulacrum 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 Jan 14, 2022
x.py has support for excluding some steps from the invocation, but
unfortunately that's not granular enough: some steps have the same name
in different modules, and that prevents excluding only *some* of them.

As a practical example, let's say you need to run everything in `./x.py
test` except for the standard library tests, as those tests require IPv6
and need to be executed on a separate machine. Before this commit, if
you were to just run this:

    ./x.py test --exclude library/std

...the execution would fail, as that would not only exclude running the
tests for the standard library, it would also exclude generating its
documentation (breaking linkchecker).

This commit adds support for an optional module annotation in --exclude
paths, allowing the user to choose which module to exclude from:

    ./x.py test --exclude test::library/std

This maintains backward compatibility, but also allows for more ganular
exclusion. More examples on how this works:

| `--exclude`         | Docs    | Tests   |
| ------------------- | ------- | ------- |
| `library/std`       | Skipped | Skipped |
| `doc::library/std`  | Skipped | Run     |
| `test::library/std` | Run     | Skipped |

Note that the new behavior only works in the `--exclude` flag, and not
in other x.py arguments or flags yet.
@pietroalbini
Copy link
Member Author

Squashed.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jan 21, 2022

📌 Commit b3ad405 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#91965 (Add more granular `--exclude` in `x.py`)
 - rust-lang#92467 (Ensure that early-bound function lifetimes are always 'local')
 - rust-lang#92586 (Set the allocation MIN_ALIGN for espidf to 4.)
 - rust-lang#92835 (Improve error message for key="value" cfg arguments.)
 - rust-lang#92843 (Improve string concatenation suggestion)
 - rust-lang#92963 (Implement tuple array diagnostic)
 - rust-lang#93046 (Use let_else in even more places)
 - rust-lang#93109 (Improve `Arc` and `Rc` documentation)
 - rust-lang#93134 (delete `Stdin::split` forwarder)
 - rust-lang#93139 (rustdoc: fix overflow-wrap for table layouts)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc69406 into rust-lang:master Jan 22, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 22, 2022
@pietroalbini pietroalbini deleted the pa-more-granular-exclude branch March 17, 2022 09:20
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 10, 2023
…-Simulacrum

bootstrap: Disallow `--exclude test::std`

Use the top-level Kind to determine whether Steps are excluded.

Previously, this would use the `Kind` passed to `--exclude` (and not do any filtering at all if no kind was passed).
That meant that `x test linkchecker --exclude std` would fail - you had to explicitly say `--exclude test::std`.

Change bootstrap to use the top-level Kind instead, which does the right thing automatically.
Note that this breaks things like `x test --exclude doc::std`, but I'm not sure why you'd ever want to do that.

There's a lot of churn here, but the 1-line change in the first commit is the actual behavior change, the rest is just cleanup.

Fixes rust-lang#103201. Note that this effectively reverts most of rust-lang#91965.

cc `@pietroalbini`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants