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 BuildScriptOutput when a build script is run multiple times. #7857

Merged
merged 1 commit into from Feb 7, 2020

Conversation

@ehuss
Copy link
Contributor

ehuss commented Feb 2, 2020

When I implemented profile build overrides, I created a scenario where a build script could run more than once for different scenarios. See the test for an example.

However, the BuildScriptOutputs map did not take this into consideration. This caused multiple build script runs to stomp on the output of previous runs. This is further exacerbated by the new feature resolver in #7820 where build scripts can run with different features enabled.

The solution is to make the map key unique for the Unit it is running for. Since this map must be updated on different threads, Unit cannot be used as a key, so I chose just using the metadata hash which is hopefully unique. Most of this patch is involved with the fussiness of getting the correct metadata hash (we want the RunCustomBuild unit's hash). I also added some checks to avoid collisions and assert assumptions.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Feb 2, 2020

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 7, 2020

@bors: r+

Sorry for the delay in getting to this, but looks good to me, thanks for the fix!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2020

📌 Commit 2296af2 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2020

⌛️ Testing commit 2296af2 with merge 3c53211...

bors added a commit that referenced this pull request Feb 7, 2020
Fix BuildScriptOutput when a build script is run multiple times.

When I implemented profile build overrides, I created a scenario where a build script could run more than once for different scenarios.  See the test for an example.

However, the `BuildScriptOutputs` map did not take this into consideration.  This caused multiple build script runs to stomp on the output of previous runs.  This is further exacerbated by the new feature resolver in #7820 where build scripts can run with different features enabled.

The solution is to make the map key unique for the Unit it is running for.  Since this map must be updated on different threads, `Unit` cannot be used as a key, so I chose just using the metadata hash which is hopefully unique.  Most of this patch is involved with the fussiness of getting the correct metadata hash (we want the RunCustomBuild unit's hash).  I also added some checks to avoid collisions and assert assumptions.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 3c53211 to master...

@bors bors merged commit 2296af2 into rust-lang:master Feb 7, 2020
9 of 11 checks passed
9 of 11 checks passed
rust-lang.cargo Build #20200202.2 failed
Details
rust-lang.cargo (macOS) macOS was canceled
Details
homu Test successful
Details
rust-lang.cargo (Linux beta) Linux beta succeeded
Details
rust-lang.cargo (Linux nightly) Linux nightly succeeded
Details
rust-lang.cargo (Linux stable) Linux stable succeeded
Details
rust-lang.cargo (Windows x86_64-msvc) Windows x86_64-msvc succeeded
Details
rust-lang.cargo (build_std) build_std succeeded
Details
rust-lang.cargo (docs) docs succeeded
Details
rust-lang.cargo (resolver) resolver succeeded
Details
rust-lang.cargo (rustfmt) rustfmt succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.