Skip to content
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.

Sign up for GitHub

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

Jump to bottom

vendor: Document how to terminate --sync list #10441

Closed
wants to merge 1 commit into from
Closed

vendor: Document how to terminate --sync list #10441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/doc/man/cargo-vendor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 `--`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're OK with changing the UI, then I've opened #10448 to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{{/option}}

{{#option "`--no-delete`" }}
Expand Down
65 changes: 65 additions & 0 deletions tests/testsuite/vendor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,71 @@ fn two_lockfiles() {
p.cargo("build").cwd("bar").run();
}

#[cargo_test]
fn test_sync_argument() {
let p = project()
.no_manifest()
.file(
"foo/Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"

[dependencies]
bitflags = "=0.7.0"
"#,
)
.file("foo/src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"

[dependencies]
bitflags = "=0.8.0"
"#,
)
.file("bar/src/lib.rs", "")
.file(
"baz/Cargo.toml",
r#"
[package]
name = "baz"
version = "0.1.0"

[dependencies]
bitflags = "=0.8.0"
"#,
)
.file("baz/src/lib.rs", "")
.build();

Package::new("bitflags", "0.7.0").publish();
Package::new("bitflags", "0.8.0").publish();

p.cargo("vendor --respect-source-config --manifest-path foo/Cargo.toml -s bar/Cargo.toml baz/Cargo.toml test_vendor")
.with_stderr("\
error: failed to read [..]

Caused by:
No such file or directory (os error 2)
",
)
.with_status(101)
.run();

p.cargo("vendor --respect-source-config --manifest-path foo/Cargo.toml -s bar/Cargo.toml baz/Cargo.toml -- test_vendor")
.run();

let lock = p.read_file("test_vendor/bitflags/Cargo.toml");
assert!(lock.contains("version = \"0.8.0\""));
let lock = p.read_file("test_vendor/bitflags-0.7.0/Cargo.toml");
assert!(lock.contains("version = \"0.7.0\""));
}

#[cargo_test]
fn delete_old_crates() {
let p = project()
Expand Down