Skip to content

Commit

Permalink
Auto merge of #10188 - weihanglo:issue-9528, r=alexcrichton
Browse files Browse the repository at this point in the history 
Detect filesystem loop during walking the projects

Resolves #9528

~~This PR also adds a new dependency `same-file` but since it's already a
dependency of `cargo-util`, so nothing added actually.~~

Use `walkdir` to detect filesystem loop and gain performance boost!
  • Loading branch information
@bors
bors committed Dec 16, 2021
2 parents b9bc3d1 + b49fe50 commit b05697d
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 55 deletions.
105 changes: 54 additions & 51 deletions src/cargo/sources/path.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::collections::HashSet;
use std::fmt::{self, Debug, Formatter};
use std::fs;
use std::path::{Path, PathBuf};

use crate::core::source::MaybePackage;
Expand All @@ -11,8 +10,8 @@ use anyhow::Context as _;
use cargo_util::paths;
use filetime::FileTime;
use ignore::gitignore::GitignoreBuilder;
use ignore::Match;
use log::{trace, warn};
use walkdir::WalkDir;

pub struct PathSource<'cfg> {
source_id: SourceId,
Expand Down Expand Up @@ -131,39 +130,36 @@ impl<'cfg> PathSource<'cfg> {
}
let ignore_include = include_builder.build()?;

