From 528ef7e13468fec97a0b838168a3d3a6b69864dc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 09:32:54 -0600 Subject: [PATCH] refactor(fingerprint): Shift focus from Metadata to MetaInfo Now called `UnitHash` and `Metadata`. The goal is to provide role-specific `UnitHash` accessors on the new `Metadata`. I chose this method, instead of changing the accessors on `CompilationFiles`, to have the least impact on the documentation as what is called `Metadata` can remain the focus of the documentation, rather than having it apply to different `CompilationFiles` accessors which don't house the actual logic. --- .../build_runner/compilation_files.rs | 83 +++++++++++-------- src/cargo/core/compiler/build_runner/mod.rs | 10 +-- src/cargo/core/compiler/compilation.rs | 14 ++-- src/cargo/core/compiler/custom_build.rs | 24 +++--- src/cargo/core/compiler/job_queue/mod.rs | 2 +- src/cargo/core/compiler/mod.rs | 21 +++-- src/cargo/ops/cargo_test.rs | 4 +- 7 files changed, 86 insertions(+), 72 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 7047c142792..d0a404fb189 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -25,6 +25,22 @@ use crate::util::{self, CargoResult, StableHasher}; /// use the same rustc version. const METADATA_VERSION: u8 = 2; +/// Uniquely identify a [`Unit`] under specific circumstances, see [`Metadata`] for more. +#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] +pub struct UnitHash(u64); + +impl fmt::Display for UnitHash { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:016x}", self.0) + } +} + +impl fmt::Debug for UnitHash { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "UnitHash({:016x})", self.0) + } +} + /// The `Metadata` is a hash used to make unique file names for each unit in a /// build. It is also used for symbol mangling. /// @@ -68,30 +84,25 @@ const METADATA_VERSION: u8 = 2; /// /// Note that the `Fingerprint` is in charge of tracking everything needed to determine if a /// rebuild is needed. -#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] -pub struct Metadata(u64); - -impl fmt::Display for Metadata { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:016x}", self.0) - } +#[derive(Copy, Clone, Debug)] +pub struct Metadata { + meta_hash: UnitHash, + use_extra_filename: bool, } -impl fmt::Debug for Metadata { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Metadata({:016x})", self.0) +impl Metadata { + /// The symbol hash to use. + pub fn meta_hash(&self) -> UnitHash { + self.meta_hash } -} -/// Information about the metadata hashes used for a `Unit`. -struct MetaInfo { - /// The symbol hash to use. - meta_hash: Metadata, /// Whether or not the `-C extra-filename` flag is used to generate unique /// output filenames for this `Unit`. /// /// If this is `true`, the `meta_hash` is used for the filename. - use_extra_filename: bool, + pub fn use_extra_filename(&self) -> bool { + self.use_extra_filename + } } /// Collection of information about the files emitted by the compiler, and the @@ -108,7 +119,7 @@ pub struct CompilationFiles<'a, 'gctx> { roots: Vec, ws: &'a Workspace<'gctx>, /// Metadata hash to use for each unit. - metas: HashMap, + metas: HashMap, /// For each Unit, a list all files produced. outputs: HashMap>>>, } @@ -177,13 +188,7 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { /// /// [`fingerprint`]: super::super::fingerprint#fingerprints-and-metadata pub fn metadata(&self, unit: &Unit) -> Metadata { - self.metas[unit].meta_hash - } - - /// Returns whether or not `-C extra-filename` is used to extend the - /// output filenames to make them unique. - pub fn use_extra_filename(&self, unit: &Unit) -> bool { - self.metas[unit].use_extra_filename + self.metas[unit] } /// Gets the short hash based only on the `PackageId`. @@ -224,9 +229,9 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { /// taken in those cases! fn pkg_dir(&self, unit: &Unit) -> String { let name = unit.pkg.package_id().name(); - let meta = &self.metas[unit]; - if meta.use_extra_filename { - format!("{}-{}", name, meta.meta_hash) + let meta = self.metas[unit]; + if meta.use_extra_filename() { + format!("{}-{}", name, meta.meta_hash()) } else { format!("{}-{}", name, self.target_short_hash(unit)) } @@ -467,7 +472,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { // The file name needs to be stable across Cargo sessions. // This originally used unit.buildkey(), but that isn't stable, // so we use metadata instead (prefixed with name for debugging). - let file_name = format!("{}-{}.examples", unit.pkg.name(), self.metadata(unit)); + let file_name = format!( + "{}-{}.examples", + unit.pkg.name(), + self.metadata(unit).meta_hash() + ); let path = self.deps_dir(unit).join(file_name); vec![OutputFile { path, @@ -523,8 +532,10 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { // Convert FileType to OutputFile. let mut outputs = Vec::new(); for file_type in file_types { - let meta = &self.metas[unit]; - let meta_opt = meta.use_extra_filename.then(|| meta.meta_hash.to_string()); + let meta = self.metas[unit]; + let meta_opt = meta + .use_extra_filename() + .then(|| meta.meta_hash().to_string()); let path = out_dir.join(file_type.output_filename(&unit.target, meta_opt.as_deref())); // If, the `different_binary_name` feature is enabled, the name of the hardlink will @@ -558,8 +569,8 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { fn metadata_of<'a>( unit: &Unit, build_runner: &BuildRunner<'_, '_>, - metas: &'a mut HashMap, -) -> &'a MetaInfo { + metas: &'a mut HashMap, +) -> &'a Metadata { if !metas.contains_key(unit) { let meta = compute_metadata(unit, build_runner, metas); metas.insert(unit.clone(), meta); @@ -574,8 +585,8 @@ fn metadata_of<'a>( fn compute_metadata( unit: &Unit, build_runner: &BuildRunner<'_, '_>, - metas: &mut HashMap, -) -> MetaInfo { + metas: &mut HashMap, +) -> Metadata { let bcx = &build_runner.bcx; let mut hasher = StableHasher::new(); @@ -669,8 +680,8 @@ fn compute_metadata( target_configs_are_different.hash(&mut hasher); } - MetaInfo { - meta_hash: Metadata(hasher.finish()), + Metadata { + meta_hash: UnitHash(hasher.finish()), use_extra_filename: use_extra_filename(bcx, unit), } } diff --git a/src/cargo/core/compiler/build_runner/mod.rs b/src/cargo/core/compiler/build_runner/mod.rs index af8c0a7e4a1..e7a79a55a7d 100644 --- a/src/cargo/core/compiler/build_runner/mod.rs +++ b/src/cargo/core/compiler/build_runner/mod.rs @@ -27,7 +27,7 @@ use super::{ mod compilation_files; use self::compilation_files::CompilationFiles; -pub use self::compilation_files::{Metadata, OutputFile}; +pub use self::compilation_files::{Metadata, OutputFile, UnitHash}; /// Collection of all the stuff that is needed to perform a build. /// @@ -86,7 +86,7 @@ pub struct BuildRunner<'a, 'gctx> { /// Set of metadata of Docscrape units that fail before completion, e.g. /// because the target has a type error. This is in an Arc> /// because it is continuously updated as the job progresses. - pub failed_scrape_units: Arc>>, + pub failed_scrape_units: Arc>>, } impl<'a, 'gctx> BuildRunner<'a, 'gctx> { @@ -435,15 +435,15 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { /// the given unit. /// /// If the package does not have a build script, this returns None. - pub fn find_build_script_metadata(&self, unit: &Unit) -> Option { + pub fn find_build_script_metadata(&self, unit: &Unit) -> Option { let script_unit = self.find_build_script_unit(unit)?; Some(self.get_run_build_script_metadata(&script_unit)) } /// Returns the metadata hash for a `RunCustomBuild` unit. - pub fn get_run_build_script_metadata(&self, unit: &Unit) -> Metadata { + pub fn get_run_build_script_metadata(&self, unit: &Unit) -> UnitHash { assert!(unit.mode.is_run_custom_build()); - self.files().metadata(unit) + self.files().metadata(unit).meta_hash() } pub fn is_primary_package(&self, unit: &Unit) -> bool { diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 372a0a5694d..c31ee9ee4eb 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -9,7 +9,7 @@ use cargo_util::{paths, ProcessBuilder}; use crate::core::compiler::apply_env_config; use crate::core::compiler::BuildContext; -use crate::core::compiler::{CompileKind, Metadata, Unit}; +use crate::core::compiler::{CompileKind, Unit, UnitHash}; use crate::core::Package; use crate::util::{context, CargoResult, GlobalContext}; @@ -45,7 +45,7 @@ pub struct Doctest { /// The script metadata, if this unit's package has a build script. /// /// This is used for indexing [`Compilation::extra_env`]. - pub script_meta: Option, + pub script_meta: Option, /// Environment variables to set in the rustdoc process. pub env: HashMap, @@ -61,7 +61,7 @@ pub struct UnitOutput { /// The script metadata, if this unit's package has a build script. /// /// This is used for indexing [`Compilation::extra_env`]. - pub script_meta: Option, + pub script_meta: Option, } /// A structure returning the result of a compilation. @@ -101,7 +101,7 @@ pub struct Compilation<'gctx> { /// /// The key is the build script metadata for uniquely identifying the /// `RunCustomBuild` unit that generated these env vars. - pub extra_env: HashMap>, + pub extra_env: HashMap>, /// Libraries to test with rustdoc. pub to_doc_test: Vec, @@ -197,7 +197,7 @@ impl<'gctx> Compilation<'gctx> { pub fn rustdoc_process( &self, unit: &Unit, - script_meta: Option, + script_meta: Option, ) -> CargoResult { let mut rustdoc = ProcessBuilder::new(&*self.gctx.rustdoc()?); if self.gctx.extra_verbose() { @@ -256,7 +256,7 @@ impl<'gctx> Compilation<'gctx> { cmd: T, kind: CompileKind, pkg: &Package, - script_meta: Option, + script_meta: Option, ) -> CargoResult { let builder = if let Some((runner, args)) = self.target_runner(kind) { let mut builder = ProcessBuilder::new(runner); @@ -285,7 +285,7 @@ impl<'gctx> Compilation<'gctx> { &self, mut cmd: ProcessBuilder, pkg: &Package, - script_meta: Option, + script_meta: Option, kind: CompileKind, tool_kind: ToolKind, ) -> CargoResult { diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index fce9c96d3bf..95126f1f2e9 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -33,7 +33,7 @@ use super::{fingerprint, BuildRunner, Job, Unit, Work}; use crate::core::compiler::artifact; -use crate::core::compiler::build_runner::Metadata; +use crate::core::compiler::build_runner::UnitHash; use crate::core::compiler::fingerprint::DirtyReason; use crate::core::compiler::job_queue::JobState; use crate::core::{profiles::ProfileRoot, PackageId, Target}; @@ -111,13 +111,13 @@ pub struct BuildOutput { /// inserted during `build_map`. The rest of the entries are added /// immediately after each build script runs. /// -/// The `Metadata` is the unique metadata hash for the `RunCustomBuild` Unit of +/// The [`UnitHash`] is the unique metadata hash for the `RunCustomBuild` Unit of /// the package. It needs a unique key, since the build script can be run /// multiple times with different profiles or features. We can't embed a /// `Unit` because this structure needs to be shareable between threads. #[derive(Default)] pub struct BuildScriptOutputs { - outputs: HashMap, + outputs: HashMap, } /// Linking information for a `Unit`. @@ -141,9 +141,9 @@ pub struct BuildScripts { /// usage here doesn't blow up too much. /// /// For more information, see #2354. - pub to_link: Vec<(PackageId, Metadata)>, + pub to_link: Vec<(PackageId, UnitHash)>, /// This is only used while constructing `to_link` to avoid duplicates. - seen_to_link: HashSet<(PackageId, Metadata)>, + seen_to_link: HashSet<(PackageId, UnitHash)>, /// Host-only dependencies that have build scripts. Each element is an /// index into `BuildScriptOutputs`. /// @@ -152,7 +152,7 @@ pub struct BuildScripts { /// Any `BuildOutput::library_paths` path relative to `target` will be /// added to `LD_LIBRARY_PATH` so that the compiler can find any dynamic /// libraries a build script may have generated. - pub plugins: BTreeSet<(PackageId, Metadata)>, + pub plugins: BTreeSet<(PackageId, UnitHash)>, } /// Dependency information as declared by a build script that might trigger @@ -649,7 +649,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul fn insert_log_messages_in_build_outputs( build_script_outputs: Arc>, id: PackageId, - metadata_hash: Metadata, + metadata_hash: UnitHash, log_messages: Vec, ) { let build_output_with_only_log_messages = BuildOutput { @@ -1245,7 +1245,7 @@ pub fn build_map(build_runner: &mut BuildRunner<'_, '_>) -> CargoResult<()> { // When adding an entry to 'to_link' we only actually push it on if the // script hasn't seen it yet (e.g., we don't push on duplicates). - fn add_to_link(scripts: &mut BuildScripts, pkg: PackageId, metadata: Metadata) { + fn add_to_link(scripts: &mut BuildScripts, pkg: PackageId, metadata: UnitHash) { if scripts.seen_to_link.insert((pkg, metadata)) { scripts.to_link.push((pkg, metadata)); } @@ -1297,7 +1297,7 @@ fn prev_build_output( impl BuildScriptOutputs { /// Inserts a new entry into the map. - fn insert(&mut self, pkg_id: PackageId, metadata: Metadata, parsed_output: BuildOutput) { + fn insert(&mut self, pkg_id: PackageId, metadata: UnitHash, parsed_output: BuildOutput) { match self.outputs.entry(metadata) { Entry::Vacant(entry) => { entry.insert(parsed_output); @@ -1314,17 +1314,17 @@ impl BuildScriptOutputs { } /// Returns `true` if the given key already exists. - fn contains_key(&self, metadata: Metadata) -> bool { + fn contains_key(&self, metadata: UnitHash) -> bool { self.outputs.contains_key(&metadata) } /// Gets the build output for the given key. - pub fn get(&self, meta: Metadata) -> Option<&BuildOutput> { + pub fn get(&self, meta: UnitHash) -> Option<&BuildOutput> { self.outputs.get(&meta) } /// Returns an iterator over all entries. - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator { self.outputs.iter() } } diff --git a/src/cargo/core/compiler/job_queue/mod.rs b/src/cargo/core/compiler/job_queue/mod.rs index c8df3f2be02..6f4c66fcd98 100644 --- a/src/cargo/core/compiler/job_queue/mod.rs +++ b/src/cargo/core/compiler/job_queue/mod.rs @@ -685,7 +685,7 @@ impl<'gctx> DrainState<'gctx> { .failed_scrape_units .lock() .unwrap() - .insert(build_runner.files().metadata(&unit)); + .insert(build_runner.files().metadata(&unit).meta_hash()); self.queue.finish(&unit, &artifact); } Err(error) => { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 61d88a68383..599056dad21 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -73,7 +73,7 @@ pub use self::build_context::{ BuildContext, FileFlavor, FileType, RustDocFingerprint, RustcTargetData, TargetInfo, }; use self::build_plan::BuildPlan; -pub use self::build_runner::{BuildRunner, Metadata}; +pub use self::build_runner::{BuildRunner, Metadata, UnitHash}; pub use self::compilation::{Compilation, Doctest, UnitOutput}; pub use self::compile_kind::{CompileKind, CompileTarget}; pub use self::crate_type::CrateType; @@ -279,11 +279,11 @@ fn rustc( // don't pass the `-l` flags. let pass_l_flag = unit.target.is_lib() || !unit.pkg.targets().iter().any(|t| t.is_lib()); - let dep_info_name = if build_runner.files().use_extra_filename(unit) { + let dep_info_name = if build_runner.files().metadata(unit).use_extra_filename() { format!( "{}-{}.d", unit.target.crate_name(), - build_runner.files().metadata(unit) + build_runner.files().metadata(unit).meta_hash() ) } else { format!("{}.d", unit.target.crate_name()) @@ -764,7 +764,9 @@ fn prepare_rustdoc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResu } let metadata = build_runner.metadata_for_doc_units[unit]; - rustdoc.arg("-C").arg(format!("metadata={}", metadata)); + rustdoc + .arg("-C") + .arg(format!("metadata={}", metadata.meta_hash())); if unit.mode.is_doc_scrape() { debug_assert!(build_runner.bcx.scrape_units.contains(unit)); @@ -842,7 +844,7 @@ fn rustdoc(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResult, + metadata: Option, ) -> CargoResult<()> { if let Some(metadata) = metadata { if let Some(output) = build_script_outputs.get(metadata) { diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index 1144b31c63c..c3e4eec860f 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/src/cargo/ops/cargo_test.rs @@ -1,4 +1,4 @@ -use crate::core::compiler::{Compilation, CompileKind, Doctest, Metadata, Unit, UnitOutput}; +use crate::core::compiler::{Compilation, CompileKind, Doctest, Unit, UnitHash, UnitOutput}; use crate::core::profiles::PanicStrategy; use crate::core::shell::ColorChoice; use crate::core::shell::Verbosity; @@ -355,7 +355,7 @@ fn cmd_builds( cwd: &Path, unit: &Unit, path: &PathBuf, - script_meta: &Option, + script_meta: &Option, test_args: &[&str], compilation: &Compilation<'_>, exec_type: &str,