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

Implement `cargo install` #2026

Merged
merged 2 commits into from Oct 19, 2015

Conversation

Projects
None yet
8 participants
@alexcrichton
Copy link
Member

alexcrichton commented Oct 6, 2015

This commit is an implementation of RFC 1200 which brings two new
subcommands: cargo install and cargo uninstall. Most of this is a straight
implementation of the RFC, but a few tweaks were made:

  • The -p or --package arguments are no longer needed as you just pass
    crate as a bare argument to the command, this means cargo install foo
    works and downloads from crates.io by default.
  • Some logic around selecting which crate in a multi-crate repo is installed has
    been tweaked slightly, but mostly in the realm of "let's do the thing that
    makes sense" rather than the literal "let's do what's in the RFC".
    Specifically, we don't pick a crate with examples if there are multiple crates
    with binaries (instead an error is generated saying there are multiple binary
    crates).
@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Oct 6, 2015

r? @wycats

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

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Oct 6, 2015

the installation root's `bin` folder. The installation root is determined, in
order of precedence, by `--root`, `$CARGO_INSTALL_ROOT`, the `install.root`
configuration key, and finally the home directory (which is either
`$CARGO_HOME` if set or `$HOME/.cargo` by default).

This comment has been minimized.

@eternaleye

eternaleye Oct 6, 2015

I'd like to note my continuing discomfort with inventing Yet Another Language-Specific Homedir Install Location, rather than using ${HOME}/.local/, which is intended for user-scoped installation.

This comment has been minimized.

@steveklabnik

steveklabnik Oct 6, 2015

Member

This isn't inventing a new one, as ~/.cargo is already being used.

This comment has been minimized.

@eternaleye

eternaleye Oct 6, 2015

That's why it's continuing discomfort 😛

}

fn read_crate_list(path: &Path) -> CargoResult<CrateListingV1> {
let metadata = path.join(".crates.toml");

This comment has been minimized.

@eternaleye

eternaleye Oct 6, 2015

If I'm reading this correctly, this has one metadata file per install destination, listing all crates installed there (with their inner metadata)?

If so, this is exactly the kind of design I mentioned as being actively hostile to system package managers, as crate installs now mutate a single shared state, rather than being separable by way of (say) installing individual drop-in files to a directory. I really dislike this, as it forms an ABI which (even if not public) will be a pain to change later - and if not changed, makes interop with system PMs needlessly difficult and fraught.

By having a central state, a naive implementation simply appends new installs to the state file. If that is done, then installing crates which are independent of each other is not commutative - this breaks PMs.

A less naive implementation sorts the state file - in this case, it is commutative, but the PM needs to be specially aware of the state file, and may even need to manually regenerate it from its own metadata. This is more work for distro packagers, and weakens guarantees (such as checksums) that the PM can provide.

A much friendlier implementation would have a central directory, and drop a metadata file in per-crate. With that, all files installed by a crate can be presumed immutable by the system PM, which is far, FAR friendlier.

(A package manager's dream would be for the installed files of every package to be non-colliding and immutable, in which case all of us packagers could go home and sleep. Rust, interestingly enough, comes closer to this than just about anything else with its config hashes on libraries and such.)

This comment has been minimized.

@alexcrichton

alexcrichton Oct 6, 2015

Author Member

Note that looking at this from a package manager perspective isn't necessarily this intention, this is intended to be purely Cargo-only metadata. It's not the intention for cargo install to install binaries into system directories (like /usr/local) by default. In that sense I expect any package manager to simply ignore or delete this file or not even use cargo install at all when preparing a package.

It was discussed having a flag to not generate this when a package is installed on the RFC as well.

This comment has been minimized.

@eternaleye

eternaleye Oct 7, 2015

My concerns lie along the following lines:
1.) cargo install is a general enough tool to tempt people to apply it to system-wide use (i.e., it supports arbitrary destinations via --root and such). People already do this with NPM, gem, etc, and it's really a painful issue.
2.) The metadata is sufficiently useful that there is an incentive for others to parse it
3.) The metadata as specified is unnecessarily hostile to system PMs (i.e., doing it this way doesn't seem to provide much in the way of benefit over ways that would be less hostile)
4.) Simply not installing it at all for the system PM is a potential hazard due to (2) (as well as Cargo itself if --root is passed)

Those make it such that once the problematic way is used, it's going to be very difficult to use a way that is not.

As for a (simple!) solution that's far less problematic, simply having each crate's metadata go in ${ROOT}/some_dir/${CRATE_HASH}.crate.toml would do the trick - it'd be a very limited amount of added complexity (iterate the directory, merge the parsed data) in exchange for something that won't need a redesign later.

