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

Enable conditional compilation checking on the Rust codebase #94298

Merged
merged 3 commits into from
Mar 5, 2022
Merged

Enable conditional compilation checking on the Rust codebase #94298

merged 3 commits into from
Mar 5, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Feb 23, 2022 •
edited by Mark-Simulacrum
Loading

This pull-request enable conditional compilation checking on every rust project build by the bootstrap tool.

To be more specific, this PR only enable well known names checking + extra names (bootstrap, parallel_compiler, ...).

r? @Mark-Simulacrum

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Urgau
Copy link
Member Author

Urgau commented Feb 23, 2022

CI failure is unrelated to my changes and will most certainly will be fixed by #94290.

Mark-Simulacrum
Mark-Simulacrum reviewed Feb 25, 2022
View reviewed changes
src/bootstrap/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/lib.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Urgau
Copy link
Member Author

Urgau commented Feb 25, 2022 •
edited
Loading

I didn't expect it but it seems that the rustflags var also affects rustdoc, however rustdoc doesn't yet have the support for --check-cfg, however #94154 is already approved so this should not be blocked for long.

@Urgau Urgau force-pushed the rustbuild-check-cfg branch 2 times, most recently from 07ed339 to ff91509 Compare February 26, 2022 00:31
@Urgau
Copy link
Member Author

Urgau commented Feb 26, 2022 •
edited
Loading

With the rustdoc support landed and after adding an allow in b42b555. The changes now pass.
I believe this is ready for another review.

@rustbot ready

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

Thanks, this looks great.

@bors
Copy link
Contributor

bors commented Feb 26, 2022

📌 Commit ff9150997362e09ea4a0ee82766ba27fef180522 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2022
@bors
Copy link
Contributor

bors commented Feb 27, 2022

⌛ Testing commit ff9150997362e09ea4a0ee82766ba27fef180522 with merge a761f89db6cc66a453172fc129832370421fc1ac...

@bors
Copy link
Contributor

bors commented Feb 27, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 27, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Urgau
Copy link
Member Author

Urgau commented Feb 27, 2022 •
edited
Loading

I miss the freebsd12 and backtrace_in_std config when looking at the source code. Fixed now.

However I don't think this will pass because stdarch use this voluntarily invalid cfg dont_compile_me in their test, but we don't allow it.
@Mark-Simulacrum @Amanieu Advise on how to "fix"/solve this ? Temporally allow dont_compile_me ?

@Mark-Simulacrum
Copy link
Member

Yes, I would add it to our list here.

@Urgau Urgau force-pushed the rustbuild-check-cfg branch 2 times, most recently from bcc955e to 97cde42 Compare March 3, 2022 13:07
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Urgau
Copy link
Member Author

Urgau commented Mar 3, 2022

Finally! I had to add more exceptions, mainly for dependencies. This is ready for another review.

@Mark-Simulacrum would you happen to know why I don't get those error locally ?
It is all the more surprising that the CIs did not have them before.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2022
@bors
Copy link
Contributor

bors commented Mar 4, 2022

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

Mark-Simulacrum
Mark-Simulacrum reviewed Mar 4, 2022
View reviewed changes
src/bootstrap/lib.rs
// FIXME: Used by crossbeam-utils, but we should not be triggering on external dependencies.
(Some(Mode::Rustc), "crossbeam_loom", None),
(Some(Mode::ToolRustc), "crossbeam_loom", None),
// FIXME: Used by proc-macro2, but we should not be triggering on external dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to document this in the tracking issue for this feature, and consider if there's some kind of crate-level opt-in that can be added -- I suspect in many cases folks will want to have this sort of crate-private cfg without end users needing to opt-in to it.

It's also the case that the proc-macro2 library for example I think intends for this to be used by users, but doesn't expose it as a Cargo feature to avoid accidental use (e.g., by a library that enables that feature), forcing the 'last' user to actually pass the relevant cfg.

I think this is fine for this PR but we should consider this as part of the feature development process, especially if and when stabilization is considered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely, it should be mentioned. I don't have the permissions to edit it but fell free to do so.

I would also mention that were are getting those because we are using RUSTFLAGS which applies to all crates instead of just the one we control. We are currently forced to do it this way because the cargo integration isn't done or even design. It wasn't really discus in the RFC as it was put in the unresolved section with all the cargo stuff. Nevertheless I have some ideas about how we could have a better integration with cargo.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Mar 4, 2022

📌 Commit 976fdb1 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2022
@bors
Copy link
Contributor

bors commented Mar 4, 2022

⌛ Testing commit 976fdb1 with merge 5a7e4c6...

@bors
Copy link
Contributor

bors commented Mar 5, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2022
@bors bors merged commit 5a7e4c6 into rust-lang:master Mar 5, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 5, 2022
@bors bors mentioned this pull request Mar 5, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5a7e4c6): comparison url.

Summary: This benchmark run did not return any relevant results. 1 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@Urgau Urgau deleted the rustbuild-check-cfg branch May 5, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@wesleywiser wesleywiser wesleywiser left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Urgau @rust-log-analyzer @Mark-Simulacrum @bors @Aaron1011 @rust-timer @wesleywiser @rust-highfive @rustbot