Auto merge of #9992 - Byron:rfc-3028, r=ehuss

Implement "artifact dependencies" (RFC-3028)

Tracking issue: #9096

#### Tasks

* [x] -Z unstable option called `bindeps`
* ✅ **(config)** allow parsing of newly introduced 'artifact' fields
   * [x] into `TomlManifest`
   * [x] into `Manifest`
      - [x] ~~abort~~ warn if artifacts are used on stable
* ✅ **resolver** : assure artifact dependencies are part of the resolution process and unified into the dependency tree
* 🔬**compiler**: make it understand 'artifact' dependencies and pass new environment variables to the crate being build
  * [x] `lib=false` should not be considered a rust library for the dependent, in unit and possibly resolve graph
  * [x] assure profile settings are applied correctly
  * [x] target overrides work
      * [x] `target = "target"` in build deps
      * [x] other targets on build deps
      * [x] other targets on non-build deps
      * [x] 'no-cross doc tests' seems like a constraint we should apply as well maybe
  * [x] more confidence with `resolver = "2"`
  * [x] assure artifact placement is correct (bin and various forms of lib)
* ✅ **serialization**: rewriting manifests (i.e. for publishing) does not discard artifact information
   * [x] publishing keeps `artifact` and `lib` values
* **Other cargo subcommands**
   * [x] `cargo metadata`
        * leave unchanged
   * [x] artifacts work with `cargo check`
   * [x] artifacts work with rustdoc, such that it doesn't document them unless `lib=true`
   * [x] `cargo tree` maybe?
   * [x] `cargo clean` should clean artifacts - even though it's more complex, ultimately it deletes the `target` directory.
   * [x] artifacts work with `cargo test` (and dev-dependencies)
       * [x] doctests
       * [x] try `reproducible` repository as well.
* 🧪 **tests** for more subtle RFC constraints
   - [x] build scripts cannot access artifact environment variables at compile time, only at runtime)
   - [x] assure 'examples' which also support crate-type fields like [[lib]] won't become artifacts themselves.
   - [x] assure `--out-dir` does not leak artifacts - tested manually, it seemed to niche to add a test.
   - [x] try `target="foo"` in artifact and assure it sees a decent error message
   - [x] Assure RFC 3176 _doesn't_ work
* 🧹cleanup and finalization
    - [x] assure no `TODO(ST)` markers are left in code
    - [x] assure no tests are ignored
    - [x] use `resolver = "1"` once to assert everything also works with the previous resolver, but leave it on "2".

#### Implementation and review notes

- artifacts and unstable options are only checked when transforming them from `TomlManifest` to `Manifest`, discarding artifact information if the unstable flag is not set. Nowhere else in code will the CLI options be checked again.
- `If no binaries are specified, all the binaries in the package will be built and made available.` - this should only refer to `[[bin]]` targets, not examples for instance.
- artifact binaries won't be uplifted, hence won't be present outside of their output directory
- ❗️We don't know how [package links](https://github.com/rust-lang/cargo/blob/00e925f61fbd9f2d956046aea5af6b7636ab2931/src/cargo/core/compiler/unit_dependencies.rs#L380) will affect artifacts for build dependencies. Should probably be thought through.
- ❗️The location of artifacts is only tested roughly to avoid having to deal with different output names on the four platforms that seem to matter (gnu, macos, windows msvc, windows gnu).
- `cargo tree` doesn't handle artifacts specifically, and it might be interesting to make clear if an artifact is only an artifact, or both artifact and dependency.
- Most error and warning messages can probably be more cargo-matic.

#### Questions

* Does `cargo` without the feature enabled have to complain about the "artifact" field in a dependency, like it does right now?  It doesn't look like machinery for that exists in `do_read_manifest()`.
   - ✔️It warns now
*  Should parsing of artifact values, like "bin" be case sensitive?
   - ✔️ It's case sensitive now, which should help with serde roundtripping.

#### Review Progress

