Skip to content
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

tidy: verify that test revisions with --target have associated needs-llvm-components directives #86272

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jun 13, 2021

This ensures that people who tend to write --target #[no_core] tests don't miss specifying the needs-llvm-components directive. This is necessary for the test suite to pass when LLVM is compiled with a subset of components enabled.

While here I also took the opportunity to implement a more fine-grained handling of the ignore directives, so that they are evaluated for each revision, rather than for the entire test. With this even if people have arm component disabled, only the revision that depends on the arm component will not run.

Fixes #82405

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2021
@rust-log-analyzer

This comment has been minimized.

@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 90c56ea to 54d9fed Compare June 13, 2021 22:05
@rust-log-analyzer

This comment has been minimized.

@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 54d9fed to 25ae80e Compare June 14, 2021 10:50
@rust-log-analyzer

This comment has been minimized.

@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 25ae80e to 6c85ebb Compare June 14, 2021 11:34
@rust-log-analyzer

This comment has been minimized.

@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 6c85ebb to cb94e25 Compare June 14, 2021 12:33
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does make me wonder if we should automatically deduce the component needed based on --target (in the simple cases, at least)...

src/tools/tidy/src/target_specific_tests.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r=me with nit fixed

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2021
@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 76abcb6 to 2eaaca3 Compare June 24, 2021 19:36
nagisa added 3 commits June 24, 2021 23:13
Herein we verify that all of the tests that specify a `--target`
compile-flag, are also annotated with the minimal set of required llvm
components necessary to run that test.
Doesn't work though, because compiletest doesn't process ignores on a
per-revision manner.
Otherwise something that ought to seemingly work like `//[x86]
needs-llvm-components: x86` or `//[nll_beyond]should-fail` do not get
evaluated properly.
@nagisa nagisa force-pushed the nagisa/tidy-llvm-components branch from 2eaaca3 to cfcb2b6 Compare June 24, 2021 20:13
@nagisa
Copy link
Member Author

nagisa commented Jun 24, 2021

@bors r=Mark-Simulacrum

This does make me wonder if we should automatically deduce the component needed based on --target (in the simple cases, at least)...

I had in my head some corner cases that I decided were nasty enough that this idea wouldn't work, but now I don't remember what they were.

@bors
Copy link
Contributor

bors commented Jun 24, 2021

📌 Commit cfcb2b6 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 24, 2021
@bors
Copy link
Contributor

bors commented Jun 24, 2021

⌛ Testing commit cfcb2b6 with merge d4e7cb3...

@bors
Copy link
Contributor

bors commented Jun 25, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing d4e7cb3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail tests that have a --target compile-flag, but not needs-llvm-components
6 participants