Skip to content

compiletest directives docs for "ignore-" are incorrect or confusing #139516

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

Open
RalfJung opened this issue Apr 8, 2025 · 13 comments
Open

compiletest directives docs for "ignore-" are incorrect or confusing #139516

RalfJung opened this issue Apr 8, 2025 · 13 comments
Labels
A-compiletest Area: The compiletest test runner A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 8, 2025

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

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 8, 2025
@jieyouxu
Copy link
Member

jieyouxu commented 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
Copy link
Member Author

RalfJung commented 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
Copy link
Member

jieyouxu commented 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
Copy link
Member Author

RalfJung commented 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
Copy link
Member Author

RalfJung commented Apr 8, 2025

The intention in the long term is to not have to rely on any allow list, it obviously doesn't scale

compiletest already queries rustc for various things, right? Can't it query the target list?

@jieyouxu
Copy link
Member

jieyouxu commented Apr 8, 2025

There's two parts to this:

  1. compiletest already does the target list query. Those targets are already valid ignore-$targets cf
    condition! {
    name: &config.target,
    allowed_names: &target_cfgs.all_targets,
    message: "when the target is {name}"
    }
    .
  2. But they just get denied by the allow-list (which is a short-term bandaid to detect unknown directives) that I want to get rid of soon

@jieyouxu
Copy link
Member

jieyouxu commented 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.

Yes, it's the same for archs. The valid archs are already computed based on all the builtion-targets combined + some extra archs

condition! {
name: &target_cfg.arch,
allowed_names: ContainsEither { a: &target_cfgs.all_archs, b: &EXTRA_ARCHS },
message: "when the architecture is {name}"
}

@jieyouxu jieyouxu added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-compiletest Area: The compiletest test runner and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 8, 2025
@jieyouxu
Copy link
Member

jieyouxu commented 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 to

const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
.

@jieyouxu
Copy link
Member

jieyouxu commented Apr 8, 2025

@RalfJung
Copy link
Member Author

RalfJung commented Apr 8, 2025 via email

@jieyouxu
Copy link
Member

jieyouxu commented Apr 8, 2025

Why can't they be added to the allowlist automatically? We should be rejecting unknown directives, that is not a bandaid. But targets on the builtin target list are not "unknown".

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
Copy link
Member Author

RalfJung commented Apr 8, 2025 via email

@jieyouxu
Copy link
Member

jieyouxu commented 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools
Projects
None yet
3 participants