fix(embedded): Don't create an intermediate manifest #12268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-cli
bors
added a commit
that referenced
this pull request
Jun 14, 2023
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
rustbot
added
the
A-infrastructure
epage
force-pushed
the
direct
branch
5 times, most recently
from
e1348cf
to
eab5985
Compare
33 tasks
|
☔ 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`
|
@weihanglo figured out the windows issue (wasn't making paths absolute correctly) so this is now ready |
weihanglo
approved these changes
Jun 17, 2023
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
That combined with manually |
epage
added a commit
to epage/cargo
that referenced
this pull request
Jun 17, 2023
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
epage
added a commit
to epage/cargo
that referenced
this pull request
Jun 17, 2023
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.
bors
added a commit
that referenced
this pull request
Jun 17, 2023
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.
epage
added a commit
to epage/cargo
that referenced
this pull request
Jun 17, 2023
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.
bors
added a commit
that referenced
this pull request
Jun 17, 2023
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
bors
added a commit
that referenced
this pull request
Jun 17, 2023
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.
epage
added a commit
to epage/cargo
that referenced
this pull request
Jun 17, 2023
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.
epage
added a commit
to epage/cargo
that referenced
this pull request
Jun 19, 2023
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.
bors
added a commit
that referenced
this pull request
Jun 19, 2023
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.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 20, 2023
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`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.lockandCARGO_TARGET_DIRare changing from being next tothe 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.