Auto merge of #7560 - ehuss:stabilize-install-upgrade, r=alexcrichton

Stabilize install-upgrade.

Tracking issue: #6797

This stabilizes the install-upgrade feature, which causes `cargo install` to reinstall a package if it appears to be out of date, or exit cleanly if it is up-to-date.

There are no changes from `-Zinstall-upgrade`. See [the old unstable docs](https://github.com/rust-lang/cargo/blob/6a7f505a185b000e38bdad64392098e0b2e50802/src/doc/src/reference/unstable.md#install-upgrade) for a refresher on the details of what it does.

This also stabilizes the following changes:
- `--version` no longer allows an implicit version requirement like `1.2`.  It must be either contain all 3 components (like `1.2.3`) or use a requirement operator (like `^1.2`).  This has been a warning for a very long time, and is now changed to a hard error.
- Added `--no-track` to disable install tracking.

**Motivation**

I just personally prefer this behavior, and it has been requested a few times in the past. I've been using it locally, and haven't run into any issues. If this goes into 1.41, then it will release on Jan 30, about 10 months since it was added in #6798.

**Concerns**

Regarding some of the concerns I had:

- Is it tracking the correct set of information?

  I'm satisfied with the current set. It also tracks, but does not use, the version of rustc and the version specified in the `--version` flag, in case we ever want to use that in the future. It is also designed to be backwards and forwards compatible, so more information can be added in the future. I think the current set strikes a good balance of tracking the really important things, without causing unnecessary rebuilds.

- Method to upgrade all outdated packages?

  This can always be added as a new flag or command in the future, and shouldn't block stabilization.

- Should `--no-track` be kept? Does it work correctly?

  I kept it. It's not too hard to support, and nobody said anything (other than maybe using a less confusing name).

- Should this be the default? Should there be a way to use the old behavior?

  I like it as the default, and don't see a real need for the old behavior. I think we could always bring back the old behavior with a flag in the future, but I would like to avoid it for simplicity. There is also the workaround of `which foo || cargo install foo`.

Closes #6797.
Closes #6727.
Closes #6485.
Closes #2082.

Author: bors

src/bin/cargo/cli.rs

@@ -34,7 +34,6 @@ Available unstable (nightly-only) flags:
     -Z no-index-update  -- Do not update the registry, avoids a network request for benchmarking
     -Z unstable-options -- Allow the usage of unstable options such as --registry
     -Z config-profile   -- Read profiles from .cargo/config files
-    -Z install-upgrade  -- `cargo install` will upgrade instead of failing
     -Z timings          -- Display concurrency information
     -Z doctest-xcompile -- Compile and run doctests for non-host target using runner config
 

src/bin/cargo/commands/install.rs

@@ -46,10 +46,7 @@ pub fn cli() -> App {
         ))
         .arg_jobs()
         .arg(opt("force", "Force overwriting existing crates or binaries").short("f"))
-        .arg(opt(
-            "no-track",
-            "Do not save tracking information (unstable)",
-        ))
+        .arg(opt("no-track", "Do not save tracking information"))
         .arg_features()
         .arg_profile("Install artifacts with the specified profile")
         .arg(opt("debug", "Build in debug mode instead of release mode"))
@@ -90,13 +87,9 @@ crate has multiple binaries, the `--bin` argument can selectively install only
 one of them, and if you'd rather install examples the `--example` argument can
 be used as well.
 
-By default cargo will refuse to overwrite existing binaries. The `--force` flag
-enables overwriting existing binaries. Thus you can reinstall a crate with
-`cargo install --force <crate>`.
-
-Omitting the <crate> specification entirely will install the crate in the
-current directory. This behaviour is deprecated, and it no longer works in the
-Rust 2018 edition. Use the more explicit `install --path .` instead.
+If the package is already installed, Cargo will reinstall it if the installed
+version does not appear to be up-to-date. Installing with `--path` will always
+build and install, unless there are conflicting binaries from another package.
 
 If the source is crates.io or `--git` then by default the crate will be built
 in a temporary target directory. To avoid this, the target directory can be
@@ -159,13 +152,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     let version = args.value_of("version");
     let root = args.value_of("root");
 
