Auto merge of #3102 - nipunn1313:attempt, r=alexcrichton

Mix feature flags into fingerprint/metadata shorthash

Since building dependencies results in different libraries
depending on the feature flags, I added the feature flags into
the short_hash.

This solves an issue when multiple crates share a target directory
or multiple targets share a common library with divergent feature
flag choice.

I'm not sure if this architecturally the best way to solve this problem, but I did confirm that this fixes the issue I was seeing. I can also add a test for this case if this code is taking the right approach (or if it would help illustrate the issue).

Author: bors

src/cargo/core/manifest.rs

@@ -395,6 +395,13 @@ impl Target {
         }
     }
 
+    pub fn is_dylib(&self) -> bool {
+        match self.kind {
+            TargetKind::Lib(ref libs) => libs.iter().any(|l| *l == LibKind::Dylib),
+            _ => false
+        }
+    }
+
     pub fn linkable(&self) -> bool {
         match self.kind {
             TargetKind::Lib(ref kinds) => {

src/cargo/ops/cargo_clean.rs

@@ -77,9 +77,11 @@ pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> {
         rm_rf(&layout.proxy().fingerprint(&unit.pkg))?;
         rm_rf(&layout.build(&unit.pkg))?;
 
-        let root = cx.out_dir(&unit);
-        for (filename, _) in cx.target_filenames(&unit)? {
-            rm_rf(&root.join(&filename))?;
+        for (src, link_dst, _) in cx.target_filenames(&unit)? {
+            rm_rf(&src)?;
+            if let Some(dst) = link_dst {
+                rm_rf(&dst)?;
+            }
         }
     }
 

src/cargo/ops/cargo_rustc/context.rs

@@ -308,71 +308,144 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     }
 
     /// Get the metadata for a target in a specific profile
+    /// We build to the path: "{filename}-{target_metadata}"
+    /// We use a linking step to link/copy to a predictable filename
+    /// like `target/debug/libfoo.{a,so,rlib}` and such.
     pub fn target_metadata(&self, unit: &Unit) -> Option<Metadata> {
-        let metadata = unit.target.metadata();
+        // No metadata for dylibs because of a couple issues
+        // - OSX encodes the dylib name in the executable
+        // - Windows rustc multiple files of which we can't easily link all of them
+        //
+        // Two expeptions
+        // 1) Upstream dependencies (we aren't exporting + need to resolve name conflict)
+        // 2) __CARGO_DEFAULT_LIB_METADATA env var
+        //
+        // Note, though, that the compiler's build system at least wants
+        // path dependencies (eg libstd) to have hashes in filenames. To account for
+        // that we have an extra hack here which reads the
+        // `__CARGO_DEFAULT_METADATA` environment variable and creates a
+        // hash in the filename if that's present.
+        //
+        // This environment variable should not be relied on! It's
+        // just here for rustbuild. We need a more principled method
+        // doing this eventually.
+        if !unit.profile.test &&
+            unit.target.is_dylib() &&
+            unit.pkg.package_id().source_id().is_path() &&
+            !env::var("__CARGO_DEFAULT_LIB_METADATA").is_ok() {
+            return None;
+        }
+
+        let metadata = unit.target.metadata().cloned().map(|mut m| {
+            if let Some(features) = self.resolve.features(unit.pkg.package_id()) {
+                let mut feat_vec: Vec<&String> = features.iter().collect();
+                feat_vec.sort();
+                for feat in feat_vec {
+                    m.mix(feat);
+                }
+            }
+            m.mix(unit.profile);
+            m
+        });
+        let mut pkg_metadata = {
+            let mut m = unit.pkg.generate_metadata();
+            if let Some(features) = self.resolve.features(unit.pkg.package_id()) {
+                let mut feat_vec: Vec<&String> = features.iter().collect();
+                feat_vec.sort();
+                for feat in feat_vec {
+                    m.mix(feat);
+                }
+            }
+            m.mix(unit.profile);
+            m
+        };
+
         if unit.target.is_lib() && unit.profile.test {
             // Libs and their tests are built in parallel, so we need to make
             // sure that their metadata is different.
-            metadata.cloned().map(|mut m| {
+            metadata.map(|mut m| {
                 m.mix(&"test");
                 m
             })
         } else if unit.target.is_bin() && unit.profile.test {
             // Make sure that the name of this test executable doesn't
             // conflict with a library that has the same name and is
             // being tested
-            let mut metadata = unit.pkg.generate_metadata();
-            metadata.mix(&format!("bin-{}", unit.target.name()));
-            Some(metadata)
+            pkg_metadata.mix(&format!("bin-{}", unit.target.name()));
+            Some(pkg_metadata)
         } else if unit.pkg.package_id().source_id().is_path() &&
                   !unit.profile.test {
-            // If we're not building a unit test but we're building a path
-            // dependency, then we're likely compiling the "current package" or
-            // some package in a workspace. In this situation we pass no
-            // metadata by default so we'll have predictable
-            // file names like `target/debug/libfoo.{a,so,rlib}` and such.
-            //
-            // Note, though, that the compiler's build system at least wants
-            // path dependencies to have hashes in filenames. To account for
-            // that we have an extra hack here which reads the
-            // `__CARGO_DEFAULT_METADATA` environment variable and creates a
-            // hash in the filename if that's present.
-            //
-            // This environment variable should not be relied on! It's basically
-            // just here for rustbuild. We need a more principled method of
-            // doing this eventually.
-            if unit.target.is_lib() {
-                env::var("__CARGO_DEFAULT_LIB_METADATA").ok().map(|meta| {
-                    let mut metadata = unit.pkg.generate_metadata();
-                    metadata.mix(&meta);
-                    metadata
-                })
-            } else {
-                None
-            }
+            Some(pkg_metadata)
         } else {
-            metadata.cloned()
+            metadata
         }
     }
 
-    /// Returns the file stem for a given target/profile combo
+    /// Returns the file stem for a given target/profile combo (with metadata)
     pub fn file_stem(&self, unit: &Unit) -> String {
         match self.target_metadata(unit) {
             Some(ref metadata) => format!("{}{}", unit.target.crate_name(),
                                           metadata.extra_filename),
-            None if unit.target.allows_underscores() => {
-                unit.target.name().to_string()
+            None => self.bin_stem(unit),
+        }
+    }
+
+    /// Returns the bin stem for a given target (without metadata)
+    fn bin_stem(&self, unit: &Unit) -> String {
+        if unit.target.allows_underscores() {
+            unit.target.name().to_string()
+        } else {
+            unit.target.crate_name()
+        }
+    }
+
+    /// Returns a tuple with the directory and name of the hard link we expect
+    /// our target to be copied to. Eg, file_stem may be out_dir/deps/foo-abcdef
+    /// and link_stem would be out_dir/foo
+    /// This function returns it in two parts so the caller can add prefix/suffis
+    /// to filename separately
+
+    /// Returns an Option because in some cases we don't want to link
+    /// (eg a dependent lib)
+    pub fn link_stem(&self, unit: &Unit) -> Option<(PathBuf, String)> {
+        let src_dir = self.out_dir(unit);
+        let bin_stem = self.bin_stem(unit);
+        let file_stem = self.file_stem(unit);
+
+        // We currently only lift files up from the `deps` directory. If
+        // it was compiled into something like `example/` or `doc/` then
+        // we don't want to link it up.
+        if src_dir.ends_with("deps") {
+            // Don't lift up library dependencies
+            if unit.pkg.package_id() != &self.current_package && !unit.target.is_bin() {
+                None
+            } else {
+                Some((
+                    src_dir.parent().unwrap().to_owned(),
+                    if unit.profile.test {file_stem} else {bin_stem},
+                ))
             }
-            None => unit.target.crate_name(),
+        } else if bin_stem == file_stem {
+            None
+        } else if src_dir.ends_with("examples") {
+            Some((src_dir, bin_stem))
+        } else if src_dir.parent().unwrap().ends_with("build") {
+            Some((src_dir, bin_stem))
+        } else {
+            None
         }
     }
 
     /// Return the filenames that the given target for the given profile will
-    /// generate, along with whether you can link against that file (e.g. it's a
-    /// library).
+    /// generate as a list of 3-tuples (filename, link_dst, linkable)
+    /// filename: filename rustc compiles to. (Often has metadata suffix).
+    /// link_dst: Optional file to link/copy the result to (without metadata suffix)
+    /// linkable: Whether possible to link against file (eg it's a library)
     pub fn target_filenames(&self, unit: &Unit)
-                            -> CargoResult<Vec<(String, bool)>> {
+                            -> CargoResult<Vec<(PathBuf, Option<PathBuf>, bool)>> {
+        let out_dir = self.out_dir(unit);
         let stem = self.file_stem(unit);
+        let link_stem = self.link_stem(unit);
         let info = if unit.target.for_host() {
             &self.host_info
         } else {
@@ -386,8 +459,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 let crate_type = if crate_type == "lib" {"rlib"} else {crate_type};
                 match info.crate_types.get(crate_type) {
                     Some(&Some((ref prefix, ref suffix))) => {
-                        ret.push((format!("{}{}{}", prefix, stem, suffix),
-                                  linkable));
+                        let filename = out_dir.join(format!("{}{}{}", prefix, stem, suffix));
+                        let link_dst = link_stem.clone().map(|(ld, ls)| {
+                            ld.join(format!("{}{}{}", prefix, ls, suffix))
+                        });
+                        ret.push((filename, link_dst, linkable));
                         Ok(())
                     }
                     // not supported, don't worry about it
@@ -429,6 +505,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                    support any of the output crate types",
                   unit.pkg, self.target_triple());
         }
+        info!("Target filenames: {:?}", ret);
         Ok(ret)
     }
 

src/cargo/ops/cargo_rustc/fingerprint.rs

@@ -49,7 +49,7 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
     let _p = profile::start(format!("fingerprint: {} / {}",
                                     unit.pkg.package_id(), unit.target.name()));
     let new = dir(cx, unit);
-    let loc = new.join(&filename(unit));
+    let loc = new.join(&filename(cx, unit));
 
     debug!("fingerprint at: {}", loc.display());
 
@@ -82,8 +82,11 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
         missing_outputs = !root.join(unit.target.crate_name())
                                .join("index.html").exists();
     } else {
-        for (filename, _) in cx.target_filenames(unit)? {
-            missing_outputs |= fs::metadata(root.join(filename)).is_err();
+        for (src, link_dst, _) in cx.target_filenames(unit)? {
+            missing_outputs |= !src.exists();
+            if let Some(link_dst) = link_dst {
+                missing_outputs |= !link_dst.exists();
+            }
         }
     }
 
@@ -529,7 +532,7 @@ pub fn dir(cx: &Context, unit: &Unit) -> PathBuf {
 
 /// Returns the (old, new) location for the dep info file of a target.
 pub fn dep_info_loc(cx: &Context, unit: &Unit) -> PathBuf {
-    dir(cx, unit).join(&format!("dep-{}", filename(unit)))
+    dir(cx, unit).join(&format!("dep-{}", filename(cx, unit)))
 }
 
 fn compare_old_fingerprint(loc: &Path, new_fingerprint: &Fingerprint)
@@ -650,7 +653,11 @@ fn mtime_if_fresh<I>(output: &Path, paths: I) -> Option<FileTime>
     }
 }
 
-fn filename(unit: &Unit) -> String {
+fn filename(cx: &Context, unit: &Unit) -> String {
+    // file_stem includes metadata hash. Thus we have a different
+    // fingerprint for every metadata hash version. This works because
+    // even if the package is fresh, we'll still link the fresh target
+    let file_stem = cx.file_stem(unit);
     let kind = match *unit.target.kind() {
         TargetKind::Lib(..) => "lib",
         TargetKind::Bin => "bin",
@@ -666,7 +673,7 @@ fn filename(unit: &Unit) -> String {
     } else {
         ""
     };
-    format!("{}{}-{}", flavor, kind, unit.target.name())
+    format!("{}{}-{}", flavor, kind, file_stem)
 }
 
 // The dep-info files emitted by the compiler all have their listed paths

src/cargo/ops/cargo_rustc/layout.rs

@@ -171,10 +171,8 @@ impl<'a> LayoutProxy<'a> {
             self.build(unit.pkg)
         } else if unit.target.is_example() {
             self.examples().to_path_buf()
-        } else if unit.target.is_lib() {
-            self.deps().to_path_buf()
         } else {
-            self.root().to_path_buf()
+            self.deps().to_path_buf()
         }
     }