Auto merge of rust-lang#10214 - weihanglo:revert-10188-issue-9528, r=alexcrichton

Be resilient to most IO error and filesystem loop while walking dirs

Let `PathSource::walk` be resilient to most IO errors and filesystem loop.

This PR also

- Add a test validating the resilience against filesystem loop to prevent regression.
- Emit warning when filesystem loop found while walking the filesystem. This is the only way I can think of now to solve rust-lang#9528

Fixes rust-lang#10213.

Author: bors

src/cargo/sources/path.rs

@@ -340,7 +340,7 @@ impl<'cfg> PathSource<'cfg> {
                         ret.extend(files.into_iter());
                     }
                     Err(..) => {
-                        PathSource::walk(&file_path, &mut ret, false, filter)?;
+                        self.walk(&file_path, &mut ret, false, filter)?;
                     }
                 }
             } else if filter(&file_path, is_dir) {
@@ -378,11 +378,12 @@ impl<'cfg> PathSource<'cfg> {
         filter: &mut dyn FnMut(&Path, bool) -> bool,
     ) -> CargoResult<Vec<PathBuf>> {
         let mut ret = Vec::new();
-        PathSource::walk(pkg.root(), &mut ret, true, filter)?;
+        self.walk(pkg.root(), &mut ret, true, filter)?;
         Ok(ret)
     }
 
     fn walk(
+        &self,
         path: &Path,
         ret: &mut Vec<PathBuf>,
         is_root: bool,
@@ -420,9 +421,22 @@ impl<'cfg> PathSource<'cfg> {
                 true
             });
         for entry in walkdir {
-            let entry = entry?;
-            if !entry.file_type().is_dir() {
-                ret.push(entry.path().to_path_buf());
+            match entry {
+                Ok(entry) => {
+                    if !entry.file_type().is_dir() {
+                        ret.push(entry.into_path());
+                    }
+                }
+                Err(err) if err.loop_ancestor().is_some() => {
+                    self.config.shell().warn(err)?;
+                }
+                Err(err) => match err.path() {
+                    // If the error occurs with a path, simply recover from it.
+                    // Don't worry about error skipping here, the callers would
+                    // still hit the IO error if they do access it thereafter.
+                    Some(path) => ret.push(path.to_path_buf()),
+                    None => return Err(err.into()),
+                },
             }
         }
 

tests/testsuite/build.rs

@@ -1668,7 +1668,7 @@ package `test v0.0.0 ([CWD])`
 }
 
 #[cargo_test]
-/// Make sure broken symlinks don't break the build
+/// Make sure broken and loop 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.
@@ -1681,9 +1681,17 @@ fn ignore_broken_symlinks() {
         .file("Cargo.toml", &basic_bin_manifest("foo"))
         .file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
         .symlink("Notafile", "bar")
+        // To hit the symlink directory, we need a build script
+        // to trigger a full scan of package files.
+        .file("build.rs", &main_file(r#""build script""#, &[]))
+        .symlink_dir("a/b", "a/b/c/d/foo")
         .build();
 
-    p.cargo("build").run();
+    p.cargo("build")
+        .with_stderr_contains(
+            "[WARNING] File system loop found: [..]/a/b/c/d/foo points to an ancestor [..]/a/b",
+        )
+        .run();
     assert!(p.bin("foo").is_file());
 
     p.process(&p.bin("foo")).with_stdout("i am foo\n").run();

tests/testsuite/build_script.rs

@@ -4618,75 +4618,6 @@ fn links_interrupted_can_restart() {
         .run();
 }
 
-#[cargo_test]
-#[cfg(unix)]
-fn build_script_scan_eacces() {
-    // build.rs causes a scan of the whole project, which can be a problem if
-    // a directory is not accessible.
-    use cargo_test_support::git;
-    use std::os::unix::fs::PermissionsExt;
-
-    let p = project()
-        .file("src/lib.rs", "")
-        .file("build.rs", "fn main() {}")
-        .file("secrets/stuff", "")
-        .build();
-    let path = p.root().join("secrets");
-    fs::set_permissions(&path, fs::Permissions::from_mode(0o0)).unwrap();
-    // The last "Caused by" is a string from libc such as the following:
-    //   Permission denied (os error 13)
-    p.cargo("build")
-        .with_stderr(
-            "\
-[ERROR] failed to determine package fingerprint for build script for foo v0.0.1 ([..]/foo)
-An I/O error happened[..]
-
-By default[..]
-it to[..]
-file[..]
-See[..]
-
-Caused by:
-  failed to determine the most recently modified file in [..]/foo
-
-Caused by:
-  failed to determine list of files in [..]/foo
-
-Caused by:
-  IO error for operation on [..]/foo/secrets: [..]
-
-Caused by:
-  [..]
-",
-        )
-        .with_status(101)
-        .run();
-
-    // Try `package.exclude` to skip a directory.
-    p.change_file(
-        "Cargo.toml",
-        r#"
-        [package]
-        name = "foo"
-        version = "0.0.1"
-        exclude = ["secrets"]
-        "#,
-    );
-    p.cargo("build").run();
-
-    // Try with git. This succeeds because the git status walker ignores
-    // directories it can't access.
-    p.change_file("Cargo.toml", &basic_manifest("foo", "0.0.1"));
-    p.build_dir().rm_rf();
-    let repo = git::init(&p.root());
-    git::add(&repo);
-    git::commit(&repo);
-    p.cargo("build").run();
-
-    // Restore permissions so that the directory can be deleted.
-    fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap();
-}
-
 #[cargo_test]
 fn dev_dep_with_links() {
     let p = project()

tests/testsuite/package.rs

@@ -794,10 +794,10 @@ fn broken_symlink() {
         .with_status(101)
         .with_stderr_contains(
             "\
-error: failed to determine list of files [..]/foo
+[ERROR] failed to prepare local package for uploading
 
 Caused by:
-  IO error for operation on [..]/foo/src/foo.rs: [..]
+  failed to open for archiving: `[..]foo.rs`
 
 Caused by:
   [..]
@@ -841,9 +841,8 @@ fn filesystem_loop() {
         .symlink_dir("a/b", "a/b/c/d/foo")
         .build()
         .cargo("package -v")
-        .with_status(101)
         .with_stderr_contains(
-            "  File system loop found: [..]/a/b/c/d/foo points to an ancestor [..]/a/b",
+            "[WARNING] File system loop found: [..]/a/b/c/d/foo points to an ancestor [..]/a/b",
         )
         .run();
 }