(Personally, I'd suggest some_dir be lib/cargo, by analogy with pkgconfig.)

@florianjacob

This comment has been minimized.

Copy link

florianjacob commented Oct 6, 2015

I'd also love to see cargo following the xdg basedir specification and the systemd file hierarchy, which, as I just learned, even has a section specifically on where to place user packages like in this case.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 6, 2015

"let's do the thing that makes sense" rather than the literal "let's do what's in the RFC".

Can we amend the RFC afterwards, so there's consistency?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 6, 2015

@florianjacob that's covered by #1734

@florianjacob

This comment has been minimized.

Copy link

florianjacob commented Oct 6, 2015

@steveklabnik #1734 only covers the xdg basedir specification regarding where to put cargo's regular config and data files. But cargo install makes the systemd file hierarchy standard relevant, too, as it specifies how user packages should be put in ~/.local/bin, ~/.local/lib/<arch-id>, ~/.local/lib/<package>/ etc.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 6, 2015

@florianjacob regardless, it's off-topic here, as we have an RFC with this design already agreed upon. I'm not saying such a change isn't worth it, but it should be a new issue with its own discussion and everything else.

Though I guess @alexcrichton is already deviating from the RFC... :/

@alexcrichton alexcrichton force-pushed the alexcrichton:cargo-install branch 6 times, most recently from 53adb87 to 7abffd8 Oct 7, 2015

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Oct 16, 2015

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 16, 2015

📌 Commit 7abffd8 has been approved by brson

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 16, 2015

⌛️ Testing commit 7abffd8 with merge d090d38...

bors added a commit that referenced this pull request Oct 16, 2015

Auto merge of #2026 - alexcrichton:cargo-install, r=brson
This commit is an implementation of [RFC 1200][rfc] which brings two new
subcommands: `cargo install` and `cargo uninstall`. Most of this is a straight
implementation of the RFC, but a few tweaks were made:

* The `-p` or `--package` arguments are no longer needed as you just pass
  `crate` as a bare argument to the command, this means `cargo install foo`
  works and downloads from crates.io by default.
* Some logic around selecting which crate in a multi-crate repo is installed has
  been tweaked slightly, but mostly in the realm of "let's do the thing that
  makes sense" rather than the literal "let's do what's in the RFC".
  Specifically, we don't pick a crate with examples if there are multiple crates
  with binaries (instead an error is generated saying there are multiple binary
  crates).

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1200-cargo-install.md
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 16, 2015

💔 Test failed - cargo-mac-64

steveklabnik and others added some commits Feb 22, 2015

Finish implementing `cargo install`
This commit is an implementation of [RFC 1200][rfc] which brings two new
subcommands: `cargo install` and `cargo uninstall`. Most of this is a straight
implementation of the RFC, but a few tweaks were made:

* The `-p` or `--package` arguments are no longer needed as you just pass
  `crate` as a bare argument to the command, this means `cargo install foo`
  works and downloads from crates.io by default.
* Some logic around selecting which crate in a multi-crate repo is installed has
  been tweaked slightly, but mostly in the realm of "let's do the thing that
  makes sense" rather than the literal "let's do what's in the RFC".
  Specifically, we don't pick a crate with examples if there are multiple crates
  with binaries (instead an error is generated saying there are multiple binary
  crates).

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1200-cargo-install.md

@alexcrichton alexcrichton force-pushed the alexcrichton:cargo-install branch from 7abffd8 to bc60f64 Oct 19, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Oct 19, 2015

@bors: r=brson

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 19, 2015

📌 Commit bc60f64 has been approved by brson

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 19, 2015

⌛️ Testing commit bc60f64 with merge 1206e5e...

bors added a commit that referenced this pull request Oct 19, 2015

Auto merge of #2026 - alexcrichton:cargo-install, r=brson
This commit is an implementation of [RFC 1200][rfc] which brings two new
subcommands: `cargo install` and `cargo uninstall`. Most of this is a straight
implementation of the RFC, but a few tweaks were made:

* The `-p` or `--package` arguments are no longer needed as you just pass
  `crate` as a bare argument to the command, this means `cargo install foo`
  works and downloads from crates.io by default.
* Some logic around selecting which crate in a multi-crate repo is installed has
  been tweaked slightly, but mostly in the realm of "let's do the thing that
  makes sense" rather than the literal "let's do what's in the RFC".
  Specifically, we don't pick a crate with examples if there are multiple crates
  with binaries (instead an error is generated saying there are multiple binary
  crates).

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1200-cargo-install.md
@bors

This comment has been minimized.

@bors bors merged commit bc60f64 into rust-lang:master Oct 19, 2015

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:cargo-install branch Oct 19, 2015

carols10cents added a commit to carols10cents/rust.tmbundle that referenced this pull request Oct 24, 2015

Set some likely default locations for the racer binary
This way, people don't necessarily have to set `TM_RACER`. Add in the
locations that [cargo
install](rust-lang/cargo#2026) will be using, on the assumption that
racer will likely start recommending cargo install soon.

@carols10cents carols10cents referenced this pull request Oct 24, 2015

Closed

add Complete command #15

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.