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

Proposal: cargo install with upgrades #6667

Closed
ehuss opened this issue Feb 14, 2019 · 11 comments · Fixed by #6798
Closed

Proposal: cargo install with upgrades #6667

ehuss opened this issue Feb 14, 2019 · 11 comments · Fixed by #6798
Labels
C-tracking-issue Category: A tracking issue for something unstable. Command-install

Comments

@ehuss
Copy link
Contributor

ehuss commented Feb 14, 2019

This is a proposal to change the behavior of cargo install as discussed at #6595 (comment):

cargo install will be changed to reinstall an already installed package if it is not "up to date" instead of failing.

The determination of "up to date" is defined as follows:

  • If --version is specified with an exact version (like 1.2.3), the installed version must be exactly the given version.
  • If --version is specified with a version requirement (like ~1.2.3), it checks the registry for the greatest version matching the requirement (not including pre-release versions), and ensures the installed version is exactly that version.
  • If --git is specified, the installed git hash must match exactly.
  • If --path is specified, it is always reinstalled.
  • If none of the flags above are given (such as cargo install foo), then it is treated as --version=*.
  • The package will be reinstalled if the settings are not the same. The settings includes:
    • Enabled features
    • --debug
    • The set of binaries is different (such as using different target selectors --bin, --bins, --example, --examples)
    • --target
    • The source

Additional clarifications:

  • Fundamentally, the only change in behavior is when cargo install currently fails.
  • Installation will still fail if installation would create a binary of the same name that already exists in another package (of a different name).
  • If the package is up to date, the command exits with success (0).
  • --force continues to force an install, even if it appears up to date.
  • A report at the end should clearly indicate which packages were upgraded and which were skipped.
  • Updates in dependencies do not trigger an update.
  • This will be implemented behind an unstable flag.

.crates.toml Changes

More metadata will need to be stored. Currently it only tracks a mapping of PackageId to the set of binaries it installs. I see two ways to approach migrating to a new format:
1. Switch .crates.toml to V2. This will cause older versions of Cargo to fail to install.
2. Use a new filename (.crates2.json). This will allow older and newer Cargos to coexist.
I lean towards the second option, since it would cause less friction, with minimal drawbacks. Additionally, the new file could be designed with forwards compatibility in mind. Specifically, unknown fields for untouched packages should be preserved.

I lean towards changing the format to json. This file is not intended to be user-edited, and toml can be finicky sometimes.

Here's a commented description of the new file format:

{
    "packages": {
        "pkg_name": {
            // Version number of the package.
            "version": "1.0.0",
            // The version requested on the command-line with `--version`.
            // Not specified if `--version` was not used.
            "version_req": "1.0",
            // The source URL.
            "source": "source_url",
            // The list of binary names installed.
            "bins": ["foo", "foo2"],
            // List of features enabled.
            "features": ["default", "featname"],
            // Either "debug" or "release".
            "profile": "release",
            // The installation target.
            // Either the host or the value specified in `--target`.
            "target": "x86_64-apple-darwin",
            // Output of `rustc -V`.
            "rustc": "rustc 1.32.0 (9fda7c223 2019-01-16)",
        }
    }
}

It should continue to keep .crates.toml up-to-date with its original format.

References

Update all

I think it might be desirable to have the ability to update all outdated packages. I propose this should be decided upon separately, and should be implemented separately afterwards if it is desired. I think it is desirable to avoid too much complication in one command, so care should be taken. If it can be done with a single flag, that might be acceptable.

One possibility is to have an --outdated flag to perform this task. By default, it should update all outdated packages installed from a registry. There are various details to be decided on:

  • Should there be a confirmation before starting?
  • Should there be a way to see which packages will be updated (--dry-run? --outdated --list?)?
  • Should it support git packages? Perhaps --outdated could take some optional arguments, such as --outdated=git.
  • Should there be a way to upgrade based on toolchain? Perhaps --outdated=toolchain?
  • This command could have bad interaction with packages installed with features or other settings. Should those settings be sticky (and only version is updated?)? What if a package was installed with an explicit --version?

