Revert #134209 #134843
Revert #134209 #134843
Conversation
…s, r=jieyouxu" Unfortunately, the present logic is not quite right because `Step`s are allowed to register arbitrary *aliases* (e.g. `library/test` is aliases to `test`), which means that we incorrectly warn on ``` ./x test --exclude test ``` producing ``` WARNING: '/home/joe/repos/rust/test' does not exist. ``` even though this alias (`test`) is indeed a known and handled `--exclude` filter. A proper fix will need to do something like "collect all eligible `Step`s then check `should_run(exclude)`" in order to determine if the exclude filter will trigger for the steps. (Courtesy of jyn pointing this out.) This reverts commit 6cf13b0, reversing changes made to 2846699.
rustbot
added
S-waiting-on-review
|
This PR modifies If appropriate, please update |
in case it helps, this is not actually specific to the Step - bootstrap allows doing this for arbitrary steps rust/src/bootstrap/src/core/builder/mod.rs Line 263 in 27258c6 |
|
Right... |
|
There is no problem other than printing warning, right? I was aware of this already (see #134209 (comment)). It's a warning info not error message, if we think it doesn't make sense we can remove it or print that in verbose mode only. |
| // Never return top-level path here as it would break `--skip` | ||
| // logic on rustc's internal test framework which is utilized | ||
| // by compiletest. | ||
| p | ||
| }) |
There was a problem hiding this comment.
This is exactly why we don't include top-level paths here.
There was a problem hiding this comment.
I think the issue is that --exclude also works for non-compiletest managed test suites, like library/test or library/core.
Anyway, I'll have to look at this a bit more, because I think there's a distinction between how tests/* suites are matched versus how e.g. library/test is matched in --exclude logic...
|
Closing because we can probably just drop the warning |
…zkan bootstrap: drop warning for top-level test suite path check due to false positives The current top-level test suite directory does not exist warning logic doesn't quite handle the more exotic path suffix matches that test filters seem to accept (e.g. `library/test` can be matched with `--exclude test`), so avoid warning on non-existent top-level test suites for now. To avoid false positives, we probably need to query test `Step`s for their `should_run(exclude_filter)` logic. This retains the fix for the Windows path handling (unlike rust-lang#134843). r? `@onur-ozkan`
…zkan bootstrap: drop warning for top-level test suite path check due to false positives The current top-level test suite directory does not exist warning logic doesn't quite handle the more exotic path suffix matches that test filters seem to accept (e.g. `library/test` can be matched with `--exclude test`), so avoid warning on non-existent top-level test suites for now. To avoid false positives, we probably need to query test `Step`s for their `should_run(exclude_filter)` logic. This retains the fix for the Windows path handling (unlike rust-lang#134843). r? `@onur-ozkan`
Reverts validate --skip and --exclude paths #134209.
Reopens #134198.
Unfortunately, the present logic is not quite right because
Steps are allowed to register arbitrary aliases (e.g.library/testis aliased totest), which means that we incorrectly warn onproducing
even though this alias (
test) is indeed a known and handled--excludefilter.A proper fix will need to do something like "collect all eligible
Steps then checkshould_run(exclude)" in order to determine if the exclude filter will trigger for the steps. (Courtesy of jyn pointing this out.)I don't quite have the time to investigate the proper fix atm, so I am posting a revert for now (unless someone wants to look at it).
This reverts commit 6cf13b0, reversing changes made to 2846699.
r? @onur-ozkan