-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
eRFC: single-file packages ("cargo script") integration #3424
Merged
+585
−0
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
a165195
feat: Start cargo-script eRFC
epage 423a195
feat: Initial draft of cargo-script eRFC
epage c66632d
fix: Small tweaks
epage 3cf8f43
fix: Rename file for RFC number
epage 8422cb3
fix: Enter RFC number
epage 4b56dd4
fix: Spelling of shebang
epage 14009d1
fix: Remove stale referenve to cargo-eval
epage 48aa53f
fix: Improve guide's dependency example
epage 9f8a951
fix: Specify the lock location
epage db76c87
fix: Call out --manifest-path verbosity
epage 401ceb0
fix: Explicitly declare its a doc comment
epage d86f217
fix: Be even more explicit on doc comments
epage 755ddb6
Update tracking issue
ehuss File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? Where is this cached? Where does the equivalent of the lockfile live?
This seems a little too opaque.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was intentional as lockfiles are another big area for us to try to figure out the right way to handle them is. The Pre-RFC also has a section for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not suggesting the eRFC has to fully specify embedded lockfiles or any of the many other aspects discussed in the internals thread. I just think if we're going to state that this is cached and updated, we should give some idea of whether this is stored in the cargo home directory (which I'd expect), or next to the script (which I hope it isn't), or similar.
(Also, on a separate note, can we make just
cargo update hello_world.rs
work?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
My understanding was that cargo intentionally avoided positional arguments (
cargo add
being an exception) and one reason for this is due to the heavy use ofnum_args(0..=1)
arguments like--timings
.In the Pre-RFC, I did bring up whether we should provide a short-flag for
--manifest-path
because of how cumbersome it is to use for these files that aren't auto-discovered.I'll at least call this out as another area to explore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epage I just read https://github.com/epage/rfcs/blob/cargo-script/text/3424-cargo-script.md, but couldn't find a place where it mentions what happens with lock files at all. I was searching for
lock
string after reading the whole document. Did I somehow miss it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I didn’t use
cargo metadata
before. It will do the trick, albeit in a format that will be unfamiliar for many people.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at both
cargo metadata
andcargo tree
to see if everything contained in a lock file is present. The one thing that I see missing ischecksum
field. I think it can be pretty useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that
checksum
is internal implementation checksum useful only to cargo, and is not that useful to end users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I don't see mentioned is whether there's only a single lockfile per system, or one per script. If there's a lockfile per script, what happens if a script is renamed? Does that reset all the dependency versions? What about moving a script file to a different directory without renaming it?
Phrased differently, the document says "Cargo will cache the exact information" but doesn't specify what key the cache is indexed by.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like we'll need
cargo venv
lol