Questions

  • How should a change in rustc toolchain be handled? My preference is that it should be ignored. For the majority of cases, it doesn't have a significant impact.
  • Should there be an option to disable upgrades (à la apt-get --no-upgrade). My preference is "no" (just to reduce the number of flags), but I am not opposed. There is a workaround of which foo || cargo install foo.
  • Should other environment/config items that affect compilation be tracked? I lean towards "no". Possible items:
    • RUSTC, RUSTC_WRAPPER, RUSTFLAGS, CARGO_INCREMENTAL
    • [target] settings (rustflags, linker, ar)
    • [build] settings (rustc, rustflags, incremental)
    • Config-profiles
@alexcrichton
Copy link
Member

Thanks for opening an issue on this @ehuss! This all sounds fantastic to me, and I'm definitely in favor of this proposal.

For some of the questions you asked...:

How should a change in rustc toolchain be handled?

I think this can be bundled with a lot of other parts of this, such as what to do about the environment/compilation flags, linker settings, dependency versions, etc. This all seems like it's developing (what I think at least is) a reasonable policy of "an upgrade is considered as the original source package's code possibly changing or major codegen settings".

It only considers the package-being-installed's code which is affected by version number, features, git revs, etc. I think it's reasonable to by default ignore dependency updates because otherwise this command isn't too useful as you'll basically always be reinstalling.

Major codegen changes like --debug or not, the target, etc, also seem reasonable to automatically recompile on as they can also affect things like #[cfg] and are otherwise well-established "big decision".

For everything else I think it's reasonable to relegate it to --force. If you updated rustc binaries previously installed are unlikely to radically change. Similarly tweaks in RUSTFLAGS, incremental, linkers, etc, all in general have pretty small impacts on build settings.

Put another way I feel like ignoring many of these options strikes a good balance between "still a useful command" and "doesn't recompile everything all the time forever". The cargo install mechanism, after all, is largely intended as a convenience for Rust developers rather than a "single source of truth" like cargo build is, for example.

Should there be an option to disable upgrades (à la apt-get --no-upgrade).

Sounds like a good future extension to me :)

@joshtriplett
Copy link
Member

joshtriplett commented Feb 27, 2019

