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

Fix env/cfg set for cargo test and cargo run. #9122

Merged
merged 2 commits into from
Feb 3, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Feb 2, 2021

There are some situations where a build script may need to run multiple times for the same package during the same cargo session. There was a bug in that some of the values in the Compilation struct didn't handle this case. The solution here is to be more careful about how this extra data is associated with Units, instead of assuming a package's build script only runs once.

The things that were not being handled properly:

  • Compilation::extra_env, which is the output of cargo:rustc-env in build scripts. The solution here is to use the Metadata hash which is used elsewhere for distinguishing build script outputs.
  • Compilation::cfgs, which is the output of cargo:rustc-cfg in build scripts and the features to be set, and this is only used for doctests. The solution here is to just add those --cfg flags directly in the Doctest struct.

The situations that cause a build script to be run multiple times:

  • A package needed for both the host and target. In the test here, this was accomplished with a proc-macro (which has to be host) and a cyclical dev dependency, but there are many other ways to trigger this.
  • Something built with different features (with the new feature resolver), usually due to host/target differences.
  • Something built with different profile settings, usually due to host/target differences.

Fixes #9100

@rust-highfive
Copy link

r? @Eh2406

(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 Feb 2, 2021
@@ -51,7 +51,7 @@ pub struct BuildOutput {
/// `Unit` because this structure needs to be shareable between threads.
#[derive(Default)]
pub struct BuildScriptOutputs {
outputs: HashMap<(PackageId, Metadata), BuildOutput>,
outputs: HashMap<Metadata, BuildOutput>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to just be keyed off the Metadata to simplify things. I don't remember why I originally included PackageId, as the PackageId hash is already included in the Metadata. If we are uncomfortable with the possibility of a hash collision, I can switch it back, or we can consider using a stronger hash (like siphash 128?).

@ehuss ehuss force-pushed the fix-multiple-run-custom-build branch from 65bae6d to 3b6b612 Compare February 2, 2021 01:00
@ehuss
Copy link
Contributor Author

ehuss commented Feb 2, 2021

I was also wondering if we should update the documentation for how environment variables are set for cargo test and cargo run. Perhaps just a note that says certain environment variables (such as cargo:rustc-env) are also set for running executables for cargo test and cargo run, but that it is not recommended to rely on that since it ties the executable to Cargo's execution environment? Or maybe just leave it undocumented?

@ehuss ehuss force-pushed the fix-multiple-run-custom-build branch from 3b6b612 to f6f274d Compare February 2, 2021 01:58
@ehuss ehuss force-pushed the fix-multiple-run-custom-build branch from f6f274d to d8673d9 Compare February 2, 2021 03:21
@alexcrichton
Copy link
Member

Looks good to me, thanks!

I think it makes sense to go ahead and document this but discourage usage. We've already got users depending on it (probably accidentally?) so I don't think that we can change it anyway. If you'd prefer to do doc updates in a future PR that's ok too, r=me on this regardless.

@ehuss
Copy link
Contributor Author

ehuss commented Feb 3, 2021

Ok, I added a small note. Thanks Alex! 😄

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 3, 2021

📌 Commit 1607a68 has been approved by alexcrichton

@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 3, 2021
@bors
Copy link
Contributor

bors commented Feb 3, 2021

⌛ Testing commit 1607a68 with merge 3875bbb...

@bors
Copy link
Contributor

bors commented Feb 3, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 3875bbb to master...

@bors bors merged commit 3875bbb into rust-lang:master Feb 3, 2021
CPerezz added a commit to CPerezz/cargo that referenced this pull request Feb 3, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2021
Update cargo

5 commits in e099df243bb2495b9b197f79c19f124032b1e778..34170fcd6e0947808a1ac63ac85ffc0da7dace2f
2021-02-01 16:24:34 +0000 to 2021-02-04 15:52:52 +0000
- Fix permission issue with `cargo vendor`. (rust-lang/cargo#9131)
- Add split-debuginfo profile option (rust-lang/cargo#9112)
- Add RegistryBuilder for tests, and update crates-io error handling. (rust-lang/cargo#9126)
- Add some documentation for index and registry stuff. (rust-lang/cargo#9125)
- Fix env/cfg set for `cargo test` and `cargo run`. (rust-lang/cargo#9122)
@ehuss ehuss added this to the 1.51.0 milestone Feb 6, 2022
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.

Cargo set environment variables provided by build scripts randomly when running tests
5 participants