From 28e567638f9deb0f9a00662723d93482e03904fa Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 19:43:04 -0700 Subject: [PATCH 1/4] Walk the app with dua-core in place of jwalk jwalk 0.9's changelog says it is no longer maintained and points to dua-core, the walker its author now develops in dua-cli. dua-core asks before it reads each directory, so pks still skips hidden, gitignored and excluded directories without reading them. Each entry names the directory it's in, so once the walk is done, each file is matched to the package.yml nearest above it, from the package.yml files the walk found, with no stat of each directory for one. pks skips hidden entries, as jwalk did and packwerk's globs do. dua-core never follows symlinks. A symlink to a file still counts as the file, but pks no longer descends into a symlinked directory, as packwerk's globs don't either. jwalk followed links, and a link to a directory that contains it, such as `loop -> ..`, made pks list the same files and packs dozens of times. dua-core's workers stop once a few batches of entries wait for pks's loop, which happens each time the loop parses a package.yml. Until a release has Byron/dua-cli#413, a thread takes the entries as they come, so the walk takes as long as it did. On every fixture and four Rails apps, list-included-files, list-packs and list-references print the same as before. --- CHANGELOG.md | 9 ++ Cargo.lock | 23 +-- Cargo.toml | 2 +- src/packs/walk_directory.rs | 224 ++++++++++++++++++------------ tests/list_included_files_test.rs | 105 ++++++++++++++ 5 files changed, 260 insertions(+), 103 deletions(-) create mode 100644 tests/list_included_files_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 06bf76a..d673253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,15 @@ Building pks from source now needs a C compiler and libclang. Releases no longer include a binary for Intel Macs (`x86_64-apple-darwin`), and the Apple Silicon binary is built on macOS 26. +#### pks doesn't descend into a symlinked directory + +pks walks the app with [dua-core](https://crates.io/crates/dua-core) in place +of jwalk, which is no longer maintained, and which followed symlinks. Like +packwerk's globs, pks still reads a symlink to a file, but no longer descends +into a symlinked directory, so it no longer finds the files or packs in one. A +link to a directory that contains it, such as `loop -> ..`, made pks list the +same files and packs over and over, and no longer does. + ## 0.7.0 ### Features diff --git a/Cargo.lock b/Cargo.lock index 22fd341..a3ec20a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -453,6 +453,17 @@ dependencies = [ "syn 3.0.9", ] +[[package]] +name = "dua-core" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0664627ab16c082ac5136a232dfef44ce0627a6cc43fca6f98e0284d3f9b1d79" +dependencies = [ + "crossbeam", + "libc", + "windows-sys 0.61.2", +] + [[package]] name = "dunce" version = "1.0.5" @@ -1195,16 +1206,6 @@ dependencies = [ "zmij", ] -[[package]] -name = "jwalk" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e610b2b1eaea9e0e062c47e319d20a2e0f929e7aabf24d58514354de967bfbd" -dependencies = [ - "crossbeam", - "rayon", -] - [[package]] name = "lazy_static" version = "1.5.1" @@ -1511,13 +1512,13 @@ dependencies = [ "clap", "clap_derive", "csv", + "dua-core", "fnmatch-regex2", "glob", "globset", "ignore", "itertools 0.15.0", "jsonschema", - "jwalk", "line-col", "md5", "path-clean", diff --git a/Cargo.toml b/Cargo.toml index f46db43..fd73eb6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,8 +49,8 @@ anyhow = { version = "1.0.75", features = [] } # for err clap = { version = "4.2.1", features = ["derive"] } # cli clap_derive = "4.2.0" # cli csv = "1.3.0" # csv de/serialize +dua-core = "4.1.0" # for walking the file tree itertools = "0.15.0" # tools for iterating over iterable things -jwalk = "0.9.0" # for walking the file tree path-clean = "1.0.1" # Pathname#cleaname in Ruby rayon = "1.10.0" # for parallel iteration regex = "1.7.3" diff --git a/src/packs/walk_directory.rs b/src/packs/walk_directory.rs index 2f32de8..d0f85da 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -1,7 +1,9 @@ +use dua_core::{DirectoryId, Entry, Options, Order}; use ignore::gitignore::{Gitignore, GitignoreBuilder}; -use jwalk::WalkDirGeneric; use std::{ collections::{HashMap, HashSet}, + ffi::OsStr, + num::NonZeroUsize, path::{Path, PathBuf}, sync::Arc, }; @@ -17,15 +19,16 @@ pub struct WalkDirectoryResult { pub owning_package_yml_for_file: HashMap, } -#[derive(Debug, Default, Clone)] -struct ProcessReadDirState { - current_package_yml: PathBuf, +/// A name that starts with a dot. pks skips these, as packwerk's globs don't +/// match them. +fn is_hidden(file_name: &OsStr) -> bool { + file_name.to_str().is_some_and(|name| name.starts_with('.')) } -impl jwalk::ClientState for ProcessReadDirState { - type ReadDirState = ProcessReadDirState; - - type DirEntryState = ProcessReadDirState; +struct Directory { + id: DirectoryId, + parent: Option, + depth: usize, } /// Locates the global gitignore file from git config (`core.excludesFile`). @@ -90,20 +93,20 @@ pub fn build_gitignore_matcher( Ok(builder.build()?) } -// We use jwalk to walk directories in parallel and compare them to the `include` and `exclude` patterns +// We use dua-core to walk directories in parallel and compare them to the `include` and `exclude` patterns // specified in the `RawConfiguration` -// https://docs.rs/jwalk/0.9.0/jwalk/struct.WalkDirGeneric.html#method.process_read_dir +// https://docs.rs/dua-core/4.1.0/dua_core/fn.walk.html // We only walk the directory once and pull all of the information we need from it, // which is faster than walking the directory multiple times. // Likely, we can organize this better by moving each piece of logic into its own function so this function // allows for a sort of "visitor pattern" for different things that need to walk the directory. // // Note: `ignore::WalkBuilder` (used by ripgrep) handles gitignore natively and supports -// parallel traversal via `build_parallel()`. We keep jwalk here because its -// `ProcessReadDirState` mechanism lets us propagate `current_package_yml` state from parent -// to child directories in a single pass. WalkBuilder's visitor pattern has no equivalent -// hook, so implementing that tracking would require per-file ancestor walks — an O(depth) -// regression for large repos. +// parallel traversal via `build_parallel()`. We use dua-core here because each entry it +// yields names the directory it's in, so we can propagate `current_package_yml` from parent +// to child directories without walking each file's ancestors. WalkBuilder's visitor pattern +// has no equivalent, so implementing that tracking would require per-file ancestor walks, +// an O(depth) regression for large repos. pub(crate) fn walk_directory( absolute_root: PathBuf, raw: &RawConfiguration, @@ -161,73 +164,71 @@ pub(crate) fn walk_directory( // TODO: Pull directory walker into separate module. Allow it to be called with implementations of a trait // so separate concerns can each be in their own place. // - // WalkDirGeneric allows you to customize the directory walk, such as skipping directories, + // The walk asks before it reads each directory, which lets us skip directories, // which we do as a performance optimization. // // Specifically – if an exclude glob matches an entire directory, we don't need to continue to // explore it. For example, instead of asking every file in `vendor/bundle/**/` if it should be excluded, // we'll save a lot of time by just skipping the entire directory. - // - // For more information, check out the docs: https://docs.rs/jwalk/0.9.0/jwalk/#extended-example - let current_package_yml = PathBuf::from("package.yml"); - - let walk_dir = WalkDirGeneric::::new(&absolute_root) - .follow_links(true) - .root_read_dir_state(ProcessReadDirState { - current_package_yml, - }) - .process_read_dir( - move |_depth, absolute_dirname, read_dir_state, children| { - // We need to let the compiler know that we are using a reference and not the value itself. - // We need to then clone the Arc to get a new reference, which is a new pointer to the value/data - // (with an increase to the reference count). - let cloned_excluded_dirs = excluded_dirs_ref.clone(); - let cloned_absolute_root = absolute_root_ref.clone(); - let cloned_gitignore = gitignore_ref.clone(); - let package_yml = absolute_dirname.join("package.yml"); - - // Even if the parent has set this on children, the existence of a new - // package.yml file should override it. - if package_yml.exists() { - read_dir_state.current_package_yml = package_yml; - } - - children.iter_mut().for_each(|child_dir_entry_result| { - if let Ok(child_dir_entry) = child_dir_entry_result { - let child_absolute_dirname = child_dir_entry.path(); - child_dir_entry - .client_state - .current_package_yml - .clone_from(&read_dir_state.current_package_yml); - - let relative_path = child_absolute_dirname - .strip_prefix(cloned_absolute_root.as_ref()) - .unwrap(); - - // Check gitignore for directories only (optimization: prune ignored directory trees early) - // Files are checked separately in the main file processing loop below - if let Some(gitignore) = cloned_gitignore.as_ref() { - let is_dir = child_dir_entry.file_type.is_dir(); - if is_dir - && gitignore - .matched(relative_path, true) - .is_ignore() - { - child_dir_entry.read_children = None; - } - } - - // Then check explicit exclusions - if cloned_excluded_dirs.as_ref().is_match(relative_path) - { - child_dir_entry.read_children = None; - } - } - }); - }, - ); + let read_directory = move |entry: &Entry| { + if entry.depth == 0 { + return true; + } + if is_hidden(&entry.file_name) { + return false; + } + + let absolute_dirname = entry.path(); + let relative_path = absolute_dirname + .strip_prefix(absolute_root_ref.as_ref()) + .unwrap(); + + // Check gitignore for directories (optimization: prune ignored directory trees early) + // Files are checked separately in the main file processing loop below + if let Some(gitignore) = gitignore_ref.as_ref() { + if gitignore.matched(relative_path, true).is_ignore() { + return false; + } + } + + // Then check explicit exclusions + !excluded_dirs_ref.is_match(relative_path) + }; + + let threads = + std::thread::available_parallelism().map_or(1, NonZeroUsize::get); + let dua_walk = dua_core::walk( + &absolute_root, + threads, + Order::Completion, + Options::default().skip_metadata(), + read_directory, + ); + + // dua-core's workers stop once a few batches of entries are waiting for + // this loop, as they are while it parses a package.yml. This thread takes + // the entries as they come, so the workers keep walking. A dua-core + // release with https://github.com/Byron/dua-cli/pull/413 would make it + // unnecessary. + let (sender, walk) = std::sync::mpsc::channel(); + let receiving = std::thread::spawn(move || { + for entry in dua_walk { + if sender.send(entry).is_err() { + break; + } + } + }); + + // A file can come before the directory it's in, and before the + // package.yml there, so which package.yml each file belongs to is + // settled once the walk is done + let mut directories: Vec = Vec::new(); + let mut package_ymls_in_directories: HashMap = + HashMap::new(); + let mut included_files_in_directories: Vec<(PathBuf, DirectoryId)> = + Vec::new(); - for entry in walk_dir { + for entry in walk { // I was using this to explore what directories were being walked to potentially // find performance improvements. // Write the entry out to a log file: @@ -239,21 +240,40 @@ pub(crate) fn walk_directory( // .unwrap(); // writeln!(file, "{:?}", entry).unwrap(); - let unwrapped_entry = entry; - if let Err(_e) = unwrapped_entry { - // Encountered an invalid symlink. Being consistent with packwerk, which swallows this error and continues + let Ok(entry) = entry else { + // Couldn't read a directory or entry. Being consistent with packwerk, which swallows this error and continues + continue; + }; + if entry.depth > 0 && is_hidden(&entry.file_name) { + continue; + } + + if let Some(id) = entry.directory_id { + directories.push(Directory { + id, + parent: entry.parent_directory_id, + depth: entry.depth, + }); continue; } - let unwrapped_entry = unwrapped_entry.unwrap(); - // Note that we could also get the dir from absolute_path.is_dir() - // However, this data appears to be cached on the FileType struct, so we'll use that instead, - // which is much faster! - if unwrapped_entry.file_type.is_dir() { + let absolute_path = entry.path(); + + // The walk doesn't follow symlinks, and packwerk's globs don't + // descend into a symlinked directory either. A symlink to a file + // counts as the file. + if entry.file_type.is_symlink() && !absolute_path.is_file() { continue; } - let absolute_path = unwrapped_entry.path(); + let directory = entry + .parent_directory_id + .expect("every entry but the root is in a directory"); + let is_package_yml = entry.file_name == "package.yml"; + if is_package_yml { + package_ymls_in_directories + .insert(directory, absolute_path.clone()); + } let relative_path = absolute_path .strip_prefix(&absolute_root) @@ -267,10 +287,8 @@ pub(crate) fn walk_directory( } } - let current_package_yml = - &unwrapped_entry.client_state.current_package_yml; - - if &absolute_path == current_package_yml + // A package.yml is the one the files in its directory belong to + if is_package_yml // Ideally, we don't need the second part of this conditional, but it's here // because there is a bug where the root pack doesn't match package_paths. // We know we always want the root pack to be registered, since it's the catch-all pack for @@ -286,8 +304,7 @@ pub(crate) fn walk_directory( if includes_set.is_match(&relative_path) { if !excludes_set.is_match(&relative_path) { included_files.insert(absolute_path.clone()); - owning_package_yml_for_file - .insert(absolute_path, current_package_yml.clone()); + included_files_in_directories.push((absolute_path, directory)); } else { // println!("file excluded: {}", relative_path.display()) } @@ -300,6 +317,31 @@ pub(crate) fn walk_directory( } } + if let Err(panic) = receiving.join() { + std::panic::resume_unwind(panic); + } + + // The package.yml that the files in each directory belong to. A + // directory is shallower than those in it, so its parent's is known first. + directories.sort_unstable_by_key(|directory| directory.depth); + let mut package_ymls: HashMap = HashMap::new(); + for directory in directories { + // Even if the parent has one, the existence of a new package.yml + // file should override it. + let package_yml = match package_ymls_in_directories.get(&directory.id) { + Some(package_yml) => package_yml.clone(), + None => match directory.parent { + Some(parent) => package_ymls[&parent].clone(), + None => PathBuf::from("package.yml"), + }, + }; + package_ymls.insert(directory.id, package_yml); + } + for (absolute_path, directory) in included_files_in_directories { + owning_package_yml_for_file + .insert(absolute_path, package_ymls[&directory].clone()); + } + debug!("Finished directory walk"); Ok(WalkDirectoryResult { diff --git a/tests/list_included_files_test.rs b/tests/list_included_files_test.rs new file mode 100644 index 0000000..3ec0b5d --- /dev/null +++ b/tests/list_included_files_test.rs @@ -0,0 +1,105 @@ +use std::{collections::BTreeSet, error::Error, fs}; + +mod common; + +/// The files `list-included-files` lists, relative to the fixture's root +fn included_files( + fixture: &common::Fixture, +) -> Result, Box> { + let output = common::pks(fixture, &["--no-cache", "list-included-files"]) + .success() + .get_output() + .stdout + .clone(); + // pks prints canonical paths, and the fixture's can go through a symlink, + // as macOS's /var does + let root = format!("{}/", fixture.root().canonicalize()?.display()); + Ok(String::from_utf8(output)? + .lines() + .map(|line| line.strip_prefix(&root).unwrap_or(line).to_owned()) + .collect()) +} + +#[test] +fn test_list_included_files_skips_hidden_files_and_directories( +) -> Result<(), Box> { + // As packwerk's globs don't match them + let fixture = common::Fixture::new("simple_app"); + fs::create_dir_all(fixture.path("packs/foo/.hidden/app"))?; + fs::write(fixture.path("packs/foo/.hidden/app/hidden.rb"), "")?; + fs::write(fixture.path("packs/foo/.dotfile.rb"), "")?; + + let files = included_files(&fixture)?; + assert!( + files.contains("packs/foo/app/services/foo.rb"), + "{:?}", + files + ); + assert!( + !files.contains("packs/foo/.hidden/app/hidden.rb"), + "{:?}", + files + ); + assert!(!files.contains("packs/foo/.dotfile.rb"), "{:?}", files); + + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_list_included_files_lists_a_symlink_to_a_file( +) -> Result<(), Box> { + use std::os::unix::fs::symlink; + + let fixture = common::Fixture::new("simple_app"); + let outside = tempfile::tempdir()?; + fs::write(outside.path().join("linked.rb"), "")?; + let services = fixture.path("packs/foo/app/services"); + symlink(outside.path().join("linked.rb"), services.join("linked.rb"))?; + symlink(services.join("missing.rb"), services.join("broken.rb"))?; + + let files = included_files(&fixture)?; + assert!( + files.contains("packs/foo/app/services/linked.rb"), + "{:?}", + files + ); + assert!( + !files.contains("packs/foo/app/services/broken.rb"), + "{:?}", + files + ); + + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_list_included_files_doesnt_descend_into_a_symlinked_directory( +) -> Result<(), Box> { + use std::os::unix::fs::symlink; + + // As packwerk's globs don't. Nor can a link back up the tree make pks + // list the same files over and over. + let fixture = common::Fixture::new("simple_app"); + let outside = tempfile::tempdir()?; + fs::create_dir_all(outside.path().join("app"))?; + fs::write(outside.path().join("app/outside.rb"), "")?; + symlink(outside.path(), fixture.path("packs/linked"))?; + symlink("..", fixture.path("packs/foo/loop"))?; + + let files = included_files(&fixture)?; + assert!( + files.contains("packs/foo/app/services/foo.rb"), + "{:?}", + files + ); + assert!( + !files.iter().any(|file| file.starts_with("packs/linked/") + || file.starts_with("packs/foo/loop/")), + "{:?}", + files + ); + + Ok(()) +} From 9f7731e203d2465b6f056382e5bb4b71eb832158 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 19:58:24 -0700 Subject: [PATCH 2/4] Trim the comments on the walk Two restated the code under them, and three said more than they needed to. --- src/packs/walk_directory.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/packs/walk_directory.rs b/src/packs/walk_directory.rs index d0f85da..3dd7e25 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -19,8 +19,7 @@ pub struct WalkDirectoryResult { pub owning_package_yml_for_file: HashMap, } -/// A name that starts with a dot. pks skips these, as packwerk's globs don't -/// match them. +/// packwerk's globs don't match a name that starts with a dot fn is_hidden(file_name: &OsStr) -> bool { file_name.to_str().is_some_and(|name| name.starts_with('.')) } @@ -191,7 +190,6 @@ pub(crate) fn walk_directory( } } - // Then check explicit exclusions !excluded_dirs_ref.is_match(relative_path) }; @@ -205,11 +203,9 @@ pub(crate) fn walk_directory( read_directory, ); - // dua-core's workers stop once a few batches of entries are waiting for - // this loop, as they are while it parses a package.yml. This thread takes - // the entries as they come, so the workers keep walking. A dua-core - // release with https://github.com/Byron/dua-cli/pull/413 would make it - // unnecessary. + // dua-core's workers stop while a few batches wait for this loop, as they + // do each time it parses a package.yml, so this thread drains them. Not + // needed once dua-core has https://github.com/Byron/dua-cli/pull/413 let (sender, walk) = std::sync::mpsc::channel(); let receiving = std::thread::spawn(move || { for entry in dua_walk { @@ -321,13 +317,10 @@ pub(crate) fn walk_directory( std::panic::resume_unwind(panic); } - // The package.yml that the files in each directory belong to. A - // directory is shallower than those in it, so its parent's is known first. + // By depth, so each directory's parent has its package.yml first directories.sort_unstable_by_key(|directory| directory.depth); let mut package_ymls: HashMap = HashMap::new(); for directory in directories { - // Even if the parent has one, the existence of a new package.yml - // file should override it. let package_yml = match package_ymls_in_directories.get(&directory.id) { Some(package_yml) => package_yml.clone(), None => match directory.parent { From 0ee01a68317811f541b6714323c2f1d98f2181be Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 20:07:05 -0700 Subject: [PATCH 3/4] Say which dua-core option will replace the draining thread Byron/dua-cli#413 merged as Options::queued_batches_per_worker, which keeps the default of two batches, so pks will set it once it is released. --- src/packs/walk_directory.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/packs/walk_directory.rs b/src/packs/walk_directory.rs index 3dd7e25..e14c412 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -204,8 +204,9 @@ pub(crate) fn walk_directory( ); // dua-core's workers stop while a few batches wait for this loop, as they - // do each time it parses a package.yml, so this thread drains them. Not - // needed once dua-core has https://github.com/Byron/dua-cli/pull/413 + // do each time it parses a package.yml, so this thread drains them. Once a + // release has `Options::queued_batches_per_worker`, set that instead + // (https://github.com/Byron/dua-cli/pull/413). let (sender, walk) = std::sync::mpsc::channel(); let receiving = std::thread::spawn(move || { for entry in dua_walk { From 5d719f1610849c0ffd776b37f92ff85943642901 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 11:06:37 -0700 Subject: [PATCH 4/4] Don't panic on a file for the project root, and follow RAYON_NUM_THREADS - `--project-root` naming a file panicked on an `expect`, as dua-core yields a file root with no directory. It again ends in "No root pack found", as on `main`. - The walk takes rayon's number of threads, so `RAYON_NUM_THREADS` limits it, as it limited jwalk, which ran on rayon's pool. - `read_directory` skips a directory it can't make relative instead of unwrapping. dua-core's workers don't catch a panic, so one there would hang pks rather than abort it. - Directories are recorded before hidden names are skipped, so a file's directory is always known. That held only because `read_directory` skips hidden directories too. - Which package.yml each file belongs to is collected into a map of the right size. Growing it took about 80 ms on 118,000 files, against 27. - That resolution is now `package_ymls_of_directories`, with a unit test that gives it directories before their parents. Removing the sort by depth failed no test, since whether the walk yields a directory first varies from run to run. - The symlinked-directory test also links a directory with a name like a Ruby file's, which `exists()` in place of `is_file()` would list. - Comments that described jwalk's design are updated, and the thread that drains the walk is `draining`. --- src/packs/walk_directory.rs | 150 ++++++++++++++++++++---------- tests/list_included_files_test.rs | 25 ++++- 2 files changed, 125 insertions(+), 50 deletions(-) diff --git a/src/packs/walk_directory.rs b/src/packs/walk_directory.rs index e14c412..8be8202 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -3,7 +3,6 @@ use ignore::gitignore::{Gitignore, GitignoreBuilder}; use std::{ collections::{HashMap, HashSet}, ffi::OsStr, - num::NonZeroUsize, path::{Path, PathBuf}, sync::Arc, }; @@ -24,12 +23,36 @@ fn is_hidden(file_name: &OsStr) -> bool { file_name.to_str().is_some_and(|name| name.starts_with('.')) } +/// A directory the walk found, by `DirectoryId::index` struct Directory { - id: DirectoryId, - parent: Option, + id: usize, + parent: Option, depth: usize, } +/// The package.yml the files in each directory belong to: the directory's +/// own, or else its parent's. The walk can yield a directory before its parent. +fn package_ymls_of_directories( + mut directories: Vec, + package_ymls_in_directories: &HashMap, +) -> HashMap { + // By depth, so each directory's parent has its package.yml first + directories.sort_unstable_by_key(|directory| directory.depth); + let mut package_ymls: HashMap = + HashMap::with_capacity(directories.len()); + for directory in directories { + let package_yml = match package_ymls_in_directories.get(&directory.id) { + Some(package_yml) => package_yml.clone(), + None => match directory.parent { + Some(parent) => package_ymls[&parent].clone(), + None => PathBuf::from("package.yml"), + }, + }; + package_ymls.insert(directory.id, package_yml); + } + package_ymls +} + /// Locates the global gitignore file from git config (`core.excludesFile`). /// /// Returns `Some(PathBuf)` if `core.excludesFile` is configured and the file @@ -102,10 +125,8 @@ pub fn build_gitignore_matcher( // // Note: `ignore::WalkBuilder` (used by ripgrep) handles gitignore natively and supports // parallel traversal via `build_parallel()`. We use dua-core here because each entry it -// yields names the directory it's in, so we can propagate `current_package_yml` from parent -// to child directories without walking each file's ancestors. WalkBuilder's visitor pattern -// has no equivalent, so implementing that tracking would require per-file ancestor walks, -// an O(depth) regression for large repos. +// yields names the directory it's in by id, so once the walk is done each file gets its +// directory's package.yml without walking its ancestors. pub(crate) fn walk_directory( absolute_root: PathBuf, raw: &RawConfiguration, @@ -114,8 +135,6 @@ pub(crate) fn walk_directory( let mut included_files: HashSet = HashSet::new(); let mut included_packs: HashSet = HashSet::new(); - let mut owning_package_yml_for_file: HashMap = - HashMap::new(); // Create this vector outside of the closure to avoid reallocating it let default_excluded_dirs = [ @@ -178,9 +197,12 @@ pub(crate) fn walk_directory( } let absolute_dirname = entry.path(); - let relative_path = absolute_dirname - .strip_prefix(absolute_root_ref.as_ref()) - .unwrap(); + // dua-core's workers don't catch a panic, so one here would hang pks + let Ok(relative_path) = + absolute_dirname.strip_prefix(absolute_root_ref.as_ref()) + else { + return false; + }; // Check gitignore for directories (optimization: prune ignored directory trees early) // Files are checked separately in the main file processing loop below @@ -193,11 +215,10 @@ pub(crate) fn walk_directory( !excluded_dirs_ref.is_match(relative_path) }; - let threads = - std::thread::available_parallelism().map_or(1, NonZeroUsize::get); let dua_walk = dua_core::walk( &absolute_root, - threads, + // As many threads as rayon's, so RAYON_NUM_THREADS limits the walk too + rayon::current_num_threads(), Order::Completion, Options::default().skip_metadata(), read_directory, @@ -208,7 +229,7 @@ pub(crate) fn walk_directory( // release has `Options::queued_batches_per_worker`, set that instead // (https://github.com/Byron/dua-cli/pull/413). let (sender, walk) = std::sync::mpsc::channel(); - let receiving = std::thread::spawn(move || { + let draining = std::thread::spawn(move || { for entry in dua_walk { if sender.send(entry).is_err() { break; @@ -220,10 +241,9 @@ pub(crate) fn walk_directory( // package.yml there, so which package.yml each file belongs to is // settled once the walk is done let mut directories: Vec = Vec::new(); - let mut package_ymls_in_directories: HashMap = + let mut package_ymls_in_directories: HashMap = HashMap::new(); - let mut included_files_in_directories: Vec<(PathBuf, DirectoryId)> = - Vec::new(); + let mut included_files_in_directories: Vec<(PathBuf, usize)> = Vec::new(); for entry in walk { // I was using this to explore what directories were being walked to potentially @@ -241,18 +261,21 @@ pub(crate) fn walk_directory( // Couldn't read a directory or entry. Being consistent with packwerk, which swallows this error and continues continue; }; - if entry.depth > 0 && is_hidden(&entry.file_name) { - continue; - } + // Every directory, hidden or not, so that any file's directory has a + // package.yml to look up + let parent = entry.parent_directory_id.map(DirectoryId::index); if let Some(id) = entry.directory_id { directories.push(Directory { - id, - parent: entry.parent_directory_id, + id: id.index(), + parent, depth: entry.depth, }); continue; } + if is_hidden(&entry.file_name) { + continue; + } let absolute_path = entry.path(); @@ -263,9 +286,10 @@ pub(crate) fn walk_directory( continue; } - let directory = entry - .parent_directory_id - .expect("every entry but the root is in a directory"); + // Only the root isn't in a directory, and only when it's a file + let Some(directory) = parent else { + continue; + }; let is_package_yml = entry.file_name == "package.yml"; if is_package_yml { package_ymls_in_directories @@ -284,7 +308,6 @@ pub(crate) fn walk_directory( } } - // A package.yml is the one the files in its directory belong to if is_package_yml // Ideally, we don't need the second part of this conditional, but it's here // because there is a bug where the root pack doesn't match package_paths. @@ -314,27 +337,18 @@ pub(crate) fn walk_directory( } } - if let Err(panic) = receiving.join() { + if let Err(panic) = draining.join() { std::panic::resume_unwind(panic); } - // By depth, so each directory's parent has its package.yml first - directories.sort_unstable_by_key(|directory| directory.depth); - let mut package_ymls: HashMap = HashMap::new(); - for directory in directories { - let package_yml = match package_ymls_in_directories.get(&directory.id) { - Some(package_yml) => package_yml.clone(), - None => match directory.parent { - Some(parent) => package_ymls[&parent].clone(), - None => PathBuf::from("package.yml"), - }, - }; - package_ymls.insert(directory.id, package_yml); - } - for (absolute_path, directory) in included_files_in_directories { - owning_package_yml_for_file - .insert(absolute_path, package_ymls[&directory].clone()); - } + let package_ymls = + package_ymls_of_directories(directories, &package_ymls_in_directories); + let owning_package_yml_for_file = included_files_in_directories + .into_iter() + .map(|(absolute_path, directory)| { + (absolute_path, package_ymls[&directory].clone()) + }) + .collect(); debug!("Finished directory walk"); @@ -347,13 +361,53 @@ pub(crate) fn walk_directory( #[cfg(test)] mod tests { - use std::path::PathBuf; + use std::{collections::HashMap, path::PathBuf}; use crate::packs::{ raw_configuration::RawConfiguration, walk_directory::walk_directory, }; - use super::{build_gitignore_matcher, get_global_gitignore}; + use super::{ + build_gitignore_matcher, get_global_gitignore, + package_ymls_of_directories, Directory, + }; + + #[test] + fn test_package_ymls_of_directories_yielded_before_their_parents() { + // packs/a and packs/a/b/c have a package.yml, but the root and + // packs/a/b don't + let directories = [(4, 3), (3, 2), (2, 1), (1, 0)] + .into_iter() + .map(|(id, parent)| Directory { + id, + parent: Some(parent), + depth: id, + }) + .chain([Directory { + id: 0, + parent: None, + depth: 0, + }]) + .collect(); + let package_ymls_in_directories = HashMap::from([ + (2, PathBuf::from("packs/a/package.yml")), + (4, PathBuf::from("packs/a/b/c/package.yml")), + ]); + + assert_eq!( + package_ymls_of_directories( + directories, + &package_ymls_in_directories + ), + HashMap::from([ + (0, PathBuf::from("package.yml")), + (1, PathBuf::from("package.yml")), + (2, PathBuf::from("packs/a/package.yml")), + (3, PathBuf::from("packs/a/package.yml")), + (4, PathBuf::from("packs/a/b/c/package.yml")), + ]) + ); + } #[test] fn test_walk_directory() -> anyhow::Result<()> { diff --git a/tests/list_included_files_test.rs b/tests/list_included_files_test.rs index 3ec0b5d..9d189a4 100644 --- a/tests/list_included_files_test.rs +++ b/tests/list_included_files_test.rs @@ -1,3 +1,4 @@ +use assert_cmd::cargo::cargo_bin_cmd; use std::{collections::BTreeSet, error::Error, fs}; mod common; @@ -79,13 +80,17 @@ fn test_list_included_files_doesnt_descend_into_a_symlinked_directory( ) -> Result<(), Box> { use std::os::unix::fs::symlink; - // As packwerk's globs don't. Nor can a link back up the tree make pks - // list the same files over and over. + // As packwerk's globs don't, nor list one named like a Ruby file. Nor can + // a link back up the tree make pks list the same files over and over. let fixture = common::Fixture::new("simple_app"); let outside = tempfile::tempdir()?; fs::create_dir_all(outside.path().join("app"))?; fs::write(outside.path().join("app/outside.rb"), "")?; symlink(outside.path(), fixture.path("packs/linked"))?; + symlink( + outside.path(), + fixture.path("packs/foo/app/services/directory.rb"), + )?; symlink("..", fixture.path("packs/foo/loop"))?; let files = included_files(&fixture)?; @@ -96,6 +101,7 @@ fn test_list_included_files_doesnt_descend_into_a_symlinked_directory( ); assert!( !files.iter().any(|file| file.starts_with("packs/linked/") + || file.starts_with("packs/foo/app/services/directory.rb") || file.starts_with("packs/foo/loop/")), "{:?}", files @@ -103,3 +109,18 @@ fn test_list_included_files_doesnt_descend_into_a_symlinked_directory( Ok(()) } + +#[test] +fn test_list_included_files_with_a_file_for_the_project_root( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.path("packs/foo/app/services/foo.rb")) + .arg("list-included-files") + .assert() + .code(2) + .stderr(predicates::str::contains("No root pack found")); + + Ok(()) +}