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

Tracking issue for install-upgrade #6797

Closed
ehuss opened this issue Mar 31, 2019 · 14 comments · Fixed by #7560
Closed

Tracking issue for install-upgrade #6797

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

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 31, 2019

Original issue: #6667
Implementation PR: #6798
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#install-upgrade

Summary
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".

cargo +nightly install foo -Z install-upgrade

Unresolved questions

  • Is it tracking the correct information? There are many other settings that could be tracked (rustc, env variables, changes in dependencies in Cargo.lock, mtime for path sources, etc). The current set was chosen to be practical and simple and should cover most use cases. Otherwise --force is intended as a workaround for more advanced requirements.
  • Should there be a way to upgrade all outdated packages? (See Proposal: cargo install with upgrades #6667 for more about this.)
  • Should --no-track be kept, or is there a better approach for packaging? Does it have the right behavior? See also Please provide a cargo install option to not write .crates.toml #3316.
  • Should this be the new default? Should there be an option to disable upgrades and revert to the old behavior?
  • If a new version of a package drops a binary, should those dropped binaries be uninstalled when the new package is installed? Currently it only replaces binaries. Fixed in cargo install: Remove orphaned executables. #7246
@gnzlbg
Copy link
Contributor

gnzlbg commented May 28, 2019

Should there be a way to upgrade all outdated packages? (See #6667 for more about this.)

That's probably a different feature right? Or why would we need to resolve this as part of this one?

If a new version of a package drops a binary, should those dropped binaries be uninstalled when the new package is installed? Currently it only replaces binaries.

I think this should behave like uninstalling the package, and then installing the newer one, so that would mean that yes, this should do that. Having said this, printing a warning: ... when this happens wouldn't hurt (as long as there is a quiet mode available).

@ehuss
Copy link
Contributor Author

ehuss commented May 28, 2019

That's probably a different feature right? Or why would we need to resolve this as part of this one?

Yea, in my original proposal I indicated it should probably be separate. I just included it here for completeness in case people felt it was important.

I think this should behave like uninstalling the package

That sounds good to me.

@rye
Copy link

rye commented Jul 13, 2019

My perspective here is mostly that of an end-user, but having just read through the implementation PR I do have some opinions on these questions:

  • Is it tracking the correct information?

As you note, the current set (at least what is listed there) seems to me to be a pretty good set of distinguishers between versions. I was personally only thinking about versions as being criteria—this set also covers more information. It might be nice to update all installations that match the given criteria. For example, if I've installed for target A and target B of crate foo, I'd expect cargo install foo to see both targets updated, or tell the user that it wants a specific install to operate on.

  • Should there be a way to upgrade all outdated packages?

I think so, but I don't see a way to make this fit with the verbiage cargo install. I think a separate subcommand (cargo upgrade/cargo update, for example) that checks installed packages for updates and makes them happen would be best. Since the information you store has a big installs key, I think this should be somewhat straightforward.

  • Should --no-track be kept, or is there a better approach for packaging? Does it have the right behavior?

Based on the description given, I think it's just about right. For packaging, I personally am of the mind that --no-track is a good option to prevent pollution. The documentation or name could be changed; --no-track indicates to me instead that the installation will not be tracked by some analytics engine. Perhaps --no-store-installs-metadata or something more concise?

In building Docker images with Crates, I think having --no-track is a sane option; though I think the tendency is to just ignore the ~/.cargo directory entirely. So it might not be necessary, but could definitely be of use.

  • Should this be the new default? Should there be an option to disable upgrades and revert to the old behavior?

For the cargo install subcommand, I absolutely think so. This will require some education through documentation changes, but I think many users that come from other languages like JS/Ruby prefer this ergonomic over a flag that they have to pass to explicitly enable the behavior. This would also fall under the same token as that which make install users would be used to; generally install targets that I have seen in the wild will overwrite existing artifacts with no concern.

I could definitely see a cargo install --install-only or cargo install --no-upgrade that dies if something is already installed. But then again, I also think cargo install could just be assumed to install the latest version, as I don't think many users who are installing things globally via cargo install would care if a second iteration overwrites.

  • If a new version of a package drops a binary, should those dropped binaries be uninstalled when the new package is installed? Currently it only replaces binaries.

I think replacement of remaining binaries is fine, with a warning added indicating that binaries were dropped. Though, since doing so would require comparison of the former and latter states anyhow, it might just be better to uninstall dangling binaries and warn the user that you did that. This could also be toggleable behavior. (Proposal: --keep-dangling.)


There is one additional item—I would like the final version of this feature to just reuse $CARGO_HOME/.crates.toml, but with a [v2] key, if possible. I wasn't sure if this was part of the plan, but I think it'd be the cleanest end-result.

@elichai
Copy link

elichai commented Aug 9, 2019

I would've preferred this as a flag like: cargo install --upgrade but this is still good, any plans for stabilization?

bors added a commit that referenced this issue Aug 19, 2019
`cargo install`: Remove orphaned executables.

When a new version of a package is installed that no longer contains an executable from a previous version, this change causes those orphaned executables to also be removed.

I can place this new behavior behind the `install-upgrade` feature gate if anyone is uncomfortable with changing the behavior now.

cc #6797
@kathampy
Copy link

Will this also compile the crate with the nightly toolchain? Is there a way to use the install-upgrade command from nightly but compile the actual crate with stable?

@ehuss
Copy link
Contributor Author

ehuss commented Aug 27, 2019

@kathampy This feature is nightly-only until it is stabilized.

@shepmaster
Copy link
Member

  • Should there be a way to upgrade all outdated packages?

I do think it is important to have. ❤️

bors bot added a commit to nervosnetwork/ckb that referenced this issue Oct 10, 2019
1664: chore: update get_block_template rpc doc r=quake,u2,keroro520,yangby-cryptape a=shaojunda

replace difficulty with compact_target

1669: chore(test): Allow CI debug logs r=u2,zhangsoledad a=keroro520

The number of logs is acceptable after #1474. 

1671: test: disconnect check by all part r=u2,keroro520,quake a=driftluo

Disconnect check by all part

1673: docs: Update PoW mining algorithm description r=u2,quake a=ashchan

Now that this has been decided the doc should reflect that before the next round of mining test.

1680: fix: get_block_transactions_process r=u2,quake a=zhangsoledad



1689: chore: issuance comment r=u2,quake a=zhangsoledad

comments were lost on rebase

1690: ci: Make sure cargo-audit up-to-date r=quake,u2 a=zhangsoledad

rustsec/rustsec#143
https://internals.rust-lang.org/t/idea-cargo-install-update/11072
rust-lang/cargo#6797



1691: chore: speed up maturity test r=u2,quake a=zhangsoledad



Co-authored-by: shaojunda <shaojunda@gmail.com>
Co-authored-by: keroro520 <keroroxx520@gmail.com>
Co-authored-by: driftluo <driftluo@foxmail.com>
Co-authored-by: James Chen <james@ashchan.com>
Co-authored-by: zhangsoledad <787953403@qq.com>
bors bot added a commit to nervosnetwork/ckb that referenced this issue Oct 10, 2019
1680: fix: get_block_transactions_process r=u2,quake a=zhangsoledad



1689: chore: issuance comment r=u2,quake a=zhangsoledad

comments were lost on rebase

1690: ci: Make sure cargo-audit up-to-date r=quake,u2 a=zhangsoledad

rustsec/rustsec#143
https://internals.rust-lang.org/t/idea-cargo-install-update/11072
rust-lang/cargo#6797



1691: chore: speed up maturity test r=u2,quake a=zhangsoledad



1693: test: simple header field json format check r=quake,u2 a=zhangsoledad



1696: fix: set `next_epoch_diff` to one instead of panic when it is zero r=zhangsoledad,u2 a=doitian



Co-authored-by: zhangsoledad <787953403@qq.com>
Co-authored-by: ian <ian@nervos.org>
bors bot added a commit to nervosnetwork/ckb that referenced this issue Oct 10, 2019
1680: fix: get_block_transactions_process r=u2,quake a=zhangsoledad



1689: chore: issuance comment r=u2,quake a=zhangsoledad

comments were lost on rebase

1690: ci: Make sure cargo-audit up-to-date r=quake,u2 a=zhangsoledad

rustsec/rustsec#143
https://internals.rust-lang.org/t/idea-cargo-install-update/11072
rust-lang/cargo#6797



1691: chore: speed up maturity test r=u2,quake a=zhangsoledad



1693: test: simple header field json format check r=quake,u2 a=zhangsoledad



1696: fix: set `next_epoch_diff` to one instead of panic when it is zero r=zhangsoledad,u2 a=doitian



Co-authored-by: zhangsoledad <787953403@qq.com>
Co-authored-by: ian <ian@nervos.org>
@tesuji
Copy link
Contributor

tesuji commented Oct 28, 2019

I think we could adapt what pip do here about the command line flag.

  -U, --upgrade               Upgrade all specified packages to the newest available version. The handling of dependencies depends on the upgrade-strategy used.
  --upgrade-strategy <upgrade_strategy>
                              Determines how dependency upgrading should be handled [default: only-if-needed]. "eager" - dependencies are upgraded regardless of whether the currently installed version satisfies the requirements of the upgraded package(s). "only-if-
                              needed" -  are upgraded only when they do not satisfy the requirements of the upgraded package(s).

Also, do we have any plan to stabilize this feature in near future?

@shepmaster
Copy link
Member

Determines how dependency upgrading should be handled

I am not following what this option has to do with Cargo. This feature is for installing binary packages and dependencies are not installed in a separate fashion (AIUI). Are you proposing that this subcommand will reinstall the binary package if any of its dependencies have updated since the binary was compiled? Do we even track that information?

@ehuss
Copy link
Contributor Author

ehuss commented Nov 4, 2019

I have posted a proposal to stabilize at #7560.

@frederikhors
Copy link

Is cargo install-upgrade present in cargo v.1.75?

@epage
Copy link
Contributor

epage commented Jan 14, 2024

This was stabilized in 1.41 (#7560) and we still have all of the tests for it, so I presume it still works. Note that this is just about cargo install foo being able to be used to upgrade foo, instead of erroring because it is already installed. If you are looking to update all installed packages, that is being tracked in #9527

@frederikhors
Copy link

With rust 1.75 if I install an already installed package it errors. Can you please confirm?

@RalfJung
Copy link
Member

@frederikhors this is a closed issue, please don't bring up new issues here. Instead open a new issue and add all the usual details (steps to reproduce, the exact command you used, the exact output you got).

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.

10 participants