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

Warn on redundant --cfg directive when revisions are used #131925

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

clubby789
Copy link
Contributor

r? @jieyouxu

Fixes #131390
Not sure of the best way to test this

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, you can r=me after addressing the nit.

@@ -468,7 +468,13 @@ impl<'test> TestCx<'test> {

if let Some(revision) = self.revision {
let normalized_revision = normalize_revision(revision);
cmd.args(&["--cfg", &normalized_revision]);
let cfg_arg = ["--cfg", &normalized_revision];
if self.props.compile_flags.windows(2).any(|args| args == cfg_arg) {
Copy link
Member

Choose a reason for hiding this comment

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

Remark: IIRC, the compile_flags splitting is naive above (e.g. --meow="foo bar" -> -meow="foo and bar" but it's better than nothing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The flag splitter knows how to handle '--meow=foo bar', but in all other cases it will naïvely split on whitespace yeah.

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

Not sure of the best way to test this

At the moment the directive handling is neigh untestable, which is why I am still figuring out a design to rework it. I'm okay with YOLOing this change in before that is possible, though.

@clubby789 clubby789 removed the PG-exploit-mitigations Project group: Exploit mitigations label Oct 19, 2024
@clubby789
Copy link
Contributor Author

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Oct 19, 2024

📌 Commit d82a21f has been approved by jieyouxu

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 19, 2024
…r=jieyouxu

Warn on redundant `--cfg` directive when revisions are used

r? `@jieyouxu`

Fixes rust-lang#131390
Not sure of the best way to test this
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#116863 (warn less about non-exhaustive in ffi)
 - rust-lang#127675 (Remove invalid help diagnostics for const pointer)
 - rust-lang#131772 (Remove `const_refs_to_static` TODO in proc_macro)
 - rust-lang#131789 (Make sure that outer opaques capture inner opaques's lifetimes even with precise capturing syntax)
 - rust-lang#131795 (Stop inverting expectation in normalization errors)
 - rust-lang#131920 (Add codegen test for branchy bool match)
 - rust-lang#131921 (replace STATX_ALL with (STATX_BASIC_STATS | STATX_BTIME) as former is deprecated)
 - rust-lang#131925 (Warn on redundant `--cfg` directive when revisions are used)
 - rust-lang#131931 (Remove unnecessary constness from `lower_generic_args_of_path`)
 - rust-lang#131932 (use tracked_path in rustc_fluent_macro)
 - rust-lang#131936 (feat(rustdoc-json-types): introduce rustc-hash feature)
 - rust-lang#131939 (Get rid of `OnlySelfBounds`)

Failed merges:

 - rust-lang#131181 (Compiletest: Custom differ)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit efd940d into rust-lang:master Oct 20, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2024
Rollup merge of rust-lang#131925 - clubby789:redundant-revision-cfg, r=jieyouxu

Warn on redundant `--cfg` directive when revisions are used

r? ``@jieyouxu``

Fixes rust-lang#131390
Not sure of the best way to test this
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-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiletest should warn on redundant --cfg compile-flags
6 participants