fix: set OUT_DIR for all units with build scripts #13204

Rustin170506 · 2023-12-25T12:26:42Z

What does this PR try to resolve?

close #13127

Added a minimal case to cover this behavior.
Set OUT_DIR for all units with build scripts.

How should we test and review this PR?

Check out the unit test.

Additional information

None

rustbot · 2023-12-25T12:26:47Z

r? @ehuss

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

tests/testsuite/test.rs

Rustin170506

🔢 Self-check

tests/testsuite/test.rs

epage · 2023-12-27T17:11:24Z

src/cargo/core/compiler/context/mod.rs

+        // Add `OUT_DIR` to env vars if unit has a build script.
+        let units_with_build_script = &self
+            .bcx
+            .roots
+            .iter()
+            .filter(|unit| self.build_scripts.contains_key(unit))
+            .dedup_by(|x, y| x.pkg.package_id() == y.pkg.package_id())
+            .collect::<Vec<_>>();
+        for unit in units_with_build_script {
+            for dep in &self.bcx.unit_graph[unit] {
+                if dep.unit.mode.is_run_custom_build() {
+                    let out_dir = self
+                        .files()
+                        .build_script_out_dir(&dep.unit)
+                        .display()
+                        .to_string();
+                    let script_meta = self.get_run_build_script_metadata(&dep.unit);
+                    self.compilation
+                        .extra_env
+                        .entry(script_meta)
+                        .or_insert_with(Vec::new)
+                        .push(("OUT_DIR".to_string(), out_dir));
+                }
+            }
+        }


If I'm understanding this, we only really need to set OUT_DIR once per package and the previous code did that be assuming the lib unit was sufficient (but some packages don't have that).

I'm assuming there isn't a more direct way to iterate over packages to find out the necessary information and set it, and instead we have to walk all units and de-dupe down to individual packages?

Yes. I think your understanding is correct. Before https://github.com/rust-lang/cargo/pull/3310/files, we set it for every unit.

ehuss · 2024-01-02T14:59:46Z

@rustbot author

Signed-off-by: hi-rustin <rustin.liu@gmail.com> test: remove the env set by Cargo Signed-off-by: hi-rustin <rustin.liu@gmail.com> Fix Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 · 2024-01-08T13:17:27Z

@rustbot ready

ehuss · 2024-01-08T17:21:11Z

Thanks!

@bors r+

bors · 2024-01-08T17:21:13Z

📌 Commit 6739c7e has been approved by ehuss

It is now in the queue for this repository.

bors · 2024-01-08T17:45:16Z

⌛ Testing commit 6739c7e with merge 0c98d6e...

bors · 2024-01-08T18:26:58Z

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

Update cargo 14 commits in 2ce45605d9db521b5fd6c1211ce8de6055fdb24e..3e428a38a34e820a461d2cc082e726d3bda71bcb 2024-01-04 18:04:13 +0000 to 2024-01-09 20:46:36 +0000 - refactor: replace `iter_join` with `itertools::join` (rust-lang/cargo#13275) - docs(unstable): doc comments for items and fields (rust-lang/cargo#13274) - crates-io: Set `Content-Type: application/json` only for requests with a body payload (rust-lang/cargo#13264) - fix: only inherit workspace package table if the new package is a member (rust-lang/cargo#13261) - feat(cli): add colors to `-Zhelp` console output (rust-lang/cargo#13269) - chore(deps): update msrv (rust-lang/cargo#13266) - refactor(toml): Make it more obvious to update package-dependent fields (rust-lang/cargo#13267) - chore(ci): Fix MSRV:3 updates (rust-lang/cargo#13268) - chore(ci): Shot-in-the-dark fix for MSRV updating (rust-lang/cargo#13265) - fix: set OUT_DIR for all units with build scripts (rust-lang/cargo#13204) - fix(manifest): Provide unused key warnings for lints table (rust-lang/cargo#13262) - test(manifest): Verify we warn on unused workspace.package fields (rust-lang/cargo#13263) - docs(changelog): Call out cargo-new lockfile change (rust-lang/cargo#13260) - chore: Add dependency dashboard (rust-lang/cargo#13255) r? ghost

rustbot assigned ehuss Dec 25, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2023

Rustin170506 force-pushed the rustin-patch-test-out-dir branch from c8ebc18 to 1f496dd Compare December 26, 2023 02:04

Rustin170506 commented Dec 26, 2023

View reviewed changes

tests/testsuite/test.rs Show resolved Hide resolved

Rustin170506 changed the title ~~WIP: test: include a case for setting OUT_DIR~~ fix: set OUT_DIR for all units with build scripts Dec 27, 2023

Rustin170506 marked this pull request as ready for review December 27, 2023 02:50

Rustin170506 force-pushed the rustin-patch-test-out-dir branch from c8d00f0 to 3aaf4ee Compare December 27, 2023 02:53

Rustin170506 commented Dec 27, 2023

View reviewed changes

epage reviewed Dec 27, 2023

View reviewed changes

tests/testsuite/test.rs Outdated Show resolved Hide resolved

epage reviewed Dec 27, 2023

View reviewed changes

tests/testsuite/test.rs Show resolved Hide resolved

epage reviewed Dec 27, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2024

Rustin170506 added 2 commits January 8, 2024 20:58

test: include a case for setting OUT_DIR

8bba4f4

Signed-off-by: hi-rustin <rustin.liu@gmail.com> test: remove the env set by Cargo Signed-off-by: hi-rustin <rustin.liu@gmail.com> Fix Signed-off-by: hi-rustin <rustin.liu@gmail.com>

fix: set OUT_DIR for all units with build scripts

6739c7e

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 force-pushed the rustin-patch-test-out-dir branch from 3aaf4ee to 6739c7e Compare January 8, 2024 13:00

rustbot added the A-testing-cargo-itself Area: cargo's tests label Jan 8, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jan 8, 2024

Rustin170506 requested a review from epage January 8, 2024 13:17

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 Jan 8, 2024

bors merged commit 0c98d6e into rust-lang:master Jan 8, 2024
17 of 18 checks passed

weihanglo mentioned this pull request Jan 10, 2024

Update cargo rust-lang/rust#119794

Merged

ehuss added this to the 1.77.0 milestone Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: set OUT_DIR for all units with build scripts #13204

fix: set OUT_DIR for all units with build scripts #13204

Rustin170506 commented Dec 25, 2023 •

edited

Loading

rustbot commented Dec 25, 2023

Rustin170506 left a comment

epage Dec 27, 2023

Rustin170506 Jan 8, 2024

ehuss commented Jan 2, 2024

Rustin170506 commented Jan 8, 2024

ehuss commented Jan 8, 2024

bors commented Jan 8, 2024

bors commented Jan 8, 2024

bors commented Jan 8, 2024

fix: set OUT_DIR for all units with build scripts #13204

fix: set OUT_DIR for all units with build scripts #13204

Conversation

Rustin170506 commented Dec 25, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 25, 2023

Rustin170506 left a comment

Choose a reason for hiding this comment

epage Dec 27, 2023

Choose a reason for hiding this comment

Rustin170506 Jan 8, 2024

Choose a reason for hiding this comment

ehuss commented Jan 2, 2024

Rustin170506 commented Jan 8, 2024

ehuss commented Jan 8, 2024

bors commented Jan 8, 2024

bors commented Jan 8, 2024

bors commented Jan 8, 2024

Rustin170506 commented Dec 25, 2023 •

edited

Loading