fix(embedded): Don't create an intermediate manifest by epage · Pull Request #12268 · rust-lang/cargo

epage · 2023-06-14T14:52:07Z

What does this PR try to resolve?

More immediately, this is to unblock rust-lang/rust#112601

More generally, this gets us away from hackily writing out an out-of-line manifest from an embedded manifest. To parse the manifest, we have to write it out so our regular manifest
loading code could handle it. This updates the manifest parsing code to
handle it.

This doesn't mean this will work everywhere in all cases though. For
example, ephemeral workspaces parses a manifest from the SourceId and
these won't have valid SourceIds.

As a consequence, Cargo.lock and CARGO_TARGET_DIR are changing from being next to
the temp manifest to being next to the script. This still isn't the
desired behavior but stepping stones.

How should we test and review this PR?

A Commit at a time

Additional information

In production code, this does not conflict with #12255 (due to #12262) but in test code, it does.

rustbot · 2023-06-14T14:52:12Z

r? @ehuss

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

weihanglo

CI is failing. Need a fix?

src/cargo/util/toml/mod.rs

tests/testsuite/script.rs

fix(embedded): Don't append hash to bin names ### What does this PR try to resolve? More immediately, this is to unblock rust-lang/rust#112601 The hash existed for sharing a target directory. That code isn't implemented yet and a per-user build cache might remove the need for it, so let's remove it for now and more carefully weigh adding it back in. In general, this is also the more appropriate way for a feature that would be first class. ### How should we test and review this PR? This originally built on #12268 but now stands alone as the other PR has windows issues to work out ### Additional information

bors · 2023-06-17T02:07:08Z

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

To parse the manifest, we have to write it out so our regular manifest loading code could handle it. This updates the manifest parsing code to handle it. This doesn't mean this will work everywhere in all cases though. For example, ephemeral workspaces parses a manifest from the SourceId and these won't have valid SourceIds. As a consequence, `Cargo.lock` and `CARGO_TARGET_DIR` are changing from being next to the temp manifest to being next to the script. This still isn't the desired behavior but stepping stones. This also exposes the fact that we didn't disable `autobins` like the documentation says we should.

This mirrors the logic `ArgMatchesExt::root_manifest`

epage · 2023-06-17T02:14:17Z

