Auto merge of #6817 - thomwiggers:fix-2748, r=ehuss

Handle symlinks to directories

When cargo encounters a link, check if it's a directory when considering it for further processing.
I'm not sure how this interacts with things such as submodules – it allowed me to publish the crate [pqcrypto-kyber768](https://github.com/rustpq/pqcrypto) which uses a link to a submodule.

Fixes #2748.

This PR:
* Add new tests that demonstrate that links to dirs can't be packaged
* Fixes those tests
* Enabled symlink tests to run on Windows
* Fix a bug in the handling of symlinks
* Add new test runner behaviour on Windows (it will ignore symlink-related tests and instead run them if you specify --ignored.)

Author: bors

src/cargo/core/features.rs

@@ -21,7 +21,7 @@
 //! 3. To actually perform the feature gate, you'll want to have code that looks
 //!    like:
 //!
-//! ```rust,ignore
+//! ```rust,compile_fail
 //! use core::{Feature, Features};
 //!
 //! let feature = Feature::launch_into_space();

src/cargo/sources/path.rs

@@ -219,9 +219,17 @@ impl<'cfg> PathSource<'cfg> {
         // the untracked files are often part of a build and may become relevant
         // as part of a future commit.
         let index_files = index.iter().map(|entry| {
-            use libgit2_sys::GIT_FILEMODE_COMMIT;
-            let is_dir = entry.mode == GIT_FILEMODE_COMMIT as u32;
-            (join(root, &entry.path), Some(is_dir))
+            use libgit2_sys::{GIT_FILEMODE_COMMIT, GIT_FILEMODE_LINK};
+            // ``is_dir`` is an optimization to avoid calling
+            // ``fs::metadata`` on every file.
+            let is_dir = if entry.mode == GIT_FILEMODE_LINK as u32 {
+                // Let the code below figure out if this symbolic link points
+                // to a directory or not.
+                None
+            } else {
+                Some(entry.mode == GIT_FILEMODE_COMMIT as u32)
+            };
+            (join(root, &entry.path), is_dir)
         });
         let mut opts = git2::StatusOptions::new();
         opts.include_untracked(true);

src/cargo/util/network.rs

@@ -73,9 +73,12 @@ fn maybe_spurious(err: &Error) -> bool {
 ///
 /// # Examples
 ///
-/// ```ignore
-/// use util::network;
-/// cargo_result = network::with_retry(&config, || something.download());
+/// ```
+/// # use crate::cargo::util::{CargoResult, Config};
+/// # let download_something = || return Ok(());
+/// # let config = Config::default().unwrap();
+/// use cargo::util::network;
+/// let cargo_result = network::with_retry(&config, || download_something());
 /// ```
 pub fn with_retry<T, F>(config: &Config, mut callback: F) -> CargoResult<T>
 where

tests/testsuite/build.rs

@@ -4,11 +4,10 @@ use std::io::prelude::*;
 
 use crate::support::paths::{root, CargoPathExt};
 use crate::support::registry::Package;
-use crate::support::ProjectBuilder;
 use crate::support::{
-    basic_bin_manifest, basic_lib_manifest, basic_manifest, rustc_host, sleep_ms,
+    basic_bin_manifest, basic_lib_manifest, basic_manifest, main_file, project, rustc_host,
+    sleep_ms, symlink_supported, Execs, ProjectBuilder,
 };
-use crate::support::{main_file, project, Execs};
 use cargo::util::paths::dylib_path_envvar;
 
 #[cargo_test]
@@ -1495,9 +1494,12 @@ package `test v0.0.0 ([CWD])`",
 }
 
 #[cargo_test]
+/// Make sure broken symlinks don't break the build
+///
+/// This test requires you to be able to make symlinks.
+/// For windows, this may require you to enable developer mode.
 fn ignore_broken_symlinks() {
-    // windows and symlinks don't currently agree that well
-    if cfg!(windows) {
+    if !symlink_supported() {
         return;
     }
 

tests/testsuite/package.rs

@@ -3,11 +3,11 @@ use std::fs::File;
 use std::io::prelude::*;
 use std::path::Path;
 
-use crate::support::cargo_process;
 use crate::support::paths::CargoPathExt;
 use crate::support::registry::Package;
 use crate::support::{
-    basic_manifest, git, path2url, paths, project, publish::validate_crate_contents, registry,
+    basic_manifest, cargo_process, git, path2url, paths, project, publish::validate_crate_contents,
+    registry, symlink_supported,
 };
 use git2;
 
@@ -504,6 +504,56 @@ fn package_git_submodule() {
         .run();
 }
 
+#[cargo_test]
+/// Tests if a symlink to a git submodule is properly handled.
+///
+/// This test requires you to be able to make symlinks.
+/// For windows, this may require you to enable developer mode.
+fn package_symlink_to_submodule() {
+    #[cfg(unix)]
+    use std::os::unix::fs::symlink;
+    #[cfg(windows)]
+    use std::os::windows::fs::symlink_dir as symlink;
+
+    if !symlink_supported() {
+        return;
+    }
+
+    let project = git::new("foo", |project| {
+        project.file("src/lib.rs", "pub fn foo() {}")
+    })
+    .unwrap();
+
+    let library = git::new("submodule", |library| {
+        library.no_manifest().file("Makefile", "all:")
+    })
+    .unwrap();
+
+    let repository = git2::Repository::open(&project.root()).unwrap();
+    let url = path2url(library.root()).to_string();
+    git::add_submodule(&repository, &url, Path::new("submodule"));
+    t!(symlink(
+        &project.root().join("submodule"),
+        &project.root().join("submodule-link")
+    ));
+    git::add(&repository);
+    git::commit(&repository);
+
+    let repository = git2::Repository::open(&project.root().join("submodule")).unwrap();
+    repository
+        .reset(
+            &repository.revparse_single("HEAD").unwrap(),
+            git2::ResetType::Hard,
+            None,
+        )
+        .unwrap();
+
+    project
+        .cargo("package --no-verify -v")
+        .with_stderr_contains("[ARCHIVING] submodule/Makefile")
+        .run();
+}
+
 #[cargo_test]
 fn no_duplicates_from_modified_tracked_files() {
     let root = paths::root().join("all");
@@ -660,9 +710,19 @@ See [..]
 }
 
 #[cargo_test]
-#[cfg(unix)]
+/// Tests if a broken symlink is properly handled when packaging.
+///
+/// This test requires you to be able to make symlinks.
+/// For windows, this may require you to enable developer mode.
 fn broken_symlink() {
-    use std::os::unix::fs;
+    #[cfg(unix)]
+    use std::os::unix::fs::symlink;
+    #[cfg(windows)]
+    use std::os::windows::fs::symlink_dir as symlink;
+
+    if !symlink_supported() {
+        return;
+    }
 
     let p = project()
         .file(
@@ -681,7 +741,7 @@ fn broken_symlink() {
         )
         .file("src/main.rs", r#"fn main() { println!("hello"); }"#)
         .build();
-    t!(fs::symlink("nowhere", &p.root().join("src/foo.rs")));
+    t!(symlink("nowhere", &p.root().join("src/foo.rs")));
 
     p.cargo("package -v")
         .with_status(101)
@@ -699,6 +759,26 @@ Caused by:
         .run();
 }
 
+#[cargo_test]
+/// Tests if a symlink to a directory is proberly included.
+///
+/// This test requires you to be able to make symlinks.
+/// For windows, this may require you to enable developer mode.
+fn package_symlink_to_dir() {
+    if !symlink_supported() {
+        return;
+    }
+
+    project()
+        .file("src/main.rs", r#"fn main() { println!("hello"); }"#)
+        .file("bla/Makefile", "all:")
+        .symlink_dir("bla", "foo")
+        .build()
+        .cargo("package -v")
+        .with_stderr_contains("[ARCHIVING] foo/Makefile")
+        .run();
+}
+
 #[cargo_test]
 fn do_not_package_if_repository_is_dirty() {
     let p = project().build();

tests/testsuite/small_fd_limits.rs

@@ -98,9 +98,6 @@ fn use_git_gc() {
 }
 
 #[cargo_test]
-// it looks like this test passes on some windows machines but not others,
-// notably not on AppVeyor's machines. Sounds like another but for another day.
-#[cfg_attr(windows, ignore)]
 fn avoid_using_git() {
     let path = env::var_os("PATH").unwrap_or_default();
     let mut paths = env::split_paths(&path).collect::<Vec<_>>();

tests/testsuite/support/mod.rs

@@ -178,11 +178,24 @@ impl FileBuilder {
 struct SymlinkBuilder {
     dst: PathBuf,
     src: PathBuf,
+    src_is_dir: bool,
 }
 
 impl SymlinkBuilder {
     pub fn new(dst: PathBuf, src: PathBuf) -> SymlinkBuilder {
-        SymlinkBuilder { dst, src }
+        SymlinkBuilder {
+            dst,
+            src,
+            src_is_dir: false,
+        }
+    }
+
+    pub fn new_dir(dst: PathBuf, src: PathBuf) -> SymlinkBuilder {
+        SymlinkBuilder {
+            dst,
+            src,
+            src_is_dir: true,
+        }
     }
 
     #[cfg(unix)]
@@ -194,7 +207,11 @@ impl SymlinkBuilder {
     #[cfg(windows)]
     fn mk(&self) {
         self.dirname().mkdir_p();
-        t!(os::windows::fs::symlink_file(&self.dst, &self.src));
+        if self.src_is_dir {
+            t!(os::windows::fs::symlink_dir(&self.dst, &self.src));
+        } else {
+            t!(os::windows::fs::symlink_file(&self.dst, &self.src));
+        }
     }
 
     fn dirname(&self) -> &Path {
@@ -252,7 +269,7 @@ impl ProjectBuilder {
             .push(FileBuilder::new(self.root.root().join(path), body));
     }
 
-    /// Adds a symlink to the project.
+    /// Adds a symlink to a file to the project.
     pub fn symlink<T: AsRef<Path>>(mut self, dst: T, src: T) -> Self {
         self.symlinks.push(SymlinkBuilder::new(
             self.root.root().join(dst),
@@ -261,6 +278,15 @@ impl ProjectBuilder {
         self
     }
 
+    /// Create a symlink to a directory
+    pub fn symlink_dir<T: AsRef<Path>>(mut self, dst: T, src: T) -> Self {
+        self.symlinks.push(SymlinkBuilder::new_dir(
+            self.root.root().join(dst),
+            self.root.root().join(src),
+        ));
+        self
+    }
+
     pub fn no_manifest(mut self) -> Self {
         self.no_manifest = true;
         self

tests/testsuite/support/paths.rs

@@ -153,7 +153,7 @@ impl CargoPathExt for Path {
         where
             F: Fn(i64, u32) -> ((i64, u32)),
         {
-            let stat = t!(path.metadata());
+            let stat = t!(path.symlink_metadata());
 
             let mtime = FileTime::from_last_modification_time(&stat);