-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
compiletest: add implied needs-target-std for codegen mode tests unless annotated with #![no_std]/#![no_core]
#151294
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
Conversation
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
| By default, codegen tests will infer `//@ needs-target-std` (that the target | ||
| needs to support std), *unless* the `#![no_std]` attribute was specified in the | ||
| test source. You can override this behavior and explicitly write `//@ | ||
| needs-target-std` to only run the test when target supports std, even if the | ||
| test is `#![no_std]`. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: note that design wise, I still don't really like this kind of inferred implicit behavior, but OTOH I agree it is really annoying having to maintain explicit //@ needs-target-std for codegen tests.
101f610 to
c2d972b
Compare
needs-target-std for codegen-{gcc,llvm,cranelift} testsneeds-target-std for codegen mode tests
needs-target-std for codegen mode testsneeds-target-std for codegen mode tests without #![no_std]
|
cc @xdoardo, lmw if you want an |
| } | ||
|
|
||
| if let Some(directive_line) = line_directive(path, line_number, ln) { | ||
| lines.push(directive_line); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked too much into it, but I noticed that the testing infrastructure looks at the directives in reverse to find reasons to ignore the test for the current target.
This means, for example, that a test like this will be ignored with the "target needs std" message if it is run for a target that does not have std and is not a 64-bit platform.
It could make sense to add the synthetic directive at the beginning of the vector instead, so that the (potential) reasons to ignore the test for the current target that are already in the file are checked first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems okay, but also I think that's an orthogonal change which can benefit from a more aggressive change: I wonder if we should report the set of criteria that contributes to a test being ignored, rather than just the "first" or "last" directive.
|
I had a couple patches to push to my PR, but I am happy to close mine and comment here the bits I had patches for.
That would be great! It's not that important, though. I'm fine without the attribution if it is a hassle. |
|
@rustbot author |
|
Terminology nitpick: This usage of “infer” feels subtly wrong to me, in a way that trips me up every time I read it. E.g. if I were the one writing, I would be more likely to say that we assume the need for an implicit/implied |
needs-target-std for codegen mode tests without #![no_std]needs-target-std for codegen mode tests unless explicit directive or annotated with #![no_std]
needs-target-std for codegen mode tests unless explicit directive or annotated with #![no_std]needs-target-std for codegen mode tests unless annotated with #![no_std]
needs-target-std for codegen mode tests unless annotated with #![no_std]needs-target-std for codegen mode tests unless annotated with #![no_{std,core}]
c2d972b to
8923124
Compare
This comment has been minimized.
This comment has been minimized.
8923124 to
9cfd996
Compare
|
Changes since last review:
@rustbot review |
needs-target-std for codegen mode tests unless annotated with #![no_{std,core}]needs-target-std for codegen mode tests unless annotated with #![no_std]/#![no_core]
|
Looks good to me, thanks. I notice there are still a few occurrences of “infer” left over; r=me after another cleanup pass to make those more consistent. |
|
@rustbot author |
…tests by default A `codegen-llvm` test (and other codegen test mode tests) will now by default have an implied `//@ needs-target-std` directive, *unless* the test explicitly has an `#![no_std]`/`#![no_core]` attribute which disables this implied behavior. - When a test has both `#![no_std]`/`#![no_core]` and `//@ needs-target-std`, the explicit `//@ needs-target-std` directive will cause the test to be ignored for targets that do not support std still. This is to make it easier to test out-of-tree targets / custom targets (and targets not tested in r-l/r CI) without requiring target maintainers to do a bunch of manual `//@ needs-target-std` busywork. Co-authored-by: Edoardo Marangoni <ecmm@anche.no>
9cfd996 to
841d781
Compare
|
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. |
needs-target-std for codegen mode tests unless annotated with #![no_std]/#![no_core]needs-target-std for codegen mode tests unless annotated with #![no_std]/#![no_core]
|
Fixed the leftover "inferred" that I apparently missed, no functional changes since last review. |
Rollup of 6 pull requests Successful merges: - #151611 (Improve is_ascii performance on x86_64 with explicit SSE2 intrinsics) - #150705 (Add missing mut to pin.rs docs) - #151294 (compiletest: add implied `needs-target-std` for `codegen` mode tests unless annotated with `#![no_std]`/`#![no_core]`) - #151589 (Add a `documentation` remapping path scope for rustdoc usage) - #151639 (Fix broken WASIp1 reference link) - #151645 (Update `sysinfo` version to `0.38.0`)
Rollup merge of #151294 - jieyouxu:infer-needs-target-std, r=Zalathar compiletest: add implied `needs-target-std` for `codegen` mode tests unless annotated with `#![no_std]`/`#![no_core]` A `codegen` mode test (such as `codegen-llvm` test suite) will now by default have an implied `//@ needs-target-std` directive, *unless* the test explicitly has an `#![no_std]`/`#![no_core]` attribute which disables this behavior. - When a test has both `#![no_std]`/`#![no_core]` and `//@ needs-target-std`, the explicit `//@ needs-target-std` directive will cause the test to be ignored for targets that do not support std still. This is to make it easier to test out-of-tree targets / custom targets (and targets not tested in r-l/r CI) without requiring target maintainers to do a bunch of manual `//@ needs-target-std` busywork. Context: [#t-compiler/help > `compiletest` cannot find `core` library for target != host](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/.60compiletest.60.20cannot.20find.20.60core.60.20library.20for.20target.20!.3D.20host/with/568652419) ## Implementation remarks This is an alternative version of #150672, with some differences: - *This* PR applies this implied-`needs-target-std` behavior to all `codegen` test mode tests. - *This* PR does the synthetic directive injection in the same place as implied-`codegen-run` directives. Both are of course hacks, but at least they're together next to each other.
Rollup of 6 pull requests Successful merges: - rust-lang/rust#151611 (Improve is_ascii performance on x86_64 with explicit SSE2 intrinsics) - rust-lang/rust#150705 (Add missing mut to pin.rs docs) - rust-lang/rust#151294 (compiletest: add implied `needs-target-std` for `codegen` mode tests unless annotated with `#![no_std]`/`#![no_core]`) - rust-lang/rust#151589 (Add a `documentation` remapping path scope for rustdoc usage) - rust-lang/rust#151639 (Fix broken WASIp1 reference link) - rust-lang/rust#151645 (Update `sysinfo` version to `0.38.0`)
A
codegenmode test (such ascodegen-llvmtest suite) will now by default have an implied//@ needs-target-stddirective, unless the test explicitly has an#![no_std]/#![no_core]attribute which disables this behavior.#![no_std]/#![no_core]and//@ needs-target-std, the explicit//@ needs-target-stddirective will cause the test to be ignored for targets that do not support std still.This is to make it easier to test out-of-tree targets / custom targets (and targets not tested in r-l/r CI) without requiring target maintainers to do a bunch of manual
//@ needs-target-stdbusywork.Context: #t-compiler/help > `compiletest` cannot find `core` library for target != host
Implementation remarks
This is an alternative version of #150672, with some differences:
needs-target-stdbehavior to allcodegentest mode tests.codegen-rundirectives. Both are of course hacks, but at least they're together next to each other.