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.

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

Import the cargo-vendor subcommand into Cargo #6869

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@alexcrichton
Copy link
Member

commented Apr 23, 2019

This commit imports the external alexcrichton/cargo-vendor
repository
into Cargo itself. This means it will no longer be
necessary to install the cargo-vendor subcommand in order to vendor
dependencies. Additionally it'll always support the latest feature set
of Cargo as it'll be built into Cargo!

All tests were imported as part of this commit, but not all features
were imported. Some flags have been left out that were added later in
the lifetime of cargo vendor which seem like they're more questionable
to stabilize. I'm hoping that they can have separate PRs adding their
implementation here, and we can make a decision of their stabilization
at a later date.

The current man page for cargo vendor -h will look like:

cargo-vendor
Vendor all dependencies for a project locally

USAGE:
cargo vendor [OPTIONS] [--] [path]

OPTIONS:
-q, --quiet                    No output printed to stdout
    --manifest-path <PATH>     Path to Cargo.toml
    --no-delete                Don't delete older crates in the vendor directory
-s, --sync <TOML>...           Additional `Cargo.toml` to sync and vendor
    --respect-source-config    Respect `[source]` config in `.cargo/config`
-v, --verbose                  Use verbose output (-vv very verbose/build.rs output)
    --color <WHEN>             Coloring: auto, always, never
    --frozen                   Require Cargo.lock and cache are up to date
    --locked                   Require Cargo.lock is up to date
-Z <FLAG>...                   Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
-h, --help                     Prints help information

ARGS:
<path>    Where to vendor crates (`vendor` by default)

This cargo subcommand will vendor all crates.io and git dependencies for a
project into the specified directory at `<path>`. After this command completes
the vendor directory specified by `<path>` will contain all remote sources from
dependencies specified. Additionally manifest beyond the default one can be
specified with the `-s` option.

The `cargo vendor` command will also print out the configuration necessary
to use the vendored sources, which when needed is then encoded into
`.cargo/config`.

Since this change is not importing 100% of the functionality of the
existing cargo vendor this change does run a risk of being a breaking
change for any folks using such functionality. Executing cargo vendor
will favor the built-in command rather than an external subcommand,
causing unimplemented features to become errors about flag usage.

@rust-highfive

This comment has been minimized.

Copy link

commented Apr 23, 2019

r? @nrc

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

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Note that I suspect we can't land this necessarily as-is, but I wanted to get the conversation started!

@ehuss could you help me out a bit with src/doc/man? Are the adoc files edited by hand and then everything else is generated?

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@ehuss could you help me out a bit with src/doc/man? Are the adoc files edited by hand and then everything else is generated?

Yes, the adoc files are written by hand. You need to install asciidoctor and run make in src/doc. I can write it and push to your branch if you want.

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch from f556435 to 956aa52 Apr 26, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Now with documentation!

@ehuss
Copy link
Contributor

left a comment

To add the man page to the website, it needs to be added to src/doc/src/SUMMARY.md. I'm not sure which category to put it in, maybe "manifest commands" or create a new category? Then add a markdown file that includes the html (see src/doc/src/commands/cargo-build.md as an example).

(I have to go, not sure what is wrong with ignore_files extracting a file, will take a look when I return.)

Show resolved Hide resolved src/bin/cargo/commands/vendor.rs Outdated
Show resolved Hide resolved src/bin/cargo/commands/vendor.rs Outdated
Show resolved Hide resolved src/cargo/ops/vendor.rs Outdated
Show resolved Hide resolved src/cargo/ops/vendor.rs Outdated
Show resolved Hide resolved tests/testsuite/vendor.rs Outdated

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch 2 times, most recently from eba9395 to 8fff7db Apr 29, 2019

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Looks great! Is this ready to go? If so, r=me. It wasn't clear to me if there was more you wanted to do now.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

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

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch from 8fff7db to fc4d5ea Apr 29, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Do you have thoughts on the breakage relative to cargo-vendor itself? I'm personally ok landing this and just saying "use cargo-vendor vendor ... if you're using the options, but we could also possibly go ahead and implement them in Cargo behind -Z or something like that.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Just to be clear, is this the set of flags we are talking about?

