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..8be8202 100644 --- a/src/packs/walk_directory.rs +++ b/src/packs/walk_directory.rs @@ -1,7 +1,8 @@ +use dua_core::{DirectoryId, Entry, Options, Order}; use ignore::gitignore::{Gitignore, GitignoreBuilder}; -use jwalk::WalkDirGeneric; use std::{ collections::{HashMap, HashSet}, + ffi::OsStr, path::{Path, PathBuf}, sync::Arc, }; @@ -17,15 +18,39 @@ pub struct WalkDirectoryResult { pub owning_package_yml_for_file: HashMap, } -#[derive(Debug, Default, Clone)] -struct ProcessReadDirState { - current_package_yml: PathBuf, +/// 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('.')) } -impl jwalk::ClientState for ProcessReadDirState { - type ReadDirState = ProcessReadDirState; +/// A directory the walk found, by `DirectoryId::index` +struct Directory { + id: usize, + parent: Option, + depth: usize, +} - type DirEntryState = ProcessReadDirState; +/// 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`). @@ -90,20 +115,18 @@ 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 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, @@ -112,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 = [ @@ -161,73 +182,70 @@ 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 read_directory = move |entry: &Entry| { + if entry.depth == 0 { + return true; + } + if is_hidden(&entry.file_name) { + return false; + } - 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 absolute_dirname = entry.path(); + // 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 + if let Some(gitignore) = gitignore_ref.as_ref() { + if gitignore.matched(relative_path, true).is_ignore() { + return false; + } + } + + !excluded_dirs_ref.is_match(relative_path) + }; - for entry in walk_dir { + let dua_walk = dua_core::walk( + &absolute_root, + // 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, + ); + + // 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. 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 draining = 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, usize)> = Vec::new(); + + 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 +257,44 @@ 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; + }; + + // 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: id.index(), + parent, + depth: entry.depth, + }); + continue; + } + if is_hidden(&entry.file_name) { 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(); + // 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 + .insert(directory, absolute_path.clone()); + } let relative_path = absolute_path .strip_prefix(&absolute_root) @@ -267,10 +308,7 @@ pub(crate) fn walk_directory( } } - let current_package_yml = - &unwrapped_entry.client_state.current_package_yml; - - if &absolute_path == current_package_yml + 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 +324,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 +337,19 @@ pub(crate) fn walk_directory( } } + if let Err(panic) = draining.join() { + std::panic::resume_unwind(panic); + } + + 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"); Ok(WalkDirectoryResult { @@ -311,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 new file mode 100644 index 0000000..9d189a4 --- /dev/null +++ b/tests/list_included_files_test.rs @@ -0,0 +1,126 @@ +use assert_cmd::cargo::cargo_bin_cmd; +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 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)?; + 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/app/services/directory.rb") + || file.starts_with("packs/foo/loop/")), + "{:?}", + files + ); + + 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(()) +}