-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Open
Copy link
Labels
A-compiletestArea: The compiletest test runnerArea: The compiletest test runnerA-docsArea: Documentation for any part of the project, including the compiler, standard library, and toolsArea: Documentation for any part of the project, including the compiler, standard library, and toolsC-discussionCategory: Discussion or questions that doesn't represent real issues.Category: Discussion or questions that doesn't represent real issues.
Description
https://rustc-dev-guide.rust-lang.org/tests/directives.html states that I can write ignore-x
wherex
is "A full target triple: aarch64-apple-ios". However, that's not correct -- some target triples work, but not all of them. I realized this when my PR failed because ignore-i686-pc-windows-gnullvm
dies not work, and neither does //@ ignore-i686-unknown-uefi
.
CC @jieyouxu
Metadata
Metadata
Assignees
Labels
A-compiletestArea: The compiletest test runnerArea: The compiletest test runnerA-docsArea: Documentation for any part of the project, including the compiler, standard library, and toolsArea: Documentation for any part of the project, including the compiler, standard library, and toolsC-discussionCategory: Discussion or questions that doesn't represent real issues.Category: Discussion or questions that doesn't represent real issues.
Activity
jieyouxu commentedon Apr 8, 2025
It's technically correct in that I believe those would work if they're included in the directives allow list (lol).
RalfJung commentedon Apr 8, 2025
If every new target has to be added to some allow list, that will never be up-to-date. compiletest should accept all targets supported by rustc then.
jieyouxu commentedon Apr 8, 2025
The intention in the long term is to not have to rely on any allow list, it obviously doesn't scale
RalfJung commentedon Apr 8, 2025
Also, I assume then this is similar for architectures etc? Would be good to document that so that one knows what to do when an architecture is not recognized.
RalfJung commentedon Apr 8, 2025
compiletest already queries rustc for various things, right? Can't it query the target list?
jieyouxu commentedon Apr 8, 2025
There's two parts to this:
ignore-$target
s cfrust/src/tools/compiletest/src/header/cfg.rs
Lines 103 to 107 in c6c1796
jieyouxu commentedon Apr 8, 2025
Yes, it's the same for archs. The valid archs are already computed based on all the builtion-targets combined + some extra archs
rust/src/tools/compiletest/src/header/cfg.rs
Lines 133 to 137 in c6c1796
jieyouxu commentedon Apr 8, 2025
I'll send a PR to document the current impl limitation in rustc-dev-guide.
If you still need those
//@ ignore-$target_tuple
for your PR, then you should only need to add them torust/src/tools/compiletest/src/directive-list.rs
Line 4 in c6c1796
{ignore,only}-{$target_triple,$target_arch}
impl limitation rust-lang/rustc-dev-guide#2319jieyouxu commentedon Apr 8, 2025
rust-lang/rustc-dev-guide#2319
RalfJung commentedon Apr 8, 2025
jieyouxu commentedon Apr 8, 2025
Because I haven't gotten around to fixing that bit yet :)
Really, if none of the directive parsers matched a directive-like line then it should just fail (but it doesn't do that yet).
RalfJung commentedon Apr 8, 2025
jieyouxu commentedon Apr 8, 2025
Exactly. The separate allowlist is an artifact leftover from when I was migrating compiletest directives from
//
to//@
(because the plain comment form had too much false-positive potential that made it too noisy to do proper unknown directive detection) to make sure I don't silently drop/miss directives, which has persisted longer than I wanted to, because I keep getting distracted by various things :D