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

Add unstable rustc-check-cfg build script output #10539

Merged
merged 4 commits into from
May 20, 2022

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Apr 7, 2022

This PR adds a new build script output as unstable behind -Zcheck-cfg=output: rustc-check-cfg.

What does this PR try to resolve?

This PR add a way to add to use the unstable --check-cfg command line option of rustc and rustdoc.

It was discover in Bump bootstrap compiler to 1.61.0 beta that rustc_llvm sets some custom cfg from a build script and because --check-cfg=values() is globally enable in the Rust codebase that cause the compilation to fail. For now no values are checked in stage 0 for the entire codebase which is a shame and should be fixed with the addition of this feature.

How should we test and review this PR?

Commits are separated in: implementation, tests and doc.

Testing should simply be done by adding a valid cargo:rustc-check-cfg in a build script.
Watch the added tests or doc to have an example.

Additional information

This PR is also the logical next step after -Zcheck-cfg-features.

@rust-highfive
Copy link

r? @ehuss

(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 Apr 7, 2022
@bors
Copy link
Contributor

bors commented Apr 12, 2022

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

@bors
Copy link
Contributor

bors commented Apr 28, 2022

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

@bors
Copy link
Contributor

bors commented Apr 28, 2022

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

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

This doesn't seem to pass the --check-cfg flag to rustdoc, was that intentional?

src/doc/src/reference/build-scripts.md Outdated Show resolved Hide resolved
src/cargo/core/compiler/custom_build.rs Outdated Show resolved Hide resolved
tests/testsuite/build_script.rs Outdated Show resolved Hide resolved
@Urgau
Copy link
Member Author

Urgau commented May 3, 2022

This doesn't seem to pass the --check-cfg flag to rustdoc, was that intentional?

No, not at all. Fixed and added a test for it.

I also fixed or responded to the review comments.
The PR should be ready for another review.

@Urgau Urgau force-pushed the check-cfg-build-script branch 2 times, most recently from 100429f to 63ee29f Compare May 7, 2022 09:43
@Urgau
Copy link
Member Author

Urgau commented May 7, 2022

I've updated the PR to use the improvements of #10566.
The tests are now in the check_cfg module, they are now also activated on Windows and this addition is now gated under -Zcheck-cfg=output (ie. the output of the build script, not sure that a great name).

@rustbot ready

src/cargo/core/compiler/custom_build.rs Outdated Show resolved Hide resolved
tests/testsuite/check_cfg.rs Outdated Show resolved Hide resolved
src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
Comment on lines 727 to 732
if !output.check_cfgs.is_empty() {
rustdoc.arg("-Zunstable-options");
for check_cfg in &output.check_cfgs {
rustdoc.arg("--check-cfg").arg(check_cfg);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a little bit of cleanup to add the --cfg flag handling in rustc and rustdoc. It looks like there is a bit of duplication and inconsistency around handling build-script flags (like --cfg, --check-cfg, and env).

A rough outline of the changes I'm thinking:

  • Add a new function to cargo::core::compiler that will handle adding these common flags. I think it might have the signature of something like fn add_custom_flags(cmd: &mut ProcessBuilder, build_script_outputs: &BuildScriptOutputs, metadata: Option<Metadata>)
  • Move all of add_custom_env to that function.
  • Move all of --cfg flag handling to that function.
  • Move all of --check-cfg flag handling to that function.
  • Use that function in both rustc and rustdoc removing the duplicate code.

How does that sound?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ehuss Sound good. Do you want to do it in this PR or as a follow-up ? (I feel like it doesn't really make sense to do it in this PR, but your call)

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is good because it is adding the --check-cfg arg.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (expect for doc-tests because of it's specificity that make it unable to use the new function).

@ehuss
Copy link
Contributor

ehuss commented May 20, 2022

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2022

📌 Commit f2a1e43 has been approved by ehuss

@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 May 20, 2022
@bors
Copy link
Contributor

bors commented May 20, 2022

⌛ Testing commit f2a1e43 with merge c8c6e33...

@bors
Copy link
Contributor

bors commented May 20, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing c8c6e33 to master...

@bors bors merged commit c8c6e33 into rust-lang:master May 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2022
Update cargo

10 commits in a4c1cd0eb6b18082a7e693f5a665548fe1534be4..39ad1039d9e3e1746177bf5d134af4c164f95528
2022-05-20 00:55:25 +0000 to 2022-05-25 00:50:02 +0000

* doc: discuss build script instruction order (rust-lang/cargo#10600)
* Require http-registry URLs to end with a '/' (rust-lang/cargo#10698)
* No printing executable names when running tests and benchmarks with json message format (rust-lang/cargo#10691)
* Restore proper error for crate not in local reg (rust-lang/cargo#10683)
* Update libcurl (rust-lang/cargo#10696)
* Fixed small typos (rust-lang/cargo#10693)
* fix bugs with `workspace` key and `update_toml` (rust-lang/cargo#10685)
* Bump to 0.64.0, update changelog (rust-lang/cargo#10687)
* List C compiler as a build dependency in README (rust-lang/cargo#10678)
* Add unstable `rustc-check-cfg` build script output (rust-lang/cargo#10539)

r? `@ehuss`
@ehuss ehuss added this to the 1.63.0 milestone Jun 1, 2022
bors added a commit that referenced this pull request Oct 11, 2024
…weihanglo

Fix panic when running cargo tree on a package with a cross compiled bindep

### What does this PR try to resolve?

This is an attempt to close out `@rukai's` [PR](#13207 (comment)) for #12358 and #10593 by adjusting the new integration test and resolving merge conflicts.

I have also separated the changes into atomic commits as per [previous review](#13207 (comment)).

### How should we test and review this PR?

The integration test that has been edited here is sufficient, plus the new integration test that confirms a more specific case where `cargo tree` throws an error.

### Additional information

I have confirmed the test `artifact_dep_target_specified` fails on master branch and succeeds on this branch.

The first commit fixes the panic and the integration test. Commits 2 and 3 add other tests that confirm behaviour mentioned in related issues.

Commits:
1. [Fix panic when running cargo tree on a package with a cross compiled bindep](5c5ea78) - fixes some panics and changes the integration test to succeed
2. [test: cargo tree panic on artifact dep target deactivated](ed294ab) - adds test to confirm the behaviour for the specific panic from [#10539 (comment)](#10593 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants