Auto merge of #11067 - tedinski:install_workspace_root, r=weihanglo

Ignore `workspace.default-members` when running `cargo install` on root package of a non-virtual workspace

### What does this PR try to resolve?

* Fixes #11058

Two observable behaviors are fixed:

1. When running `cargo install` with `--path` or `--git` and specifically requesting the root package of a non-virtual workspace, `cargo install` will accidentally build `workspace.default-members` instead of the requested root package.
2. Further, if that `default-members` does not include the root package, it will install binaries from those other packages (the `default-members`) and claim they were the binaries from the root package! There is no way, actually, to install the root package binaries.

These two behaviors have the same root cause:

* `cargo install` effectively does `cargo build --release` in the requested package directory, but when this is the root of a non-virtual workspace, that builds `default-members` instead of the requested package.

### How should we test and review this PR?

I have included a test exhibiting this behavior. It currently fails in the manner indicated in the test, and passes with the changes included in this PR.

I'm not sure the solution in the PR is the _best_ solution, but the alternative I am able to come up with involves much more extensive changes to how `cargo install` works, to produce a distinct `CompileOptions` for every package built. I tried to keep the new workspace "API" `ignore_default_members()` as narrowly-scoped in its effect as possible.

### Additional information

The only way I could think this behavior change could impact someone is if they were somehow using `cargo install --path` (or `--git`) and wanting it to actually install the binaries from all of `default-members`. However, I don't believe that's possible, since if there are multiple packages with binaries, I believe cargo requires the packages to be specified. So someone would have to be additionally relying on specifying just the root package, but then wanting the binaries from more than just the root. I think this is probably an acceptable risk for merging!

Author: bors

src/cargo/core/compiler/build_config.rs

@@ -6,10 +6,11 @@ use cargo_util::ProcessBuilder;
 use serde::ser;
 use std::cell::RefCell;
 use std::path::PathBuf;
+use std::sync::Arc;
 use std::thread::available_parallelism;
 
 /// Configuration information for a rustc build.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct BuildConfig {
     /// The requested kind of compilation for this session
     pub requested_kinds: Vec<CompileKind>,