let ignore_should_package = |relative_path: &Path, is_dir: bool| -> CargoResult<bool> {
let ignore_should_package = |relative_path: &Path, is_dir: bool| {
// "Include" and "exclude" options are mutually exclusive.
if no_include_option {
match ignore_exclude.matched_path_or_any_parents(relative_path, is_dir) {
Match::None => Ok(true),
Match::Ignore(_) => Ok(false),
Match::Whitelist(_) => Ok(true),
}
!ignore_exclude
.matched_path_or_any_parents(relative_path, is_dir)
.is_ignore()
} else {
if is_dir {
// Generally, include directives don't list every
// directory (nor should they!). Just skip all directory
// checks, and only check files.
return Ok(true);
return true;
}
match ignore_include
ignore_include
.matched_path_or_any_parents(relative_path, /* is_dir */ false)
{
Match::None => Ok(false),
Match::Ignore(_) => Ok(true),
Match::Whitelist(_) => Ok(false),
}
.is_ignore()
}
};

let mut filter = |path: &Path, is_dir: bool| -> CargoResult<bool> {
let relative_path = path.strip_prefix(root)?;
let mut filter = |path: &Path, is_dir: bool| {
let relative_path = match path.strip_prefix(root) {
Ok(p) => p,
Err(_) => return false,
};

let rel = relative_path.as_os_str();
if rel == "Cargo.lock" {
return Ok(pkg.include_lockfile());
return pkg.include_lockfile();
} else if rel == "Cargo.toml" {
return Ok(true);
return true;
}

ignore_should_package(relative_path, is_dir)
Expand Down Expand Up @@ -225,7 +221,7 @@ impl<'cfg> PathSource<'cfg> {
&self,
pkg: &Package,
repo: &git2::Repository,
filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
filter: &mut dyn FnMut(&Path, bool) -> bool,
) -> CargoResult<Vec<PathBuf>> {
warn!("list_files_git {}", pkg.package_id());
let index = repo.index()?;
Expand Down Expand Up @@ -347,7 +343,7 @@ impl<'cfg> PathSource<'cfg> {
PathSource::walk(&file_path, &mut ret, false, filter)?;
}
}
} else if (*filter)(&file_path, is_dir)? {
} else if filter(&file_path, is_dir) {
assert!(!is_dir);
// We found a file!
warn!(" found {}", file_path.display());
Expand Down Expand Up @@ -379,7 +375,7 @@ impl<'cfg> PathSource<'cfg> {
fn list_files_walk(
&self,
pkg: &Package,
filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
filter: &mut dyn FnMut(&Path, bool) -> bool,
) -> CargoResult<Vec<PathBuf>> {
let mut ret = Vec::new();
PathSource::walk(pkg.root(), &mut ret, true, filter)?;
Expand All @@ -390,39 +386,46 @@ impl<'cfg> PathSource<'cfg> {
path: &Path,
ret: &mut Vec<PathBuf>,
is_root: bool,
filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
filter: &mut dyn FnMut(&Path, bool) -> bool,
) -> CargoResult<()> {
let is_dir = path.is_dir();
if !is_root && !(*filter)(path, is_dir)? {
return Ok(());
}
if !is_dir {
ret.push(path.to_path_buf());
return Ok(());
}
// Don't recurse into any sub-packages that we have.
if !is_root && path.join("Cargo.toml").exists() {
return Ok(());
}
let walkdir = WalkDir::new(path)
.follow_links(true)
.into_iter()
.filter_entry(|entry| {
let path = entry.path();
let at_root = is_root && entry.depth() == 0;
let is_dir = entry.file_type().is_dir();

if !at_root && !filter(path, is_dir) {
return false;
}

// For package integration tests, we need to sort the paths in a deterministic order to
// be able to match stdout warnings in the same order.
//
// TODO: drop `collect` and sort after transition period and dropping warning tests.
// See rust-lang/cargo#4268 and rust-lang/cargo#4270.
let mut entries: Vec<PathBuf> = fs::read_dir(path)
.with_context(|| format!("cannot read {:?}", path))?
.map(|e| e.unwrap().path())
.collect();
entries.sort_unstable_by(|a, b| a.as_os_str().cmp(b.as_os_str()));
for path in entries {
let name = path.file_name().and_then(|s| s.to_str());
if is_root && name == Some("target") {
// Skip Cargo artifacts.
continue;
if !is_dir {
return true;
}

// Don't recurse into any sub-packages that we have.
if !at_root && path.join("Cargo.toml").exists() {
return false;
}

// Skip root Cargo artifacts.
if is_root
&& entry.depth() == 1
&& path.file_name().and_then(|s| s.to_str()) == Some("target")
{
return false;
}

true
});
for entry in walkdir {
let entry = entry?;
if !entry.file_type().is_dir() {
ret.push(entry.path().to_path_buf());
}
PathSource::walk(&path, ret, false, filter)?;
}

Ok(())
}

Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/build_script.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4653,7 +4653,7 @@ Caused by:
failed to determine list of files in [..]/foo
Caused by:
cannot read \"[..]/foo/secrets\"
IO error for operation on [..]/foo/secrets: [..]
Caused by:
[..]
Expand Down
29 changes: 26 additions & 3 deletions tests/testsuite/package.rs
View file
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 prepare local package for uploading
error: failed to determine list of files [..]/foo
Caused by:
failed to open for archiving: `[..]foo.rs`
IO error for operation on [..]/foo/src/foo.rs: [..]
Caused by:
[..]
Expand Down Expand Up @@ -826,6 +826,28 @@ fn package_symlink_to_dir() {
.run();
}

#[cargo_test]
/// Tests if a symlink to ancestor causes filesystem loop error.
///
/// This test requires you to be able to make symlinks.
/// For windows, this may require you to enable developer mode.
fn filesystem_loop() {
if !symlink_supported() {
return;
}

project()
.file("src/main.rs", r#"fn main() { println!("hello"); }"#)
.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",
)
.run();
}

#[cargo_test]
fn do_not_package_if_repository_is_dirty() {
let p = project().build();
Expand Down Expand Up @@ -1798,7 +1820,8 @@ fn package_restricted_windows() {
.build();

p.cargo("package")
.with_stderr(
// use unordered here because the order of the warning is different on each platform.
.with_stderr_unordered(
"\
[WARNING] file src/aux/mod.rs is a reserved Windows filename, it will not work on Windows platforms
[WARNING] file src/con.rs is a reserved Windows filename, it will not work on Windows platforms
Expand Down

0 comments on commit b05697d

Please sign in to comment.