Skip to content

Commit

Permalink
Auto merge of #10214 - weihanglo:revert-10188-issue-9528, r=alexcrichton
Browse files Browse the repository at this point in the history
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 #9528

Fixes #10213.
  • Loading branch information
bors committed Jan 5, 2022
2 parents dc6d847 + d92dcea commit 2478331
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 80 deletions.
24 changes: 19 additions & 5 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()),
},
}
}

Expand Down
12 changes: 10 additions & 2 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
Expand Down
69 changes: 0 additions & 69 deletions tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 3 additions & 4 deletions tests/testsuite/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
[..]
Expand Down Expand Up @@ -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();
}
Expand Down

0 comments on commit 2478331

Please sign in to comment.