-x, --explicit-version   Always include version in subdir name
--disallow-duplicates    Disallow two versions of one crate
--only-git-deps          Only vendor git dependencies, not crates.io dependencies
--relative-path          Use relative vendor path for .cargo/config
--no-merge-sources       Keep sources separate

I have no sense of how many people are using these flags, or how you decided which to leave out. Googling around, I don't see much. Like it looks like --explicit-version was once used by fuchsia, but it doesn't look like they use it anymore?

Absent any more information, I would lean towards what you said (use cargo-vendor as a fallback), and hopefully if someone is relying on one of these flags, they can file an issue. Otherwise, we could assume they are not needed anymore.

Perhaps we could just add the flags here with the hidden attribute, and check if they are used and raise an error informing the user why the flag is not supported anymore, and maybe ask them to file an issue here?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Yeah it's just those flags, and I also don't have a great sense of how often they're used. I like the idea of hidden help flags that redirect users to file issues here, so I'll implement that.

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch from fc4d5ea to 711fddb Apr 30, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Alright, that makes me comfortable with this PR, so...

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nrc

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Do you have a sense of who and how many people use cargo vendor in its current state? What is the motivation for moving it into Cargo rather than maintaining it as an 'official' plugin?

@nrc

This comment has been minimized.

Copy link
Member

commented May 1, 2019

@rfcbot concern why better to be built in?

(see above comment)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

A good question! We talked about this briefly in the last cargo mtg but I'm realizing now I never actually wrote down the rationale, so now's a good a time as any!

I get the impression that usage of cargo vendor is quite common and is the defacto way to vendor dependencies (as opposed to cargo-local-registry). This is largely from the number of feature requests I've gotten on cargo-vendor over time and the support in maintaining it. Given that it's such an important part of our offline story as well it seemed like it was time to move it into Cargo proper to say that we support first-class vendoring rather than having to go through an external subcommand.

It's certainly possible to keep it as an external crate, but it comes with all the drawbacks of "oh now you have to cargo install it". To me it's just crossed the threshold of "useful enough to be bundled directly" sort of like cargo fix

@bors

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

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

@LegNeato

This comment has been minimized.

Copy link

commented May 7, 2019

cargo-raze relies on -x when vendoring in: https://github.com/google/cargo-raze/blob/master/README.md

@cuviper

This comment has been minimized.

Copy link
Member

commented May 7, 2019

FWIW, I also found cargo-vendor useful and important enough to include in Red Hat's rust-toolset offering -- so far it's the only thing we add beyond the rust "extended tools" group. I would be happy to drop the extra package. :)

@vi

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

"oh now you have to cargo install it"

Can it be a rustup component? Or is rustup also met with some resistance?

@cramertj

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I'll add another voice on behalf of this being a cargo built-in. Currently, Fuchsia builds all of its Rust code (both tools and programs, third party and first party) using vendored dependencies. All of them, that is, except for cargo vendor, whose dependencies can't be easily vendored for somewhat obvious reasons :)

We also build our own toolchain using ./x.py install, so adding it as a rustup component wouldn't necessarily be helpful unless it was also included in the set of things built as part of ./x.py.

@AndrewGaspar

This comment has been minimized.

Copy link

commented May 7, 2019

This is also useful for my domain (lots of airgapped machines).

@repi

This comment has been minimized.

Copy link

commented May 7, 2019

I added these flags before for cargo vendor for the vendoring workflow that we were using back at SEED/EA:

--only-git-deps          Only vendor git dependencies, not crates.io dependencies
--relative-path          Use relative vendor path for .cargo/config

But it was a rather specific workflow focused on vendoring only our own private git dependencies rather than crates from crates.io so don't think it is as generally useful to be included in core Cargo.

For our new stuff at Embark we don't use that workflow, and if we would vendor the default flags (maybe with the addition of -x, which is quite nice) would be enough.

With this integrated into Cargo, I have a slight concern that it would be slower and less easy for people to contribute to it and iterate on it in the main Cargo repo vs the original small original separate repo.

