vendor: Document how to terminate --sync list #10441
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| @@ -27,7 +27,7 @@ to use the vendored sources, which you will need to add to `.cargo/config.toml`. | |||
|
|
|||
| {{#option "`-s` _manifest_" "`--sync` _manifest_" }} | |||
| Specify extra `Cargo.toml` manifests to workspaces which should also be | |||
| vendored and synced to the output. | |||
| vendored and synced to the output. Terminate the list with `--`. | |||
There was a problem hiding this comment.
I'm not sure this is really the best way to convey how command-line arguments work. Adding a bare -- prevents specifying other options, it doesn't just terminate the --sync arguments.
It's a bit unfortunate that --sync was defined with multiple_values defined, as this is a bit of a footgun. It would almost be nice to remove that, but I'm not sure if that is really feasible. @alexcrichton or @epage do you have any thoughts on that?
I wonder if cargo could maybe detect this situation and provide a better error message? That is, if one of the --sync arguments is not a Cargo.toml, it could give a suggestion?
Otherwise, maybe this could use a few more words to specify that "if you are specifying an output directory, then include a separate -- argument to separate …". Something along those lines, I'm not sure how to word it clearly.
There was a problem hiding this comment.
Yes, getting rid of multiple_values is definitely the right thing to do, but I didn't feel comfortable making a UI-incompatible change. Best I could think of was documenting it.
I didn't realize the terminator killed parsing entirely. Is that also true if we specify value_terminator in the declaration for the --sync option?
There was a problem hiding this comment.
Oh dear yeah it was never intended that --foo bar baz is considered for Cargo, that's definitely an antipattern we wouldn't ever actually opt-in to. I would hope that cargo vendor isn't so widely used we could just fix it and consider it a bugfix
There was a problem hiding this comment.
If you're OK with changing the UI, then I've opened #10448 to do that.
There was a problem hiding this comment.
I briefly reviewed all other usages of multiple_values and they all look OK to me (mostly argument lists, some spec lists, but nothing like cargo-vendor where it could eat a following significant argument).
|
Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance. |
|
Closing in favor of #10448. |
vendor: Don't allow multiple values for --sync The --sync argument to cargo vendor currently takes a list, which makes it easy for it to eat the final path argument: ```` cargo vendor --manifest-path foo/Cargo.toml -s bar/Cargo.toml ./test_vendor/ error: failed to read ./test_vendor/ Caused by: No such file or directory (os error 2) ```` Per discussion on #10441, this behavior is undesirable and hopefully used infrequently enough that we can change the UI for it. This patch will now only allow one value per --sync argument. I didn't regenerate other doc files as it's not clear to me how/when that should be done.
It's not obvious that the
--syncargument tocargo vendoris a list, which makes it easy for it to eat the final path argument:This patch adds tests showing this is expected behavior, and also documents the
--list terminator in the cargo-vendor manpage.I didn't regenerate other doc files as it's not clear to me how/when that should be done.