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.

Sign up for GitHub

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

Jump to bottom

Don't allow test revisions that conflict with built in cfgs #131930

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Don't allow test revisions that conflict with built in cfgs #131930

merged 1 commit into from
Oct 24, 2024
+24 −2

Conversation

clubby789
Copy link
Contributor

Fixes #128964

Sorry @heysujal I started working on this about 1 minute before your comment by complete coincidence 😅

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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

jieyouxu
jieyouxu reviewed Oct 19, 2024
View reviewed changes
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, this LGTM with one suggestion.

src/tools/compiletest/src/common.rs Outdated
Comment on lines 378 to 379
pub target_cfgs: OnceLock<TargetCfgs>,
pub builtin_cfgs: OnceLock<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

Remark: I feel like this is kinda overkill, but I'll see if this is even needed during the rework

src/tools/compiletest/src/common.rs Outdated Show resolved Hide resolved
@jieyouxu jieyouxu assigned jieyouxu and unassigned Mark-Simulacrum Oct 19, 2024
@heysujal
Copy link

Hi @clubby789, its fine. The issue didn't had any comment of anyone working on it, so I thought its up for grabs.
Can you explain your approach of fixing this?
I looked at your changes and want to understand how did you figure out where and what changes to make.

jieyouxu
jieyouxu approved these changes Oct 19, 2024
View reviewed changes
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 once PR CI is green.

@clubby789
Copy link
Contributor Author

clubby789 commented Oct 19, 2024 •
edited
Loading

Can you explain your approach of fixing this?

@heysujal
As suggested by Urgau on the original issue, we can use --print=check-cfg to gather all builtin cfg names. When running a test with a revision, we check if that revision is in the list and panic with an error if it is. Otherwise, we continue with the old behaviour of adding a --cfg argument for it

@Urgau
Copy link
Member

Urgau commented Oct 19, 2024 •
edited
Loading

This is already covered by the deny-by-default explicit_builtin_cfgs_in_flags lint.

What is the advantage over the native check in rustc?

Also the test cfg is a bit special.

@clubby789
Copy link
Contributor Author

I could see that error getting easily lost in a compile-fail test. test could make sense to remove though

@jieyouxu
Copy link
Member

This is already covered by the deny-by-default explicit_builtin_cfgs_in_flags lint.

What is the advantage over the native check in rustc?

The preset handling in compiletest is such that revisions are handled as early props but compile flagd are handled at late props. Revisions set cfgs of revision names implicitly and its very confusing for the end user seeing the lint fire only when the test is about to run, I would like revision names that share same name as builtin cfg names to get rejected earlier so it's obvious it's an invalid revision name.

Urgau
Urgau reviewed Oct 19, 2024
View reviewed changes
src/tools/compiletest/src/common.rs
Default::default(),
)
.lines()
.map(|l| if let Some((name, _)) = l.split_once('=') { name.to_string() } else { l.to_string() })
Copy link
Member

Choose a reason for hiding this comment

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

Just a FWIW, this parsing is quite primitive, it doesn't handle the --print=check-cfg specifics, fortunately this is only collecting names and shouldn't be in any specific case so it should always work.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is only querying for the cfg name, otherwise this can't be used

@jieyouxu
Copy link
Member

jieyouxu commented Oct 19, 2024 •
edited
Loading

Also @Urgau good point about test, let's ban that for compiletest even if its not part of builtin cfgs anymore in the future.

@bors
Copy link
Contributor

bors commented Oct 20, 2024

☔ The latest upstream changes (presumably #131948) made this pull request unmergeable. Please resolve the merge conflicts.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 20, 2024 •
edited
Loading

@bors r- (needs a rebase and explicitly adding test as a banned revision name, r=me after)

@jieyouxu jieyouxu 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 Oct 20, 2024
@clubby789
Copy link
Contributor Author

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Oct 23, 2024

📌 Commit 2e3091d has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 23, 2024
@Zalathar Zalathar mentioned this pull request Oct 24, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@bors
Auto merge of rust-lang#132094 - Zalathar:rollup-5r1ppqt, r=Zalathar
55b7f8e 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#130225 (Rename Receiver -> LegacyReceiver)
 - rust-lang#131169 (Fix `target_vendor` in QNX Neutrino targets)
 - rust-lang#131623 (misc cleanups)
 - rust-lang#131756 (Deeply normalize `TypeTrace` when reporting type error in new solver)
 - rust-lang#131898 (minor `*dyn` cast cleanup)
 - rust-lang#131909 (Prevent overflowing enum cast from ICEing)
 - rust-lang#131930 (Don't allow test revisions that conflict with built in cfgs)
 - rust-lang#131956 (coverage: Pass coverage mappings to LLVM as separate structs)
 - rust-lang#132076 (HashStable for rustc_feature::Features: stop hashing compile-time constant)
 - rust-lang#132088 (Print safety correctly in extern static items)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f7f411d into rust-lang:master Oct 24, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@rust-timer
Unrolled build for rust-lang#131930
6bd35a3 
Rollup merge of rust-lang#131930 - clubby789:revision-cfg-collide, r=jieyouxu

Don't allow test revisions that conflict with built in cfgs

Fixes rust-lang#128964

Sorry `@heysujal` I started working on this about 1 minute before your comment by complete coincidence 😅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Urgau Urgau Urgau left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

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
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.

compiletest: we should not allow revision names that collide with builtin cfg names
7 participants
@clubby789 @rustbot @heysujal @Urgau @jieyouxu @bors @Mark-Simulacrum