fix(embeded): Don't pollute the scripts dir with `target/` #12282

epage · 2023-06-17T11:44:39Z

What does this PR try to resolve?

This PR is part of #12207.

This specific behavior was broken in #12268 when we stopped using an intermediate
Cargo.toml file.

Unlike pre-#12268,

We are hashing the path, rather than the content, with the assumption
that people change content more frequently than the path
We are using a simpler hash than blake3 in the hopes that we can get
away with it

Unlike the Pre-RFC demo

We are not forcing a single target dir for all scripts in the hopes
that we get Per-user compiled artifact cache #5931

How should we test and review this PR?

A new test was added specifically to show the target dir behavior, rather than overloading an existing test or making all tests sensitive to changes in this behavior.

Additional information

In the future, we might want to resolve symlinks before we get to this point

This was broken in rust-lang#12268 when we stopped using an intermediate `Cargo.toml` file. Unlike pre-rust-lang#12268, - We are hashing the path, rather than the content, with the assumption that people change content more frequently than the path - We are using a simpler hash than `blake3` in the hopes that we can get away with it Unlike the Pre-RFC demo - We are not forcing a single target dir for all scripts in the hopes that we get rust-lang#5931

rustbot · 2023-06-17T11:44:43Z

r? @ehuss

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

weihanglo

Thanks for the fix.

I notice two things are broken after #12268.

If there is a build.rs under the same directory, cargo <script.rs> will run it.
If there is a Cargo.lock under the same directory, it will be overwritten.

Both of them don't look like desired behavior I believe.

Should I open a new issue for them? It looks like this PR might need to change accordingly when those behavior change.

weihanglo · 2023-06-17T15:33:06Z

src/cargo/core/workspace.rs

+            rel_path.push(&hash[0..2]);
+            rel_path.push(&hash[2..]);


(Not a blocker)
Maybe we don't need this intermediate directory as the short hash is short enough?

Its not about the length of the hash but the potential for the number of children of the parent directory. I can't remember if this is true for all platforms but I think its at least on Windows, you can get some pretty bad performance.

In the prior version, I had a third level instead. Here I'm hoping we can get away with just two.

epage · 2023-06-17T15:52:00Z

Thanks for the fix.

I notice two things are broken after #12268.
* If there is a `build.rs` under the same directory, `cargo <script.rs>` will run it.

* If there is a `build.rs` under the same directory, it will be overwritten.
Both of them don't look like desired behavior I believe.

Should I open a new issue for them? It looks like this PR might need to change accordingly when those behavior change.

Maybe I'm missing something but not seeing why these two affect each other.

weihanglo · 2023-06-17T16:01:52Z

There was a typo. The second one should be Cargo.lock.
(btw, it makes me recall #5707 which proposes to have an optional --lockfile-path flag)

Maybe I'm missing something but not seeing why these two affect each other.

I was thinking that if we go back to put stuff in a central place under ~/.cargo, then we might only need one layout logic fo package root path. No need to specify target-dir separately.

epage · 2023-06-17T16:05:02Z

I was thinking that if we go back to put stuff in a central place under ~/.cargo, then we might only need one layout logic fo package root path. No need to specify target-dir separately.

So move the workspace root instead? I feel like that is a bit misleading.

#12283 is now up for build.rs . I have a branch for the lockfile directory once this PR is merged.

weihanglo · 2023-06-17T16:13:04Z

Make sense! Those issues are not really directly related to this PR. I will go ahead and merge this. Thanks for the clarification!

@bors r+

bors · 2023-06-17T16:13:06Z

📌 Commit aca7b08 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-06-17T16:15:22Z

⌛ Testing commit aca7b08 with merge 81a7392...

bors · 2023-06-17T17:05:57Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 81a7392 to master...

Update cargo 12 commits in 0c14026aa84ee2ec4c67460c0a18abc8519ca6b2..dead4b8740c4b6a8ed5211e37c99cf81d01c3b1c 2023-06-14 18:43:05 +0000 to 2023-06-20 20:07:17 +0000 - Convert valid feature name warning to an error. (rust-lang/cargo#12291) - fix(embedded): Don't pollute script dir with lockfile (rust-lang/cargo#12284) - fix: remove `-Zjobserver-per-rustc` again (rust-lang/cargo#12285) - docs: some tweaks around `cargo test` (rust-lang/cargo#12288) - Enable `doctest-in-workspace` by default (rust-lang/cargo#12221) - fix(embedded): Don't auto-discover build.rs files (rust-lang/cargo#12283) - fix(embeded): Don't pollute the scripts dir with `target/` (rust-lang/cargo#12282) - feat: prepare for the next lockfile bump (rust-lang/cargo#12279) - fix(embedded): Don't create an intermediate manifest (rust-lang/cargo#12268) - refactor(embedded): Switch to `syn` for parsing doc comments (rust-lang/cargo#12258) - fix(embedded): Align package name sanitization with cargo-new (rust-lang/cargo#12255) - Clarify the default behavior of cargo-install. (rust-lang/cargo#12276) r? `@ghost`

epage added 2 commits June 16, 2023 21:17

refactor(core): Split out target-dir defaulting

282a3e7

rustbot assigned ehuss Jun 17, 2023

rustbot added A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2023

epage mentioned this pull request Jun 17, 2023

Tracking Issue for cargo-script RFC 3424 #12207

Open

33 tasks

weihanglo approved these changes Jun 17, 2023

View reviewed changes

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 Jun 17, 2023

bors merged commit 81a7392 into rust-lang:master Jun 17, 2023

epage deleted the target branch June 17, 2023 17:18

weihanglo mentioned this pull request Jun 20, 2023

Update cargo rust-lang/rust#112859

Merged

epage mentioned this pull request Jun 22, 2023

RFC: Templating CARGO_TARGET_DIR to make it the parent of all target directories rust-lang/rfcs#3371

Closed

ehuss added this to the 1.72.0 milestone Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(embeded): Don't pollute the scripts dir with `target/` #12282

fix(embeded): Don't pollute the scripts dir with `target/` #12282

epage commented Jun 17, 2023

rustbot commented Jun 17, 2023

weihanglo left a comment •

edited

Loading

weihanglo Jun 17, 2023

epage Jun 17, 2023

epage commented Jun 17, 2023

weihanglo commented Jun 17, 2023 •

edited

Loading

epage commented Jun 17, 2023 •

edited

Loading

weihanglo commented Jun 17, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

fix(embeded): Don't pollute the scripts dir with target/ #12282

fix(embeded): Don't pollute the scripts dir with target/ #12282

Conversation

epage commented Jun 17, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jun 17, 2023

weihanglo left a comment • edited Loading

Choose a reason for hiding this comment

weihanglo Jun 17, 2023

Choose a reason for hiding this comment

epage Jun 17, 2023

Choose a reason for hiding this comment

epage commented Jun 17, 2023

weihanglo commented Jun 17, 2023 • edited Loading

epage commented Jun 17, 2023 • edited Loading

weihanglo commented Jun 17, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

fix(embeded): Don't pollute the scripts dir with `target/` #12282

fix(embeded): Don't pollute the scripts dir with `target/` #12282

weihanglo left a comment •

edited

Loading

weihanglo commented Jun 17, 2023 •

edited

Loading

epage commented Jun 17, 2023 •

edited

Loading