bootstrap: Don't pass Kind to some places that don't need it - #160191
Conversation
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
Thanks, this seems right.
I wonder if Kind was previously used to do... Kind-dependent resolution 😰
@bors r+
| // This order is important for retro-compatibility, as `starts_with` was introduced later. | ||
| p.path.ends_with(needle) || p.path.starts_with(needle) |
There was a problem hiding this comment.
Remark (unrelated to this PR): I really dislike this behavior of both prefix and postfix matching, it's a nightmare, but alas
There was a problem hiding this comment.
It makes sense that you would want both ./x test compiler and ./x test rustc_middle to test the crate compiler/rustc_middle, for example.
But yeah, selector matching is several layers of inside-out spaghetti stacked on top of each other, so it's a nightmare to touch anything at the moment.
There was a problem hiding this comment.
./x test rustc_middle already matches through the crate name rather than path, right? Or did crate name matching get removed at some point?
| @@ -1,137 +1,137 @@ | |||
| --- | |||
| source: src/bootstrap/src/core/builder/cli_paths/tests.rs | |||
| expression: test --skip=tests --skip=coverage-map --skip=coverage-run --skip=library --skip=tidyselftest | |||
There was a problem hiding this comment.
I think this weirdness is because the “expression” isn't actually checked by insta, so it became stale after #159131 which changed the expression but didn't invalidate the actual snapshotted results.
|
Actually, r=me once PR CI is 🍏, just in case |
|
This pull request was unapproved. |
This comment has been minimized.
This comment has been minimized.
|
Oh interesting, does this change linkcheck skipping? 🤔 (Can't dig into this rn, but can check later) |
Given that #91965 mentions linkchecker specifically, this might be a real problem. 😿 |
|
For linkcheck I think we generally need to skip it for stage 1... 🤔 EDIT: i.e. #156792 (but I forgot to backlink to the original PR where this was discussed, argh) |
|
I think something relevant is happening in EDIT: I think it's bogus for that function to be calling |
|
Further notes:
|
|
I'm contemplating a narrower hack where we pass a two-value enum down through That's still super gross, but it might be a net improvement. |
|
Pushed an update with a hack to forcibly disable Not sure if we actually want to do this, but I figure we might as well check whether it would work. |
|
Pushed a different workaround that avoids the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks. I wonder if there are ways to go about untangling this mess, but the changes here still feel like a net improvement.
@bors r+ rollup
| // It's important that we don't just call `run_step_descriptions` here, | ||
| // because that would cause `--skip` handling for actual command-line | ||
| // arguments to inappropriately skip these steps. |
bootstrap: Don't pass `Kind` to some places that don't need it The current CLI step's `Kind` was being passed deep into some selector-matching code that doesn't seem to actually need it. It seems that the kind associated with a step's `ShouldRun` paths/aliases always comes from that step, so proceeding to compare it against the step's kind later can never fail and doesn't achieve anything. As far as I can tell, this is a relic of rust-lang#91965 that (due to subsequent changes) doesn't do anything useful and also doesn't make a lot of conceptual sense. This PR therefore removes the `Kind` parameter from `PathSet::check`, and removes the `Kind` field from `TaskPath`. Touching `TaskPath` causes a lot of churn in snapshot tests, but there don't seem to be any changes in actual behaviour.
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #160381 (comment) |
|
This pull request was unapproved. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I guess Windows didn't want me to switch path-printing from @bors try jobs=i686-msvc-2,aarch64-msvc-2 |
This comment has been minimized.
This comment has been minimized.
bootstrap: Don't pass `Kind` to some places that don't need it try-job: i686-msvc-2 try-job: aarch64-msvc-2
|
Actually, maybe I can get quoted output by doing |
|
I think I'll stick with the unquoted output after all. It's visually cleaner, and it seems unlikely that we'll encounter any ambiguity that requires quoting. Since the post-approval fix was simple (and closer to the status-quo), I feel OK about re-approving. @bors r=jieyouxu rollup=iffy |
Rollup of 5 pull requests Successful merges: - #160143 (Add NEON support for is_ascii and eq_ignore_ascii_case) - #160191 (bootstrap: Don't pass `Kind` to some places that don't need it) - #160386 (rustc_data_structures: Cleanup jobserver initialization) - #160409 (Remove redundant target check from check_eii_impl) - #160410 (Update GitHub Actions to v7.0.1)
Rollup merge of #160191 - Zalathar:unkind, r=jieyouxu bootstrap: Don't pass `Kind` to some places that don't need it The current CLI step's `Kind` was being passed deep into some selector-matching code that doesn't seem to actually need it. It seems that the kind associated with a step's `ShouldRun` paths/aliases always comes from that step, so proceeding to compare it against the step's kind later can never fail and doesn't achieve anything. As far as I can tell, this is a relic of #91965 that (due to subsequent changes) doesn't do anything useful and also doesn't make a lot of conceptual sense. This PR therefore removes the `Kind` parameter from `PathSet::check`, and removes the `Kind` field from `TaskPath`. Touching `TaskPath` causes a lot of churn in snapshot tests, but there don't seem to be any changes in actual behaviour.
View all comments
The current CLI step's
Kindwas being passed deep into some selector-matching code that doesn't seem to actually need it. It seems that the kind associated with a step'sShouldRunpaths/aliases always comes from that step, so proceeding to compare it against the step's kind later can never fail and doesn't achieve anything.As far as I can tell, this is a relic of #91965 that (due to subsequent changes) doesn't do anything useful and also doesn't make a lot of conceptual sense.
This PR therefore removes the
Kindparameter fromPathSet::check, and removes theKindfield fromTaskPath.Touching
TaskPathcauses a lot of churn in snapshot tests, but there don't seem to be any changes in actual behaviour.