That said, I do think having cargo vendor built in would be good, and would help to build a standard story around its workflow and making it more accessible and easier to use.

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch from 711fddb to cf1dee8 May 16, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Ok it's taken me quite some time to get back to this, but a week or so ago we talked about this in the Cargo team. @nrc felt that we may not necessarily want to do this because we should be pushing for the story of cargo subcommands on crates.io to be as good as possible. This is, however, an example of going in the opposite direction by trying to include things in Cargo!

We talked about a lot of different aspects, but in the end we largely concluded that this is fine to land for now. One distinction we were able to draw is subcommands that link to Cargo and those which defer to the cargo executable (aka cargo metadata). Linking to Cargo will basically always provide a subpar experience being distributed from crates.io, and it seems like we'll want vendoring to integrate into Cargo in an intrusive fashion long-term which requires linking to Cargo.

This also provides opportunities to officially support other commands such as cargo update on a vendored project (where that actually talks to the network and revendors and such).

All in all the conclusion was that this is probably ok to land for now, so we were interested in doing so.


I've rebased this PR but left it at its previous state (flags like -x still generate an error and recommend crates.io). Thinking more about this though I've realized a few things we may want to tweak:

  • With the advent of custom registries, should we delete [source] configuration by default? The purpose here was to support cargo vendor when the vendor dir was already configured, but deleting [source] configuration may not be the best way to do that.
  • Should we remove support for the -s flag and --no-delete flags? If we want to support automatic vendoring in the future (aka cargo update transparently updates the vendor folder if vendoring is enabled) then we'd have to provide configuration options for these via some mechanism. Otherwise without vendor options to configure we could say that if vendoring is configured and you run cargo update it "equivalently runs cargo vendor" just afterwards.
@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@nrc, do you also want to resolve your rfcbot concern here?

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

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

Import the cargo-vendor subcommand into Cargo
This commit imports the external [alexcrichton/cargo-vendor
repository][repo] into Cargo itself. This means it will no longer be
necessary to install the `cargo-vendor` subcommand in order to vendor
dependencies. Additionally it'll always support the latest feature set
of Cargo as it'll be built into Cargo!

All tests were imported as part of this commit, but not all features
were imported. Some flags have been left out that were added later in
the lifetime of `cargo vendor` which seem like they're more questionable
to stabilize. I'm hoping that they can have separate PRs adding their
implementation here, and we can make a decision of their stabilization
at a later date.

The current man page for `cargo vendor -h` will look like:

    cargo-vendor
    Vendor all dependencies for a project locally

    USAGE:
	cargo vendor [OPTIONS] [--] [path]

    OPTIONS:
	-q, --quiet                    No output printed to stdout
	    --manifest-path <PATH>     Path to Cargo.toml
	    --no-delete                Don't delete older crates in the vendor directory
	-s, --sync <TOML>...           Additional `Cargo.toml` to sync and vendor
	    --respect-source-config    Respect `[source]` config in `.cargo/config`
	-v, --verbose                  Use verbose output (-vv very verbose/build.rs output)
	    --color <WHEN>             Coloring: auto, always, never
	    --frozen                   Require Cargo.lock and cache are up to date
	    --locked                   Require Cargo.lock is up to date
	-Z <FLAG>...                   Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for details
	-h, --help                     Prints help information

    ARGS:
	<path>    Where to vendor crates (`vendor` by default)

    This cargo subcommand will vendor all crates.io and git dependencies for a
    project into the specified directory at `<path>`. After this command completes
    the vendor directory specified by `<path>` will contain all remote sources from
    dependencies specified. Additionally manifest beyond the default one can be
    specified with the `-s` option.

    The `cargo vendor` command will also print out the configuration necessary
    to use the vendored sources, which when needed is then encoded into
    `.cargo/config`.

Since this change is not importing 100% of the functionality of the
existing `cargo vendor` this change does run a risk of being a breaking
change for any folks using such functionality. Executing `cargo vendor`
will favor the built-in command rather than an external subcommand,
causing unimplemented features to become errors about flag usage.

[repo]: https://github.com/alexcrichton/cargo-vendor

@alexcrichton alexcrichton force-pushed the alexcrichton:vendor branch from cf1dee8 to 3842d8e May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.