-    if args.is_present("no-track") && !config.cli_unstable().install_upgrade {
-        return Err(failure::format_err!(
-            "`--no-track` flag is unstable, pass `-Z install-upgrade` to enable it"
-        )
-        .into());
-    };
-
     if args.is_present("list") {
         ops::install_list(root, config)?;
     } else {

src/cargo/core/features.rs

@@ -334,7 +334,6 @@ pub struct CliUnstable {
     pub config_profile: bool,
     pub dual_proc_macros: bool,
     pub mtime_on_use: bool,
-    pub install_upgrade: bool,
     pub named_profiles: bool,
     pub binary_dep_depinfo: bool,
     pub build_std: Option<Vec<String>>,
@@ -400,7 +399,6 @@ impl CliUnstable {
             "dual-proc-macros" => self.dual_proc_macros = parse_empty(k, v)?,
             // can also be set in .cargo/config or with and ENV
             "mtime-on-use" => self.mtime_on_use = parse_empty(k, v)?,
-            "install-upgrade" => self.install_upgrade = parse_empty(k, v)?,
             "named-profiles" => self.named_profiles = parse_empty(k, v)?,
             "binary-dep-depinfo" => self.binary_dep_depinfo = parse_empty(k, v)?,
             "build-std" => {

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -18,33 +18,31 @@ use crate::util::{FileLock, Filesystem};
 
 /// On-disk tracking for which package installed which binary.
 ///
-/// v1 is an older style, v2 is a new (experimental) style that tracks more
-/// information. The new style is only enabled with the `-Z install-upgrade`
-/// flag (which sets the `unstable_upgrade` flag). v1 is still considered the
-/// source of truth. When v2 is used, it will sync with any changes with v1,
-/// and will continue to update v1.
+/// v1 is an older style, v2 is a new style that tracks more information, and
+/// is both backwards and forwards compatible. Cargo keeps both files in sync,
+/// updating both v1 and v2 at the same time. Additionally, if it detects
+/// changes in v1 that are not in v2 (such as when an older version of Cargo
+/// is used), it will automatically propagate those changes to v2.
 ///
 /// This maintains a filesystem lock, preventing other instances of Cargo from
 /// modifying at the same time. Drop the value to unlock.
 ///
-/// If/when v2 is stabilized, it is intended that v1 is retained for a while
-/// during a longish transition period, and then v1 can be removed.
+/// It is intended that v1 should be retained for a while during a longish
+/// transition period, and then v1 can be removed.
 pub struct InstallTracker {
     v1: CrateListingV1,
     v2: CrateListingV2,
     v1_lock: FileLock,
-    v2_lock: Option<FileLock>,
-    unstable_upgrade: bool,
+    v2_lock: FileLock,
 }
 
 /// Tracking information for the set of installed packages.
-///
-/// This v2 format is unstable and requires the `-Z unstable-upgrade` option
-/// to enable.
 #[derive(Default, Deserialize, Serialize)]
 struct CrateListingV2 {
+    /// Map of every installed package.
     installs: BTreeMap<PackageId, InstallInfo>,
-    /// Forwards compatibility.
+    /// Forwards compatibility. Unknown keys from future versions of Cargo
+    /// will be stored here and retained when the file is saved.
     #[serde(flatten)]
     other: BTreeMap<String, serde_json::Value>,
 }
@@ -56,7 +54,7 @@ struct CrateListingV2 {
 /// determine if it needs to be rebuilt/reinstalled. If nothing has changed,
 /// then Cargo will inform the user that it is "up to date".
 ///
-/// This is only used for the (unstable) v2 format.
+/// This is only used for the v2 format.
 #[derive(Debug, Deserialize, Serialize)]
 struct InstallInfo {
     /// Version requested via `--version`.
@@ -87,19 +85,15 @@ struct InstallInfo {
 /// Tracking information for the set of installed packages.
 #[derive(Default, Deserialize, Serialize)]
 pub struct CrateListingV1 {
+    /// Map of installed package id to the set of binary names for that package.
     v1: BTreeMap<PackageId, BTreeSet<String>>,
 }
 
 impl InstallTracker {
     /// Create an InstallTracker from information on disk.
     pub fn load(config: &Config, root: &Filesystem) -> CargoResult<InstallTracker> {
-        let unstable_upgrade = config.cli_unstable().install_upgrade;
         let v1_lock = root.open_rw(Path::new(".crates.toml"), config, "crate metadata")?;
-        let v2_lock = if unstable_upgrade {
-            Some(root.open_rw(Path::new(".crates2.json"), config, "crate metadata")?)
-        } else {
-            None
-        };
+        let v2_lock = root.open_rw(Path::new(".crates2.json"), config, "crate metadata")?;
 
         let v1 = (|| -> CargoResult<_> {
             let mut contents = String::new();
@@ -119,26 +113,21 @@ impl InstallTracker {
         })?;
 
         let v2 = (|| -> CargoResult<_> {
-            match &v2_lock {
-                Some(lock) => {
-                    let mut contents = String::new();
-                    lock.file().read_to_string(&mut contents)?;
-                    let mut v2 = if contents.is_empty() {
-                        CrateListingV2::default()
-                    } else {
-                        serde_json::from_str(&contents)
-                            .chain_err(|| format_err!("invalid JSON found for metadata"))?
-                    };
-                    v2.sync_v1(&v1)?;
-                    Ok(v2)
-                }
-                None => Ok(CrateListingV2::default()),
-            }
+            let mut contents = String::new();
+            v2_lock.file().read_to_string(&mut contents)?;
+            let mut v2 = if contents.is_empty() {
+                CrateListingV2::default()
+            } else {
+                serde_json::from_str(&contents)
+                    .chain_err(|| format_err!("invalid JSON found for metadata"))?
+            };
+            v2.sync_v1(&v1)?;
+            Ok(v2)
         })()
         .chain_err(|| {
             format_err!(
                 "failed to parse crate metadata at `{}`",
-                v2_lock.as_ref().unwrap().path().to_string_lossy()
+                v2_lock.path().to_string_lossy()
             )
         })?;
 
@@ -147,7 +136,6 @@ impl InstallTracker {
             v2,
             v1_lock,
             v2_lock,
-            unstable_upgrade,
         })
     }
 
@@ -204,7 +192,7 @@ impl InstallTracker {
 
         // If both sets are the same length, that means all duplicates come
         // from packages with the same name.
-        if self.unstable_upgrade && matching_duplicates.len() == duplicates.len() {
+        if matching_duplicates.len() == duplicates.len() {
             // Determine if it is dirty or fresh.
             let source_id = pkg.package_id().source_id();
             if source_id.is_path() {
@@ -265,11 +253,8 @@ impl InstallTracker {
             .filter_map(|name| {
                 if !dst.join(&name).exists() {
                     None
-                } else if self.unstable_upgrade {
-                    let p = self.v2.package_for_bin(name);
-                    Some((name.clone(), p))
                 } else {
-                    let p = self.v1.package_for_bin(name);
+                    let p = self.v2.package_for_bin(name);
                     Some((name.clone(), p))
                 }
             })
@@ -286,10 +271,8 @@ impl InstallTracker {
         target: &str,
         rustc: &str,
     ) {
-        if self.unstable_upgrade {
-            self.v2
-                .mark_installed(package, bins, version_req, opts, target, rustc)
-        }
+        self.v2
+            .mark_installed(package, bins, version_req, opts, target, rustc);
         self.v1.mark_installed(package, bins);
     }
 
@@ -302,14 +285,12 @@ impl InstallTracker {
             )
         })?;
 
-        if self.unstable_upgrade {
-            self.v2.save(self.v2_lock.as_ref().unwrap()).chain_err(|| {
-                format_err!(
-                    "failed to write crate metadata at `{}`",
-                    self.v2_lock.as_ref().unwrap().path().to_string_lossy()
-                )
-            })?;
-        }
+        self.v2.save(&self.v2_lock).chain_err(|| {
+            format_err!(
+                "failed to write crate metadata at `{}`",
+                self.v2_lock.path().to_string_lossy()
+            )
+        })?;
         Ok(())
     }
 
@@ -329,20 +310,11 @@ impl InstallTracker {
     /// Remove a package from the tracker.
     pub fn remove(&mut self, pkg_id: PackageId, bins: &BTreeSet<String>) {
         self.v1.remove(pkg_id, bins);
-        if self.unstable_upgrade {
-            self.v2.remove(pkg_id, bins);
-        }
+        self.v2.remove(pkg_id, bins);
     }
 }
 
 impl CrateListingV1 {
-    fn package_for_bin(&self, bin_name: &str) -> Option<PackageId> {
-        self.v1
-            .iter()
-            .find(|(_, bins)| bins.contains(bin_name))
-            .map(|(pkg_id, _)| *pkg_id)
-    }
-
     fn mark_installed(&mut self, pkg: &Package, bins: &BTreeSet<String>) {
         // Remove bins from any other packages.
         for other_bins in self.v1.values_mut() {
@@ -600,24 +572,11 @@ where
                 match v.to_semver() {
                     Ok(v) => Some(format!("={}", v)),
                     Err(e) => {
-                        let mut msg = if config.cli_unstable().install_upgrade {
-                            format!(
-                                "the `--vers` provided, `{}`, is \
-                                 not a valid semver version: {}\n",
-                                v, e
-                            )
-                        } else {
-                            format!(
-                                "the `--vers` provided, `{}`, is \
-                                 not a valid semver version\n\n\
-                                 historically Cargo treated this \
-                                 as a semver version requirement \
-                                 accidentally\nand will continue \
-                                 to do so, but this behavior \
-                                 will be removed eventually",
-                                v
-                            )
-                        };
+                        let mut msg = format!(
+                            "the `--vers` provided, `{}`, is \
+                             not a valid semver version: {}\n",
+                            v, e
+                        );
 
                         // If it is not a valid version but it is a valid version
                         // requirement, add a note to the warning
@@ -628,12 +587,7 @@ where
                                 v
                             ));
                         }
-                        if config.cli_unstable().install_upgrade {
-                            bail!(msg);
-                        } else {
-                            config.shell().warn(&msg)?;
-                        }
-                        Some(v.to_string())
+                        bail!(msg);
                     }
                 }
             }

src/doc/man/cargo-install.adoc

@@ -37,6 +37,20 @@ crate has multiple binaries, the `--bin` argument can selectively install only
 one of them, and if you'd rather install examples the `--example` argument can
 be used as well.
 
+If the package is already installed, Cargo will reinstall it if the installed
+version does not appear to be up-to-date. If any of the following values
+change, then Cargo will reinstall the package:
+
+- The package version and source.
+- The set of binary names installed.
+- The chosen features.
+- The release mode (`--debug`).
+- The target (`--target`).
+
+Installing with `--path` will always build and install, unless there are
+conflicting binaries from another package. The `--force` flag may be used to
+force Cargo to always reinstall the package.
+
 If the source is crates.io or `--git` then by default the crate will be built
 in a temporary target directory. To avoid this, the target directory can be
 specified by setting the `CARGO_TARGET_DIR` environment variable to a relative
@@ -63,7 +77,13 @@ available.
 
 *--vers* _VERSION_::
 *--version* _VERSION_::
-    Specify a version to install.
+    Specify a version to install. This may be a
+    linkcargo:reference/specifying-dependencies.md[version requirement], like
+    `~1.2`, to have Cargo select the newest version from the given
+    requirement. If the version does not have a requirement operator (such as
+    `^` or `~`), then it must be in the form _MAJOR.MINOR.PATCH_, and will
+    install exactly that version; it is *not* treated as a caret requirement
+    like Cargo dependencies are.
 
 *--git* _URL_::
     Git URL to install the specified crate from.
@@ -85,9 +105,18 @@ available.
 
 *-f*::
 *--force*::
-    Force overwriting existing crates or binaries. This can be used to
-    reinstall or upgrade a crate.
-
+    Force overwriting existing crates or binaries. This can be used if a
+    package has installed a binary with the same name as another package. This
+    is also useful if something has changed on the system that you want to
+    rebuild with, such as a newer version of `rustc`.
+
+*--no-track*::
+    By default, Cargo keeps track of the installed packages with a metadata
+    file stored in the installation root directory. This flag tells Cargo not
+    to use or create that file. With this flag, Cargo will refuse to overwrite
+    any existing files unless the `--force` flag is used. This also disables
+    Cargo's ability to protect against multiple concurrent invocations of
+    Cargo installing at the same time.
 
 *--bin* _NAME_...::
     Install only the specified binary.
@@ -137,13 +166,17 @@ include::section-exit-status.adoc[]
 
 == EXAMPLES
 
-. Install a package from crates.io:
+. Install or upgrade a package from crates.io:
 
     cargo install ripgrep
 
-. Reinstall or upgrade a package:
+. Install or reinstall the package in the current directory:
+
+    cargo install --path .
+
+. View the list of installed packages:
 
-    cargo install ripgrep --force
+    cargo install --list
 
 == SEE ALSO
 man:cargo[1], man:cargo-uninstall[1], man:cargo-search[1], man:cargo-publish[1]