Right now, I expect cargo install to work like make install: always build and install, don't care what's in the target directory. (I would ideally like to stop installing .crates.toml entirely, but that's a separate issue.) I certainly don't think that cargo install should fail here; I think cargo install should build and install.

There needs to be a command that forces build and install; ideally, that command will also skip installing .crates.toml entirely. I would prefer that that command be spelled cargo install, but regardless, I'd like to have that command, and since that's the command I'm going to run any time I want to build and install a Rust crate, I'd like to have that command be simple and short.

@joshtriplett
Copy link
Member

joshtriplett commented Feb 27, 2019

We discussed this in the Cargo meeting, and @ehuss clarified several things. Based on those clarifications:

  • Does this rebuild if you change the cargo or rustc version? I do want to be able to cargo +nightly install foo and have that rebuild and reinstall, even if I already have that version of foo installed.

  • Assuming that the above is true, the current proposal should work for me, and I don't have any further objection given that it doesn't change behavior if you don't have a .crates.toml or if you pass --path.

  • Could I please have an option that prevents cargo install from installing a .crates.toml file at all, to avoid having to remove it after installation?

@dwijnand
Copy link
Member

I think the name of the command is wrong. cargo install should install, and if it's already present it should error out with appropriate messaging. My comparison is brew install:

$ brew install tokei
Error: tokei 8.0.1 is already installed
To upgrade to 9.0.0, run `brew upgrade tokei`

If a user wants to upgrade, then maybe we should introduce cargo upgrade, with those "up to date" determining semantics. (And possibly cargo reinstall which is uninstall and then install latest)

  • If --version is specified, the installed version must be exactly the given version.

I liked https://crates.io/crates/cargo-ensure-installed's behaviour of "a suitable version", where (IIUC) the version argument is as a semver version requirement spec. Should we do the same?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 28, 2019

Does this rebuild if you change the cargo or rustc version?

No, I explicitly excluded rustc version changes. My thinking is that there are many environmental factors that are not tracked, that generally have little-to-no impact on the behavior of the installed binary (and rustc is one of those). That said, I did include rustc version in the metadata in case we want to use it in the future, or is in some way useful. Generally, --force is the intended way to deal with issues like these.

version argument is as a semver version requirement spec. Should we do the same?

That is how it works today, and this wouldn't change that.

cargo install should install, and if it's already present it should error out

I'm not a fan of that behavior, because I cannot think of use cases where that is desired behavior, and several where upgrading is beneficial. If one wants to avoid an upgrade, one can test for the existence of the binary first (or lock the version with --version). As I mentioned, there is an option to add --no-upgrade, but I don't see it as being needed. I also think additional commands (like cargo upgrade) would be confusing (I can never remember update vs upgrade) and more difficult to use. Can you say more about why you would want it to fail?

I did a small survey of other tools to see what their behavior is.
Tools that install or upgrade:

  • rustup
  • apt
  • gem
  • nix
  • macports
  • Some tools are more like cargo update which add or update if needed, ignore if already up-to-date:
    • cargo update
    • npm
    • bundler
    • dep
    • nuget

Tools that fail if already installed:

  • homebrew
  • snap

@dwijnand
Copy link
Member

cargo install should install, and if it's already present it should error out

I'm not a fan of that behavior, because I cannot think of use cases where that is desired behavior, and several where upgrading is beneficial. If one wants to avoid an upgrade, one can test for the existence of the binary first (or lock the version with --version). As I mentioned, there is an option to add --no-upgrade, but I don't see it as being needed. I also think additional commands (like cargo upgrade) would be confusing (I can never remember update vs upgrade) and more difficult to use. Can you say more about why you would want it to fail?

If by "use cases" you mean scripting or CI setup use cases, I agree I can't see any for this desired behaviour. But I think it's the right behaviour when a human is interacting with the tool, simply because it matches my expectations of "install". I see cargo install foo as a statement of "I want foo to be installed" and if foo, even a non-latest version, is installed, it's already still "installed" and so no installation, reinstallation or upgrading should happen.

So I guess I care more about it not upgrading then the exit code: I'd be ok with it warning and then exiting with code 0.

Also, in the case of local sources, i.e. cargo install --path ., I'm happy with the reinstall behaviour because I feel with modifiable sources readily available it makes sense (to me).

But I'm also not 100% happy the idea of cargo upgrade because it's similar but completely different to cargo update. But that, in my opinion, is because I think it's wrong for cargo to try and be both a build tool and a package manager (and for crates.io to be both a repository of library crates and distributable binaries - see today's #6702 for another symptom of that).

@joshtriplett
Copy link
Member

joshtriplett commented Feb 28, 2019 via email

@ehuss
Copy link
Contributor Author

ehuss commented Mar 7, 2019

cargo +nightly install foo doesn't seem like an unreasonable use case.

The issue there is that if you follow nightly, it would essentially disable the caching since it would appear to change every day. I agree that on stable it would be more reasonable. I just wanted to lean towards a more conservative approach, since you can work around that by using --force, whereas if it checked the rustc version, there would be no workaround if you don't care about the rustc version.

bors added a commit that referenced this issue Apr 5, 2019
Add install-upgrade.

This implements the feature described in #6667. Instead of failing when `cargo install` detects a package is already installed, it will upgrade if the versions don't match, or do nothing (exit 0) if it is considered "up-to-date".

Closes #6667.

Notes:
- This feature rejects ambiguous `--version` input (such as `1.0`). This is not required, but seemed like a reasonable time to make the change.
- There is a slight change to the output on stable which reports what was installed/replaced.
- Added better support for `*` in `--version` (don't show warning).
@bors bors closed this as completed in #6798 Apr 5, 2019
@alexcrichton
Copy link
Member

Implemented in #6798, let's leave this open as a tracking issue

@alexcrichton alexcrichton reopened this Apr 5, 2019
@alexcrichton alexcrichton added the C-tracking-issue Category: A tracking issue for something unstable. label Apr 5, 2019
@ehuss
Copy link
Contributor Author

ehuss commented Apr 5, 2019

Oh, I opened a separate tracking issue (#6797) to keep the post-implementation discussion focused there.

@alexcrichton
Copy link
Member

Oops, just kidding then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable. Command-install
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants