bootstrap: PathSet::check only considers starts_with for --skip flag by jyn514 · Pull Request #148223 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits1 Checks11 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The original motivation for adding this path.starts_with check was to allow things like --skip=src/etc: #133492. But it affects quite a lot more things than just --skip; bootstrap is really not designed for the case when multiple Steps match the same filter (#134919 (comment)). For example, x test src does ... something! Not sure what, but something!
This has already caused several issues; see #135022, #135022, probably others I am forgetting.
Change starts_with to only affect --skip, not anything else.
The immediate motivation for this change was to prevent x doc --open src/doc from opening a dozen different pages (see was_explicitly_invoked for how this is related).
r? bootstrap cc @marcoieni @onur-ozkan @jieyouxu @Zalathar
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
This comment has been minimized.
… flag
The original motivation for adding this path.starts_with check was to
allow things like --skip=src/etc. But it affects quite a lot more
things than just --skip; bootstrap is really not designed for the case
when multiple Steps match the same filter. For example, x test src
does ... something! Not sure what, but something!
Change starts_with to only affect --skip, not anything else.
The original motivation for this was to make it so that x doc src/doc --open doesn't open a dozen different books, but I expect it to fix
various other steps in bootstrap as well.
the failure is because the path("compiler/rustc_codegen_*") in all the codegen backends is now being ignored. that seems ok, actually? I wouldn't expect x test --stage 2 compiler to test codegen backends given the current should_run.
I've added a manual .path("compiler") to each of the backends, restoring the old behavior as a one-off for those specific steps.
As an aside, I'm not sure the snapshot test is testing what it intends to ... I don't think it realizes that the stage 2 compiler is only built as a dependency of the codegen backends.
| } |
|---|
| fn has(&self, needle: &Path, module: Kind) -> bool { |
| fn has(&self, needle: &Path, module: Kind, filter_start: bool) -> bool { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very wary of seeing cryptic booleans threaded through functions that are already have mysterious semantics to begin with.
This seems like it's going to cause a lot of frustration for whoever tries to properly clean up PathSet someday.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to add more doc-comments, or convert it to an enum? I really do think the current behavior is not correct ... Happy to hear other suggestions for fixing it.
The path checking in bootstrap is quite.. interesting 😆 Tbh I don't think it's unreasonable to expect that x test compiler runs tests of everything under the compiler directory, not just compiler tests. The issue is that we use aliases and actual on-disk paths kind of interchangeably, but sometimes there is a meaningful difference.
Ideally, we would revamp the whole system from the ground up based on some agreed-upon reasoning that is well-defined for all our current use-cases, but.. you know 😆
I think that at some places we should be using prefixes, at some places suffixes/prefixes, at some places an exact match, etc., but that's not trivial to do at the moment. For your fix, I would appreciate using at least an enum (as Zalathar mentioned), so that we can make it a bit more explicit how does the matching work.
As an aside, I'm not sure the snapshot test is testing what it intends to ... I don't think it realizes that the stage 2 compiler is only built as a dependency of the codegen backends.
The snapshot tests simply print all non-trivial steps that are executed for a given bootstrap invocation, they don't care whether they are built as a dependency or not. We want to see e.g. if some step is suddenly becoming executed even if only as a dependency.
I think that at some places we should be using prefixes, at some places suffixes/prefixes, at some places an exact match, etc., but that's not trivial to do at the moment. For your fix, I would appreciate using at least an enum (as Zalathar mentioned), so that we can make it a bit more explicit how does the matching work.
I have a concern about spot-fixing the pathset-related handling logic, this is an aspect that I genuinely think we as a team need to take a step back, and survey all the different variations of pathset-like handling we have in bootstrap, before changing its current behavior any further. The pathset-related handling, at least when I briefly had a look, was the product of organic growth over the years, and there's no overarching design philosophy (even if there was, the PathSet abstraction is broken by many other variations of path filter handling).
I understand that this PR only
Change
starts_withto only affect--skip, not anything else.
But I really would encourage us to take a step back and have a closer look at the larger picture surrounding pathset logic before making any changes to its current behavior.
That is to say, usually I would be fine with these kind of fixes, but the PathSet-related logic is so much of a mess that I can almost confidentally say that nobody fully understands it that I rather block this fix on first figuring out what our design for the path filtering -> step dispatching/matching behavior is.
I fully agree that the current behavior surely isn't right, and "what should be considered correct" is the question that I would like the team to discuss.
EDIT: cf. #t-infra/bootstrap > Path filters in bootstrap
Labels
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)