@@ -33,7 +34,7 @@ pub struct BuildConfig {
     pub primary_unit_rustc: Option<ProcessBuilder>,
     /// A thread used by `cargo fix` to receive messages on a socket regarding
     /// the success/failure of applying fixes.
-    pub rustfix_diagnostic_server: RefCell<Option<RustfixDiagnosticServer>>,
+    pub rustfix_diagnostic_server: Arc<RefCell<Option<RustfixDiagnosticServer>>>,
     /// The directory to copy final artifacts to. Note that even if `out_dir` is
     /// set, a copy of artifacts still could be found a `target/(debug\release)`
     /// as usual.
@@ -100,7 +101,7 @@ impl BuildConfig {
             build_plan: false,
             unit_graph: false,
             primary_unit_rustc: None,
-            rustfix_diagnostic_server: RefCell::new(None),
+            rustfix_diagnostic_server: Arc::new(RefCell::new(None)),
             export_dir: None,
             future_incompat_report: false,
             timing_outputs: Vec::new(),

src/cargo/ops/cargo_compile/compile_filter.rs

@@ -34,7 +34,7 @@ pub enum FilterRule {
 ///
 /// [`generate_root_units`]: super::UnitGenerator::generate_root_units
 /// [`Packages`]: crate::ops::Packages
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum CompileFilter {
     /// The default set of Cargo targets.
     Default {

src/cargo/ops/cargo_compile/mod.rs

@@ -68,7 +68,7 @@ pub use packages::Packages;
 /// of it as `CompileOptions` are high-level settings requested on the
 /// command-line, and `BuildConfig` are low-level settings for actually
 /// driving `rustc`.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct CompileOptions {
     /// Configuration information for a rustc build
     pub build_config: BuildConfig,

src/cargo/ops/cargo_compile/packages.rs

@@ -13,7 +13,7 @@ use anyhow::{bail, Context as _};
 ///
 /// Generally, it represents the combination of all `-p` flag. When working within
 /// a workspace, `--exclude` and `--workspace` flags also contribute to it.
-#[derive(PartialEq, Eq, Debug)]
+#[derive(PartialEq, Eq, Debug, Clone)]
 pub enum Packages {
     /// Packages selected by default. Usually means no flag provided.
     Default,

src/cargo/ops/cargo_install.rs

@@ -5,8 +5,8 @@ use std::{env, fs};
 
 use crate::core::compiler::{CompileKind, DefaultExecutor, Executor, UnitOutput};
 use crate::core::{Dependency, Edition, Package, PackageId, Source, SourceId, Workspace};
-use crate::ops::CompileFilter;
 use crate::ops::{common_for_install_and_uninstall::*, FilterRule};
+use crate::ops::{CompileFilter, Packages};
 use crate::sources::{GitSource, PathSource, SourceConfigMap};
 use crate::util::errors::CargoResult;
 use crate::util::{Config, Filesystem, Rustc, ToSemver, VersionReqExt};
@@ -37,7 +37,7 @@ impl Drop for Transaction {
 
 struct InstallablePackage<'cfg, 'a> {
     config: &'cfg Config,
-    opts: &'a ops::CompileOptions,
+    opts: ops::CompileOptions,
     root: Filesystem,
     source_id: SourceId,
     vers: Option<&'a str>,
@@ -60,7 +60,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
         source_id: SourceId,
         from_cwd: bool,
         vers: Option<&'a str>,
-        opts: &'a ops::CompileOptions,
+        original_opts: &'a ops::CompileOptions,
         force: bool,
         no_track: bool,
         needs_update_if_source_is_index: bool,
@@ -145,7 +145,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
                     dep.clone(),
                     &mut source,
                     config,
-                    opts,
+                    original_opts,
                     &root,
                     &dst,
                     force,
@@ -167,7 +167,14 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
             }
         };
 
-        let (ws, rustc, target) = make_ws_rustc_target(config, opts, &source_id, pkg.clone())?;
+        // When we build this package, we want to build the *specified* package only,
+        // and avoid building e.g. workspace default-members instead. Do so by constructing
+        // specialized compile options specific to the identified package.
+        // See test `path_install_workspace_root_despite_default_members`.
+        let mut opts = original_opts.clone();
+        opts.spec = Packages::Packages(vec![pkg.name().to_string()]);
+
+        let (ws, rustc, target) = make_ws_rustc_target(config, &opts, &source_id, pkg.clone())?;
         // If we're installing in --locked mode and there's no `Cargo.lock` published
         // ie. the bin was published before https://github.com/rust-lang/cargo/pull/7026
         if config.locked() && !ws.root().join("Cargo.lock").exists() {
@@ -235,7 +242,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
             // Check for conflicts.
             ip.no_track_duplicates(&dst)?;
         } else if is_installed(
-            &ip.pkg, config, opts, &ip.rustc, &ip.target, &ip.root, &dst, force,
+            &ip.pkg, config, &ip.opts, &ip.rustc, &ip.target, &ip.root, &dst, force,
         )? {
             let msg = format!(
                 "package `{}` is already installed, use --force to override",
@@ -297,7 +304,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
         self.check_yanked_install()?;
 
         let exec: Arc<dyn Executor> = Arc::new(DefaultExecutor);
-        let compile = ops::compile_ws(&self.ws, self.opts, &exec).with_context(|| {
+        let compile = ops::compile_ws(&self.ws, &self.opts, &exec).with_context(|| {
             if let Some(td) = td_opt.take() {
                 // preserve the temporary directory, so the user can inspect it
                 td.into_path();
@@ -372,7 +379,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
                 &dst,
                 &self.pkg,
                 self.force,
-                self.opts,
+                &self.opts,
                 &self.target,
                 &self.rustc.verbose_version,
             )?;
@@ -439,7 +446,7 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> {
                 &self.pkg,
                 &successful_bins,
                 self.vers.map(|s| s.to_string()),
-                self.opts,
+                &self.opts,
                 &self.target,
                 &self.rustc.verbose_version,
             );

tests/testsuite/install.rs

@@ -1292,6 +1292,46 @@ fn use_path_workspace() {
     assert_eq!(lock, lock2, "different lockfiles");
 }
 
+#[cargo_test]
+fn path_install_workspace_root_despite_default_members() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "ws-root"
+                version = "0.1.0"
+                authors = []
+
+                [workspace]
+                members = ["ws-member"]
+                default-members = ["ws-member"]
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            "ws-member/Cargo.toml",
+            r#"
+                [package]
+                name = "ws-member"
+                version = "0.1.0"
+                authors = []
+            "#,
+        )
+        .file("ws-member/src/main.rs", "fn main() {}")
+        .build();
+
+    p.cargo("install --path")
+        .arg(p.root())
+        .arg("ws-root")
+        .with_stderr_contains(
+            "[INSTALLED] package `ws-root v0.1.0 ([..])` (executable `ws-root[EXE]`)",
+        )
+        // Particularly avoid "Installed package `ws-root v0.1.0 ([..]])` (executable `ws-member`)":
+        .with_stderr_does_not_contain("ws-member")
+        .run();
+}
+
 #[cargo_test]
 fn dev_dependencies_no_check() {
     Package::new("foo", "1.0.0").publish();