From 31d679e81a55208e93fe50fa9a46eda31b259e35 Mon Sep 17 00:00:00 2001 From: Ted Kaminski Date: Fri, 21 Apr 2023 12:28:01 -0500 Subject: [PATCH 1/4] Build by PackageIdSpec, not name, to avoid ambiguity --- src/cargo/ops/cargo_install.rs | 7 +++++-- tests/testsuite/install.rs | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 53c3e72f79a..fc908051c88 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -4,7 +4,9 @@ use std::sync::Arc; use std::{env, fs}; use crate::core::compiler::{CompileKind, DefaultExecutor, Executor, UnitOutput}; -use crate::core::{Dependency, Edition, Package, PackageId, Source, SourceId, Target, Workspace}; +use crate::core::{ + Dependency, Edition, Package, PackageId, PackageIdSpec, Source, SourceId, Target, Workspace, +}; use crate::ops::{common_for_install_and_uninstall::*, FilterRule}; use crate::ops::{CompileFilter, Packages}; use crate::sources::{GitSource, PathSource, SourceConfigMap}; @@ -173,7 +175,8 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> { // 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 pkgidspec = PackageIdSpec::from_package_id(pkg.package_id()); + opts.spec = Packages::Packages(vec![pkgidspec.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 diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index dd9844f170b..2447f0a79d4 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2287,3 +2287,39 @@ fn sparse_install() { "#, ); } + +#[cargo_test] +fn self_referential() { + // Some packages build-dep on prior versions of themselves. + Package::new("foo", "0.0.1") + .file("src/lib.rs", "fn hello() {}") + .file("src/main.rs", "fn main() {}") + .file("build.rs", "fn main() {}") + .publish(); + Package::new("foo", "0.0.2") + .file("src/lib.rs", "fn hello() {}") + .file("src/main.rs", "fn main() {}") + .file("build.rs", "fn main() {}") + .build_dep("foo", "0.0.1") + .publish(); + + cargo_process("install foo") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.2 (registry [..]) +[INSTALLING] foo v0.0.2 +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry [..]) +[COMPILING] foo v0.0.1 +[COMPILING] foo v0.0.2 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE] +[INSTALLED] package `foo v0.0.2` (executable `foo[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); + assert_has_installed_exe(cargo_home(), "foo"); +} From e97cc80d9a1dbb273a3ea7c379ce6598ca690f30 Mon Sep 17 00:00:00 2001 From: Ted Kaminski Date: Fri, 21 Apr 2023 13:49:32 -0500 Subject: [PATCH 2/4] Determine pkg-spec from workspace, not original package which may be a git url --- src/cargo/ops/cargo_install.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index fc908051c88..a183975fb0e 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -170,15 +170,8 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> { } }; - // 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(); - let pkgidspec = PackageIdSpec::from_package_id(pkg.package_id()); - opts.spec = Packages::Packages(vec![pkgidspec.to_string()]); - - let (ws, rustc, target) = make_ws_rustc_target(config, &opts, &source_id, pkg.clone())?; + let (ws, rustc, target) = + make_ws_rustc_target(config, &original_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() { @@ -195,6 +188,16 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> { ws.current()?.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(); + // Unlike install source tracking (for --git, see above), use the spec url from the + // build workspace (hence unconditional `ws.current()` instead of `pkg` to get `package_id()`). + let pkgidspec = PackageIdSpec::from_package_id(ws.current()?.package_id()); + opts.spec = Packages::Packages(vec![pkgidspec.to_string()]); + if from_cwd { if pkg.manifest().edition() == Edition::Edition2015 { config.shell().warn( From 79bb2d71430999ccbf0284943b0494bee7e128f5 Mon Sep 17 00:00:00 2001 From: Ted Kaminski Date: Mon, 1 May 2023 17:21:37 -0500 Subject: [PATCH 3/4] Update pkg-spec comment, and add 2 more test cases --- src/cargo/ops/cargo_install.rs | 5 +- tests/testsuite/install.rs | 85 ++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index a183975fb0e..5f843e8c731 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -193,8 +193,9 @@ impl<'cfg, 'a> InstallablePackage<'cfg, 'a> { // specialized compile options specific to the identified package. // See test `path_install_workspace_root_despite_default_members`. let mut opts = original_opts.clone(); - // Unlike install source tracking (for --git, see above), use the spec url from the - // build workspace (hence unconditional `ws.current()` instead of `pkg` to get `package_id()`). + // For cargo install tracking, we retain the source git url in `pkg`, but for the build spec + // we need to unconditionally use `ws.current()` to correctly address the path where we + // locally cloned that repo. let pkgidspec = PackageIdSpec::from_package_id(ws.current()?.package_id()); opts.spec = Packages::Packages(vec![pkgidspec.to_string()]); diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 2447f0a79d4..a0a77c2d6ca 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -1415,6 +1415,46 @@ fn path_install_workspace_root_despite_default_members() { .run(); } +#[cargo_test] +fn git_install_workspace_root_despite_default_members() { + let p = git::repo(&paths::root().join("foo")) + .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(); + + cargo_process("install --git") + .arg(p.url().to_string()) + .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(); @@ -2323,3 +2363,48 @@ fn self_referential() { .run(); assert_has_installed_exe(cargo_home(), "foo"); } + +#[cargo_test] +fn ambiguous_registry_vs_local_workspace_package() { + // Correctly install 'foo' from a workspace, even if that workspace + // (somewhere) also depends on a registry package named 'foo'. + Package::new("foo", "0.0.1") + .file("src/lib.rs", "fn hello() {}") + .publish(); + + let p = project() + .file("src/main.rs", "fn main() {}") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2021" + + [dependencies] + foo = "0.0.1" + "#, + ) + .build(); + + cargo_process("install --path") + .arg(p.root()) + .with_stderr( + "\ +[INSTALLING] foo v0.1.0 ([..]) +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry [..]) +[COMPILING] foo v0.0.1 +[COMPILING] foo v0.1.0 ([..]) +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/foo[EXE] +[INSTALLED] package `foo v0.1.0 ([..])` (executable `foo[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); + assert_has_installed_exe(cargo_home(), "foo"); +} From 7fb35c9c4eb99c67d0488edf412cd1d6f17c374c Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 3 May 2023 01:04:18 +0100 Subject: [PATCH 4/4] test(install): correct the term workspace -> local packagd --- tests/testsuite/install.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index a0a77c2d6ca..9b881dfdc68 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2365,9 +2365,9 @@ fn self_referential() { } #[cargo_test] -fn ambiguous_registry_vs_local_workspace_package() { - // Correctly install 'foo' from a workspace, even if that workspace - // (somewhere) also depends on a registry package named 'foo'. +fn ambiguous_registry_vs_local_package() { + // Correctly install 'foo' from a local package, even if that package also + // depends on a registry dependency named 'foo'. Package::new("foo", "0.0.1") .file("src/lib.rs", "fn hello() {}") .publish();