* [x] address Josh's review notes one by one
   * [x] introduce `IsArtifact` (see [this answer](#9992 (comment))) (76cd48a2d62d74e043a1a482199c5bb920f50311)
   * [x] prefer uplifting artifact deps that were written into the `deps` directory, but prefer to do that in [this PR instead](#9992)
   * [x] add extra-tests as described in Josh's comment: " features get unified between a Rust library and a binary, and one that confirms features don't get unified between a Rust library and a binary for a different target?"
   * [x] Make target-based artifact splitting work by porting parts of RFC-3176
       * [x] test-support/cross-compile
       * [x] namespace separation
* [x] re-create RFC-3176 what's not in RFC-3028, namely multidep support and all related tests
* [x] Address Eh2406 's review comments
* [x] Address Eric's comments
   * [x] Unstable features need to be documented in unstable.md.
   * [x] sort out [`target_data`](#9992 (comment))
   * [x] figure out [cargo metadata](#9992 (comment))
   * [x] See if target-data can work without an [index format update](#9992 (comment)).
   * [x] truncate comments at 80-90 lines and remove unused methods, remove -Z unstable-options, use `cfg!(target_env = "msvc")`
   * [x] add missing doc comments to newly added methods and funtions
   * [x] simplify method parameters and inline some functions
   * [x] add test and extend test to show additional issues
* [x] assure current set of tests works consistently, also on windows

Sponsored by [Profian](https://www.profian.com)

crates/cargo-test-support/src/cross_compile.rs

@@ -191,6 +191,7 @@ pub fn native_arch() -> &'static str {
         .expect("Target triple has unexpected format")
     {
         "x86_64" => "x86_64",
+        "aarch64" => "aarch64",
         "i686" => "x86",
         _ => panic!("This test should be gated on cross_compile::disabled."),
     }
@@ -200,7 +201,9 @@ pub fn native_arch() -> &'static str {
 ///
 /// Only use this function on tests that check `cross_compile::disabled`.
 pub fn alternate() -> &'static str {
-    if cfg!(target_os = "macos") {
+    if cfg!(all(target_os = "macos", target_arch = "aarch64")) {
+        "x86_64-apple-darwin"
+    } else if cfg!(target_os = "macos") {
         "x86_64-apple-ios"
     } else if cfg!(target_os = "linux") {
         "i686-unknown-linux-gnu"

crates/cargo-test-support/src/registry.rs

@@ -331,6 +331,7 @@ pub struct Dependency {
     name: String,
     vers: String,
     kind: String,
+    artifact: Option<(String, Option<String>)>,
     target: Option<String>,
     features: Vec<String>,
     registry: Option<String>,
@@ -591,6 +592,7 @@ impl Package {
                     "features": dep.features,
                     "default_features": true,
                     "target": dep.target,
+                    "artifact": dep.artifact,
                     "optional": dep.optional,
                     "kind": dep.kind,
                     "registry": registry_url,
@@ -744,6 +746,12 @@ impl Package {
             "#,
                 target, kind, dep.name, dep.vers
             ));
+            if let Some((artifact, target)) = &dep.artifact {
+                manifest.push_str(&format!("artifact = \"{}\"\n", artifact));
+                if let Some(target) = &target {
+                    manifest.push_str(&format!("target = \"{}\"\n", target))
+                }
+            }
             if let Some(registry) = &dep.registry {
                 assert_eq!(registry, "alternative");
                 manifest.push_str(&format!("registry-index = \"{}\"", alt_registry_url()));
@@ -799,6 +807,7 @@ impl Dependency {
             name: name.to_string(),
             vers: vers.to_string(),
             kind: "normal".to_string(),
+            artifact: None,
             target: None,
             features: Vec::new(),
             package: None,
@@ -825,6 +834,13 @@ impl Dependency {
         self
     }
 
+    /// Change the artifact to be of the given kind, like "bin", or "staticlib",
+    /// along with a specific target triple if provided.
+    pub fn artifact(&mut self, kind: &str, target: Option<String>) -> &mut Self {
+        self.artifact = Some((kind.to_string(), target));
+        self
+    }
+
     /// Adds `registry = $registry` to this dependency.
     pub fn registry(&mut self, registry: &str) -> &mut Self {
         self.registry = Some(registry.to_string());

src/cargo/core/compiler/artifact.rs

@@ -0,0 +1,57 @@
+/// Generate artifact information from unit dependencies for configuring the compiler environment.
+use crate::core::compiler::unit_graph::UnitDep;
+use crate::core::compiler::{Context, CrateType, FileFlavor, Unit};
+use crate::core::TargetKind;
+use crate::CargoResult;
+use std::collections::HashMap;
+use std::ffi::OsString;
+
+/// Return all environment variables for the given unit-dependencies
+/// if artifacts are present.
+pub fn get_env(
+    cx: &Context<'_, '_>,
+    dependencies: &[UnitDep],
+) -> CargoResult<HashMap<String, OsString>> {
+    let mut env = HashMap::new();
+    for unit_dep in dependencies.iter().filter(|d| d.unit.artifact.is_true()) {
+        for artifact_path in cx
+            .outputs(&unit_dep.unit)?
+            .iter()
+            .filter_map(|f| (f.flavor == FileFlavor::Normal).then(|| &f.path))
+        {
+            let artifact_type_upper = unit_artifact_type_name_upper(&unit_dep.unit);
+            let dep_name = unit_dep.dep_name.unwrap_or(unit_dep.unit.pkg.name());
+            let dep_name_upper = dep_name.to_uppercase().replace("-", "_");
+
+            let var = format!("CARGO_{}_DIR_{}", artifact_type_upper, dep_name_upper);
+            let path = artifact_path.parent().expect("parent dir for artifacts");
+            env.insert(var, path.to_owned().into());
+
+            let var = 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() {
+                let var = format!("CARGO_{}_FILE_{}", artifact_type_upper, dep_name_upper,);
+                env.insert(var, artifact_path.to_owned().into());
+            }
+        }
+    }
+    Ok(env)
+}
+
+fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str {
+    match unit.target.kind() {
+        TargetKind::Lib(kinds) => match kinds.as_slice() {
+            &[CrateType::Cdylib] => "CDYLIB",
+            &[CrateType::Staticlib] => "STATICLIB",
+            invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid),
+        },
+        TargetKind::Bin => "BIN",
+        invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid),
+    }
+}

src/cargo/core/compiler/build_context/target_info.rs

@@ -1,7 +1,7 @@
 use crate::core::compiler::{
     BuildOutput, CompileKind, CompileMode, CompileTarget, Context, CrateType,
 };
-use crate::core::{Dependency, Target, TargetKind, Workspace};
+use crate::core::{Dependency, Package, Target, TargetKind, Workspace};
 use crate::util::config::{Config, StringList, TargetConfig};
 use crate::util::{CargoResult, Rustc};
 use anyhow::Context as _;
@@ -748,11 +748,17 @@ impl<'cfg> RustcTargetData<'cfg> {
         // Get all kinds we currently know about.
         //
         // For now, targets can only ever come from the root workspace
-        // units as artifact dependencies are not a thing yet, so this
-        // correctly represents all the kinds that can happen. When we
-        // have artifact dependencies or other ways for targets to
-        // appear at places that are not the root units, we may have
-        // to revisit this.
+        // units and artifact dependencies, so this
+        // correctly represents all the kinds that can happen. When we have
+        // other ways for targets to appear at places that are not the root units,
+        // we may have to revisit this.
+        fn artifact_targets(package: &Package) -> impl Iterator<Item = CompileKind> + '_ {
+            package
+                .manifest()
+                .dependencies()
+                .iter()
+                .filter_map(|d| d.artifact()?.target()?.to_compile_kind())
+        }
         let all_kinds = requested_kinds
             .iter()
             .copied()
@@ -761,25 +767,32 @@ impl<'cfg> RustcTargetData<'cfg> {
                     .default_kind()
                     .into_iter()
                     .chain(p.manifest().forced_kind())
+                    .chain(artifact_targets(p))
             }));
         for kind in all_kinds {
-            if let CompileKind::Target(target) = kind {
-                if !res.target_config.contains_key(&target) {
-                    res.target_config
-                        .insert(target, res.config.target_cfg_triple(target.short_name())?);
-                }
-                if !res.target_info.contains_key(&target) {
-                    res.target_info.insert(
-                        target,
-                        TargetInfo::new(res.config, &res.requested_kinds, &res.rustc, kind)?,
-                    );
-                }
-            }
+            res.merge_compile_kind(kind)?;
         }
 
         Ok(res)
     }
 
+    /// Insert `kind` into our `target_info` and `target_config` members if it isn't present yet.
+    fn merge_compile_kind(&mut self, kind: CompileKind) -> CargoResult<()> {
+        if let CompileKind::Target(target) = kind {
+            if !self.target_config.contains_key(&target) {
+                self.target_config
+                    .insert(target, self.config.target_cfg_triple(target.short_name())?);
+            }
+            if !self.target_info.contains_key(&target) {
+                self.target_info.insert(
+                    target,
+                    TargetInfo::new(self.config, &self.requested_kinds, &self.rustc, kind)?,
+                );
+            }
+        }
+        Ok(())
+    }
+
     /// Returns a "short" name for the given kind, suitable for keying off
     /// configuration in Cargo or presenting to users.
     pub fn short_name<'a>(&'a self, kind: &'a CompileKind) -> &'a str {

src/cargo/core/compiler/compilation.rs

@@ -25,6 +25,9 @@ pub struct Doctest {
     ///
     /// This is used for indexing [`Compilation::extra_env`].
     pub script_meta: Option<Metadata>,
+
+    /// Environment variables to set in the rustdoc process.
+    pub env: HashMap<String, OsString>,
 }
 
 /// Information about the output of a unit.
@@ -190,14 +193,14 @@ impl<'cfg> Compilation<'cfg> {
     ) -> CargoResult<ProcessBuilder> {
         let rustdoc = ProcessBuilder::new(&*self.config.rustdoc()?);
         let cmd = fill_rustc_tool_env(rustdoc, unit);
-        let mut p = self.fill_env(cmd, &unit.pkg, script_meta, unit.kind, true)?;
-        unit.target.edition().cmd_edition_arg(&mut p);
+        let mut cmd = self.fill_env(cmd, &unit.pkg, script_meta, unit.kind, true)?;
+        unit.target.edition().cmd_edition_arg(&mut cmd);
 
         for crate_type in unit.target.rustc_crate_types() {
-            p.arg("--crate-type").arg(crate_type.as_str());
+            cmd.arg("--crate-type").arg(crate_type.as_str());
         }
 
-        Ok(p)
+        Ok(cmd)
     }
 
     /// Returns a [`ProcessBuilder`] appropriate for running a process for the

src/cargo/core/compiler/compile_kind.rs

@@ -158,8 +158,8 @@ impl CompileTarget {
     /// Typically this is pretty much the same as `short_name`, but for the case
     /// of JSON target files this will be a full canonicalized path name for the
     /// current filesystem.
-    pub fn rustc_target(&self) -> &str {
-        &self.name
+    pub fn rustc_target(&self) -> InternedString {
+        self.name
     }
 
     /// Returns a "short" version of the target name suitable for usage within

src/cargo/core/compiler/context/compilation_files.rs

@@ -201,6 +201,8 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
             self.build_script_dir(unit)
         } else if unit.target.is_example() {
             self.layout(unit.kind).examples().to_path_buf()
+        } else if unit.artifact.is_true() {
+            self.artifact_dir(unit)
         } else {
             self.deps_dir(unit).to_path_buf()
         }
@@ -287,6 +289,30 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         self.layout(CompileKind::Host).build().join(dir)
     }
 
+    /// Returns the directory for compiled artifacts files.
+    /// `/path/to/target/{debug,release}/deps/artifact/KIND/PKG-HASH`
+    fn artifact_dir(&self, unit: &Unit) -> PathBuf {
+        assert!(self.metas.contains_key(unit));
+        assert!(unit.artifact.is_true());
+        let dir = self.pkg_dir(unit);
+        let kind = match unit.target.kind() {
+            TargetKind::Bin => "bin",
+            TargetKind::Lib(lib_kinds) => match lib_kinds.as_slice() {
+                &[CrateType::Cdylib] => "cdylib",
+                &[CrateType::Staticlib] => "staticlib",
+                invalid => unreachable!(
+                    "BUG: unexpected artifact library type(s): {:?} - these should have been split",
+                    invalid
+                ),
+            },
+            invalid => unreachable!(
+                "BUG: {:?} are not supposed to be used as artifacts",
+                invalid
+            ),
+        };
+        self.layout(unit.kind).artifact().join(dir).join(kind)
+    }
+
     /// Returns the directory where information about running a build script
     /// is stored.
     /// `/path/to/target/{debug,release}/build/PKG-HASH`
@@ -354,7 +380,12 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         if unit.mode != CompileMode::Build || file_type.flavor == FileFlavor::Rmeta {
             return None;
         }
-        // Only uplift:
+
+        // Artifact dependencies are never uplifted.
+        if unit.artifact.is_true() {
+            return None;
+        }
+
         // - Binaries: The user always wants to see these, even if they are
         //   implicitly built (for example for integration tests).
         // - dylibs: This ensures that the dynamic linker pulls in all the

src/cargo/core/compiler/context/mod.rs

@@ -3,7 +3,7 @@ use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 
 use crate::core::compiler::compilation::{self, UnitOutput};
-use crate::core::compiler::{self, Unit};
+use crate::core::compiler::{self, artifact, Unit};
 use crate::core::PackageId;
 use crate::util::errors::CargoResult;
 use crate::util::profile;
@@ -133,7 +133,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
 
         // We need to make sure that if there were any previous docs
         // already compiled, they were compiled with the same Rustc version that we're currently
-        // using. Otherways we must remove the `doc/` folder and compile again forcing a rebuild.
+        // using. Otherwise we must remove the `doc/` folder and compile again forcing a rebuild.
         //
         // This is important because the `.js`/`.html` & `.css` files that are generated by Rustc don't have
         // any versioning (See https://github.com/rust-lang/cargo/issues/8461).
@@ -262,6 +262,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     unstable_opts,
                     linker: self.bcx.linker(unit.kind),
                     script_meta,
+                    env: artifact::get_env(&self, self.unit_deps(unit))?,
                 });
             }
 

src/cargo/core/compiler/custom_build.rs

@@ -1,5 +1,6 @@
 use super::job::{Freshness, Job, Work};
 use super::{fingerprint, Context, LinkType, Unit};
+use crate::core::compiler::artifact;
 use crate::core::compiler::context::Metadata;
 use crate::core::compiler::job_queue::JobState;
 use crate::core::{profiles::ProfileRoot, PackageId, Target};
@@ -203,6 +204,11 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
         .env("RUSTDOC", &*bcx.config.rustdoc()?)
         .inherit_jobserver(&cx.jobserver);
 
+    // Find all artifact dependencies and make their file and containing directory discoverable using environment variables.
+    for (var, value) in artifact::get_env(cx, dependencies)? {
+        cmd.env(&var, value);
+    }
+
     if let Some(linker) = &bcx.target_data.target_config(unit.kind).linker {
         cmd.env(
             "RUSTC_LINKER",

src/cargo/core/compiler/job_queue.rs

@@ -395,7 +395,8 @@ impl<'cfg> JobQueue<'cfg> {
             .filter(|dep| {
                 // Binaries aren't actually needed to *compile* tests, just to run
                 // them, so we don't include this dependency edge in the job graph.
-                !dep.unit.target.is_test() && !dep.unit.target.is_bin()
+                (!dep.unit.target.is_test() && !dep.unit.target.is_bin())
+                    || dep.unit.artifact.is_true()
             })
             .map(|dep| {
                 // Handle the case here where our `unit -> dep` dependency may

src/cargo/core/compiler/layout.rs

@@ -47,6 +47,9 @@
 //!         # prevent collisions. One notable exception is dynamic libraries.
 //!         deps/
 //!
+//!             # Each artifact dependency gets in its own directory.
+//!             /artifact/$pkgname-$META/$kind
+//!
 //!         # Root directory for all compiled examples.
 //!         examples/
 //!
@@ -117,6 +120,8 @@ pub struct Layout {
     deps: PathBuf,
     /// The directory for build scripts: `$dest/build`
     build: PathBuf,
+    /// The directory for artifacts, i.e. binaries, cdylibs, staticlibs: `$dest/deps/artifact`
+    artifact: PathBuf,
     /// The directory for incremental files: `$dest/incremental`
     incremental: PathBuf,
     /// The directory for fingerprints: `$dest/.fingerprint`
@@ -164,10 +169,13 @@ impl Layout {
         let lock = dest.open_rw(".cargo-lock", ws.config(), "build directory")?;
         let root = root.into_path_unlocked();
         let dest = dest.into_path_unlocked();
+        let deps = dest.join("deps");
+        let artifact = deps.join("artifact");
 
         Ok(Layout {
-            deps: dest.join("deps"),
+            deps,
             build: dest.join("build"),
+            artifact,
             incremental: dest.join("incremental"),
             fingerprint: dest.join(".fingerprint"),
             examples: dest.join("examples"),
@@ -222,6 +230,10 @@ impl Layout {
     pub fn build(&self) -> &Path {
         &self.build
     }
+    /// Fetch the artifact path.
+    pub fn artifact(&self) -> &Path {
+        &self.artifact
+    }
     /// Create and return the tmp path.
     pub fn prepare_tmp(&self) -> CargoResult<&Path> {
         paths::create_dir_all(&self.tmp)?;