@weihanglo figured out the windows issue (wasn't making paths absolute correctly) so this is now ready

weihanglo

cargo_util::paths::normalize_path did the trick, right? Thanks for the update!

weihanglo · 2023-06-17T08:08:45Z

@bors r+

bors · 2023-06-17T08:08:47Z

📌 Commit 224d2e6 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-06-17T08:08:53Z

⌛ Testing commit 224d2e6 with merge 0d5370a...

bors · 2023-06-17T08:57:24Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 0d5370a to master...

epage · 2023-06-17T11:36:52Z

cargo_util::paths::normalize_path did the trick, right? Thanks for the update!

That combined with manually joining with the CWD

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

With rust-lang#12268, we moved the manifest root to be the scripts parent directory, making it so auto-discovery might pick some things up. We previously ensured `auto*` don't pick things up but missed `build.rs` This is now addressed.

fix(embedded): Don't auto-discover build.rs files With #12268, we moved the manifest root to be the scripts parent directory, making it so auto-discovery might pick some things up. We previously ensured `auto*` don't pick things up but missed `build.rs` This is now addressed.

With rust-lang#12268, we moved the manifest root to be the scripts parent directory, making it so auto-discovery might pick some things up. We previously ensured `auto*` don't pick things up but missed `build.rs` This is now addressed.

fix(embeded): Don't pollute the scripts dir with `target/` ### 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 #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

fix(embedded): Don't auto-discover build.rs files With #12268, we moved the manifest root to be the scripts parent directory, making it so auto-discovery might pick some things up. We previously ensured `auto*` don't pick things up but missed `build.rs` This is now addressed.

This puts the lockfile back into a target directory in the users home, like before rust-lang#12268. Another idea that came up was to move the workspace root to be in the target directory (which would effectively be like pre-rust-lang#12268) but I think that is a bit hacky / misleading. This does mean that the lockfile is buried away from the user and they can't pass it along with their script. In most cases I've dealt with, this would be fine. When the lockfile is needed, they will also most likely have a workspace, so it shoud have a local lockfile in that case. The inbetween case is something that needs further evaluation for whether we should handle it and how.

fix(embedded): Don't pollute script dir with lockfile ### What does this PR try to resolve? This puts the lockfile back into a target directory in the users home, like before #12268. Another idea that came up was to move the workspace root to be in the target directory (which would effectively be like pre-#12268) but I think that is a bit hacky / misleading. This does mean that the lockfile is buried away from the user and they can't pass it along with their script. In most cases I've dealt with, this would be fine. When the lockfile is needed, they will also most likely have a workspace, so it shoud have a local lockfile in that case. The inbetween case is something that needs further evaluation for whether we should handle it and how. ### How should we test and review this PR? Its a bit difficult to test for the lockfile in the new location, so this is mostly being tested in that the lockfile no longer exists next to the script.

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`

rustbot assigned ehuss Jun 14, 2023

rustbot added A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces Command-package Command-run S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2023

epage mentioned this pull request Jun 14, 2023

fix(embedded): Don't append hash to bin names #12269

Merged

weihanglo reviewed Jun 14, 2023

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

tests/testsuite/script.rs Show resolved Hide resolved

epage force-pushed the direct branch from d2d1811 to e56b873 Compare June 14, 2023 17:19

epage force-pushed the direct branch from e56b873 to 4fc97fe Compare June 14, 2023 19:27

rustbot added the A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. label Jun 14, 2023

epage force-pushed the direct branch 5 times, most recently from e1348cf to eab5985 Compare June 15, 2023 17:08

epage marked this pull request as draft June 16, 2023 00:14

epage mentioned this pull request Jun 16, 2023

Tracking Issue for cargo-script RFC 3424 #12207

Open

39 tasks

epage force-pushed the direct branch from 2ef9466 to dc529db Compare June 17, 2023 01:24

epage added 3 commits June 16, 2023 21:06

refactor(embedded): Be accurate in variable name

4fe39e8

fix(toml)!: Prevent people parsing manifests from strings

fc0bb8b

test(embedded): Verify no autobin behavior is running

2c70983

epage added 3 commits June 16, 2023 21:08

fix(embedded): Ensure we don't auto-discover any targets

5ed0792

fix(cli): Make 'cargo foo.rs' behave like --manifest-path

224d2e6

This mirrors the logic `ArgMatchesExt::root_manifest`

epage force-pushed the direct branch from dc529db to 224d2e6 Compare June 17, 2023 02:12

epage marked this pull request as ready for review June 17, 2023 02:12

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 0d5370a into rust-lang:master Jun 17, 2023

epage deleted the direct branch June 17, 2023 11:36

epage mentioned this pull request Jun 17, 2023

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

Merged

epage mentioned this pull request Jun 17, 2023

fix(embedded): Don't auto-discover build.rs files #12283

Merged

epage mentioned this pull request Jun 17, 2023

fix(embedded): Don't pollute script dir with lockfile #12284

Merged

weihanglo mentioned this pull request Jun 20, 2023

Update cargo rust-lang/rust#112859

Merged

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(embedded): Don't create an intermediate manifest#12268

fix(embedded): Don't create an intermediate manifest#12268
bors merged 6 commits intorust-lang:masterfrom
epage:direct

epage commented Jun 14, 2023

Uh oh!

rustbot commented Jun 14, 2023

Uh oh!

weihanglo left a comment

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 17, 2023

Uh oh!

epage commented Jun 17, 2023

Uh oh!

weihanglo left a comment

Uh oh!

weihanglo commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

epage commented Jun 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

epage commented Jun 14, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Jun 14, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 17, 2023

Uh oh!

epage commented Jun 17, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

bors commented Jun 17, 2023

Uh oh!

epage commented Jun 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants