From 8bcc0fa49d6a30cca9d346d106bf0e57c9b4cce5 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Fri, 13 Oct 2023 12:19:49 +0200 Subject: [PATCH 1/3] cargo/manifest: remember whether target names were inferred Add a boolean state to `Target` that tells us whether the name of the target was inferred by Cargo, or whether it was directly specified in the Manifest. This value will be required in the future, to allow changing the inferred names of targets, but retaining enough information to keep backwards compatibility. --- src/cargo/core/manifest.rs | 11 +++++++++++ src/cargo/util/toml/targets.rs | 1 + 2 files changed, 12 insertions(+) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 7886abec302..fcabe098ba6 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -207,6 +207,8 @@ pub struct Target { struct TargetInner { kind: TargetKind, name: String, + // Whether the name was inferred by Cargo, or explicitly given. + name_inferred: bool, // Note that `bin_name` is used for the cargo-feature `different_binary_name` bin_name: Option, // Note that the `src_path` here is excluded from the `Hash` implementation @@ -366,6 +368,7 @@ compact_debug! { [debug_the_fields( kind name + name_inferred bin_name src_path required_features @@ -657,6 +660,7 @@ impl Target { inner: Arc::new(TargetInner { kind: TargetKind::Bin, name: String::new(), + name_inferred: false, bin_name: None, src_path, required_features: None, @@ -790,6 +794,9 @@ impl Target { pub fn name(&self) -> &str { &self.inner.name } + pub fn name_inferred(&self) -> bool { + self.inner.name_inferred + } pub fn crate_name(&self) -> String { self.name().replace("-", "_") } @@ -964,6 +971,10 @@ impl Target { Arc::make_mut(&mut self.inner).name = name.to_string(); self } + pub fn set_name_inferred(&mut self, inferred: bool) -> &mut Target { + Arc::make_mut(&mut self.inner).name_inferred = inferred; + self + } pub fn set_binary_name(&mut self, bin_name: Option) -> &mut Target { Arc::make_mut(&mut self.inner).bin_name = bin_name; self diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 8455ae95b3e..46e86a192fb 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -247,6 +247,7 @@ fn clean_lib( let mut target = Target::lib_target(&lib.name(), crate_types, path, edition); configure(lib, &mut target)?; + target.set_name_inferred(toml_lib.map_or(true, |v| v.name.is_none())); Ok(Some(target)) } From 8b2cc62d377c9bef66453f32f4872bb84c498b70 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Fri, 13 Oct 2023 11:26:42 +0200 Subject: [PATCH 2/3] cargo/artifact: prepare compatibility env-vars We are about to change the default value for target-names of libraries. They used to match the package-name. In the future, they will use the package-name with dashes converted to underscores. This will affect the artifact env-variables, since they expose target-names. Hence, set the old env-vars, too, to avoid breakage. Note that we do not retain the name of a target before it was converted, and the conversion is lossy, so we cannot reconstruct it. However, we can rely on the fact that the conversion only happens for default values (since user-supplied values never allowed dashes). Furthermore, we now remember whether a target-name was inferred, so we can exactly reconstruct whether a library-target could have contained dashes in older releases, or not. --- src/cargo/core/compiler/artifact.rs | 30 ++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/compiler/artifact.rs b/src/cargo/core/compiler/artifact.rs index 1f3b12b5c09..ad6c4843b0d 100644 --- a/src/cargo/core/compiler/artifact.rs +++ b/src/cargo/core/compiler/artifact.rs @@ -29,15 +29,39 @@ pub fn get_env( let path = artifact_path.parent().expect("parent dir for artifacts"); env.insert(var, path.to_owned().into()); - let var = format!( + let var_file = format!( "CARGO_{}_FILE_{}_{}", artifact_type_upper, dep_name_upper, unit_dep.unit.target.name() ); - env.insert(var, artifact_path.to_owned().into()); - if unit_dep.unit.target.name() == dep_name.as_str() { + // In older releases, lib-targets defaulted to the name of the package. Newer releases + // use the same name as default, but with dashes replaced. Hence, if the name of the + // target was inferred by Cargo, we also set the env-var with the unconverted name for + // backwards compatibility. + let need_compat = unit_dep.unit.target.is_lib() && unit_dep.unit.target.name_inferred(); + if need_compat { + let var_compat = format!( + "CARGO_{}_FILE_{}_{}", + artifact_type_upper, + dep_name_upper, + unit_dep.unit.pkg.name(), + ); + if var_compat != var_file { + env.insert(var_compat, artifact_path.to_owned().into()); + } + } + + env.insert(var_file, artifact_path.to_owned().into()); + + // If the name of the target matches the name of the dependency, we strip the + // repetition and provide the simpler env-var as well. + // For backwards-compatibility of inferred names, we compare against the name of the + // package as well, since that used to be the default for library targets. + if unit_dep.unit.target.name() == dep_name.as_str() + || (need_compat && unit_dep.unit.pkg.name() == dep_name.as_str()) + { let var = format!("CARGO_{}_FILE_{}", artifact_type_upper, dep_name_upper,); env.insert(var, artifact_path.to_owned().into()); } From 3ca04e261e1abde062b1c58a31a0ae622547d972 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Fri, 6 Oct 2023 14:59:58 +0200 Subject: [PATCH 3/3] cargo: prevent dashes in lib.name The TOML parser of Cargo currently refuses `lib.name` entries that contain dashes. Unfortunately, it uses the package-name as default if no explicit `lib.name` entry is specified. This package-name, however, can contain dashes. Cargo documentation states that the package name is converted first, yet this was never implemented by the code-base. Fix this inconsistency and convert the package name to a suitable crate-name first. --- src/cargo/util/toml/targets.rs | 8 +++++--- tests/testsuite/artifact_dep.rs | 2 ++ tests/testsuite/collisions.rs | 2 +- tests/testsuite/doc.rs | 4 ++-- tests/testsuite/metadata.rs | 10 +++++----- tests/testsuite/test.rs | 16 ++++++++-------- 6 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 46e86a192fb..2a2104e6da1 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -154,18 +154,20 @@ fn clean_lib( let lib = match toml_lib { Some(lib) => { if let Some(ref name) = lib.name { - // XXX: other code paths dodge this validation if name.contains('-') { anyhow::bail!("library target names cannot contain hyphens: {}", name) } } Some(TomlTarget { - name: lib.name.clone().or_else(|| Some(package_name.to_owned())), + name: lib + .name + .clone() + .or_else(|| Some(package_name.replace("-", "_"))), ..lib.clone() }) } None => inferred.as_ref().map(|lib| TomlTarget { - name: Some(package_name.to_string()), + name: Some(package_name.replace("-", "_")), path: Some(PathValue(lib.clone())), ..TomlTarget::new() }), diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 64aa9d8afbc..1b7644b73e4 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -817,8 +817,10 @@ fn lib_with_selected_dashed_bin_artifact_and_lib_true() { let _b = include_bytes!(env!("CARGO_BIN_FILE_BAR_BAZ_baz-suffix")); let _b = include_bytes!(env!("CARGO_STATICLIB_FILE_BAR_BAZ")); let _b = include_bytes!(env!("CARGO_STATICLIB_FILE_BAR_BAZ_bar-baz")); + let _b = include_bytes!(env!("CARGO_STATICLIB_FILE_BAR_BAZ_bar_baz")); let _b = include_bytes!(env!("CARGO_CDYLIB_FILE_BAR_BAZ")); let _b = include_bytes!(env!("CARGO_CDYLIB_FILE_BAR_BAZ_bar-baz")); + let _b = include_bytes!(env!("CARGO_CDYLIB_FILE_BAR_BAZ_bar_baz")); } "#, ) diff --git a/tests/testsuite/collisions.rs b/tests/testsuite/collisions.rs index 77e05dd9c6d..0d370d92ed3 100644 --- a/tests/testsuite/collisions.rs +++ b/tests/testsuite/collisions.rs @@ -534,7 +534,7 @@ fn collision_with_root() { [DOWNLOADING] crates ... [DOWNLOADED] foo-macro v1.0.0 [..] warning: output filename collision. -The lib target `foo-macro` in package `foo-macro v1.0.0` has the same output filename as the lib target `foo-macro` in package `foo-macro v1.0.0 [..]`. +The lib target `foo_macro` in package `foo-macro v1.0.0` has the same output filename as the lib target `foo_macro` in package `foo-macro v1.0.0 [..]`. Colliding filename is: [CWD]/target/doc/foo_macro/index.html The targets should have unique names. This is a known bug where multiple crates with the same name use diff --git a/tests/testsuite/doc.rs b/tests/testsuite/doc.rs index a1698091246..b16dbc6d00d 100644 --- a/tests/testsuite/doc.rs +++ b/tests/testsuite/doc.rs @@ -2089,7 +2089,7 @@ fn doc_test_in_workspace() { ) .build(); p.cargo("test --doc -vv") - .with_stderr_contains("[DOCTEST] crate-a") + .with_stderr_contains("[DOCTEST] crate_a") .with_stdout_contains( " running 1 test @@ -2098,7 +2098,7 @@ test crate-a/src/lib.rs - (line 1) ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out[..] ", ) - .with_stderr_contains("[DOCTEST] crate-b") + .with_stderr_contains("[DOCTEST] crate_b") .with_stdout_contains( " running 1 test diff --git a/tests/testsuite/metadata.rs b/tests/testsuite/metadata.rs index fbead4dea97..f6fa6cfabbd 100644 --- a/tests/testsuite/metadata.rs +++ b/tests/testsuite/metadata.rs @@ -1616,7 +1616,7 @@ fn workspace_metadata_with_dependencies_and_resolve() { "kind": [ "lib" ], - "name": "non-artifact", + "name": "non_artifact", "src_path": "[..]/foo/non-artifact/src/lib.rs", "test": true } @@ -3110,7 +3110,7 @@ fn filter_platform() { "crate_types": [ "lib" ], - "name": "alt-dep", + "name": "alt_dep", "src_path": "[..]/alt-dep-0.0.1/src/lib.rs", "edition": "2015", "test": true, @@ -3154,7 +3154,7 @@ fn filter_platform() { "crate_types": [ "lib" ], - "name": "cfg-dep", + "name": "cfg_dep", "src_path": "[..]/cfg-dep-0.0.1/src/lib.rs", "edition": "2015", "test": true, @@ -3198,7 +3198,7 @@ fn filter_platform() { "crate_types": [ "lib" ], - "name": "host-dep", + "name": "host_dep", "src_path": "[..]/host-dep-0.0.1/src/lib.rs", "edition": "2015", "test": true, @@ -3242,7 +3242,7 @@ fn filter_platform() { "crate_types": [ "lib" ], - "name": "normal-dep", + "name": "normal_dep", "src_path": "[..]/normal-dep-0.0.1/src/lib.rs", "edition": "2015", "test": true, diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index 5f6528109be..d8fff6b8208 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -4671,9 +4671,9 @@ fn test_workspaces_cwd() { .build(); p.cargo("test --workspace --all") - .with_stderr_contains("[DOCTEST] root-crate") - .with_stderr_contains("[DOCTEST] nested-crate") - .with_stderr_contains("[DOCTEST] deep-crate") + .with_stderr_contains("[DOCTEST] root_crate") + .with_stderr_contains("[DOCTEST] nested_crate") + .with_stderr_contains("[DOCTEST] deep_crate") .with_stdout_contains("test test_unit_root_cwd ... ok") .with_stdout_contains("test test_unit_nested_cwd ... ok") .with_stdout_contains("test test_unit_deep_cwd ... ok") @@ -4683,33 +4683,33 @@ fn test_workspaces_cwd() { .run(); p.cargo("test -p root-crate --all") - .with_stderr_contains("[DOCTEST] root-crate") + .with_stderr_contains("[DOCTEST] root_crate") .with_stdout_contains("test test_unit_root_cwd ... ok") .with_stdout_contains("test test_integration_root_cwd ... ok") .run(); p.cargo("test -p nested-crate --all") - .with_stderr_contains("[DOCTEST] nested-crate") + .with_stderr_contains("[DOCTEST] nested_crate") .with_stdout_contains("test test_unit_nested_cwd ... ok") .with_stdout_contains("test test_integration_nested_cwd ... ok") .run(); p.cargo("test -p deep-crate --all") - .with_stderr_contains("[DOCTEST] deep-crate") + .with_stderr_contains("[DOCTEST] deep_crate") .with_stdout_contains("test test_unit_deep_cwd ... ok") .with_stdout_contains("test test_integration_deep_cwd ... ok") .run(); p.cargo("test --all") .cwd("nested-crate") - .with_stderr_contains("[DOCTEST] nested-crate") + .with_stderr_contains("[DOCTEST] nested_crate") .with_stdout_contains("test test_unit_nested_cwd ... ok") .with_stdout_contains("test test_integration_nested_cwd ... ok") .run(); p.cargo("test --all") .cwd("very/deeply/nested/deep-crate") - .with_stderr_contains("[DOCTEST] deep-crate") + .with_stderr_contains("[DOCTEST] deep_crate") .with_stdout_contains("test test_unit_deep_cwd ... ok") .with_stdout_contains("test test_integration_deep_cwd ... ok") .run();