From d149b153ff3fe1215ff3d1b09500e125c2c24d33 Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Wed, 13 Sep 2023 14:34:48 -0300 Subject: [PATCH 1/4] Ignore file support when finding buildpacks Previously we could rely on knowing that packaged buildpacks would be written to a location within the `target` directory configured in Crate. This was easy to determine through program code and could be supplied to `find_buildpack_dirs` so that packaged buildpacks weren't accidentally included when searching for buildpacks. With the changes introduced in [#583](https://github.com/heroku/libcnb.rs/pull/583) it is no longer possible to easily determine the output location for packaged buildpacks. This PR adds the [ignore](https://crates.io/crates/ignore) crate to provide directory iteration that respects standard ignore files. This will give the user the ability to configure where we search within a project for buildpacks. --- CHANGELOG.md | 2 + libcnb-cargo/src/package/command.rs | 5 +-- libcnb-package/Cargo.toml | 1 + .../src/buildpack_dependency_graph.rs | 5 +-- libcnb-package/src/lib.rs | 38 +++++-------------- 5 files changed, 17 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 692dad2d..db59f78a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `libcnb-cargo`: - No longer outputs paths for non-libcnb.rs and non-meta buildpacks. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) - Build output for humans changed slightly, output intended for machines/scripting didn't change. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) +- `libcnb-package` + - dropped `ignore` argument from `find_buildpack_dirs` that allowed for paths to be filtered out of the directory traversal. This function will now respect standard ignore files (like `.gitignore`) while traversing. ## [0.14.0] - 2023-08-18 diff --git a/libcnb-cargo/src/package/command.rs b/libcnb-cargo/src/package/command.rs index 668dbb4b..351e0854 100644 --- a/libcnb-cargo/src/package/command.rs +++ b/libcnb-cargo/src/package/command.rs @@ -61,9 +61,8 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<(), Error> { }; eprintln!("šŸ—ļø Building buildpack dependency graph..."); - let buildpack_dependency_graph = - build_libcnb_buildpacks_dependency_graph(&workspace_root_path, &[&package_dir]) - .map_err(Error::CannotBuildBuildpackDependencyGraph)?; + let buildpack_dependency_graph = build_libcnb_buildpacks_dependency_graph(&workspace_root_path) + .map_err(Error::CannotBuildBuildpackDependencyGraph)?; eprintln!("šŸ”€ Determining build order..."); let root_nodes = buildpack_dependency_graph diff --git a/libcnb-package/Cargo.toml b/libcnb-package/Cargo.toml index 2f136e9c..f60779e5 100644 --- a/libcnb-package/Cargo.toml +++ b/libcnb-package/Cargo.toml @@ -13,6 +13,7 @@ include = ["src/**/*", "LICENSE", "README.md"] [dependencies] cargo_metadata = "0.17.0" +ignore = "0.4" libcnb-common.workspace = true libcnb-data.workspace = true petgraph = { version = "0.6.3", default-features = false } diff --git a/libcnb-package/src/buildpack_dependency_graph.rs b/libcnb-package/src/buildpack_dependency_graph.rs index 5021cb4b..a696288d 100644 --- a/libcnb-package/src/buildpack_dependency_graph.rs +++ b/libcnb-package/src/buildpack_dependency_graph.rs @@ -27,9 +27,8 @@ use std::path::{Path, PathBuf}; /// package.toml or an IO error occurred while traversing the given directory. pub fn build_libcnb_buildpacks_dependency_graph( cargo_workspace_root: &Path, - ignore: &[&Path], ) -> Result, BuildBuildpackDependencyGraphError> { - find_buildpack_dirs(cargo_workspace_root, ignore) + find_buildpack_dirs(cargo_workspace_root) .map_err(BuildBuildpackDependencyGraphError::FindBuildpackDirectories) .and_then(|buildpack_directories| { buildpack_directories @@ -86,7 +85,7 @@ fn build_libcnb_buildpack_dependency_graph_node( #[derive(thiserror::Error, Debug)] pub enum BuildBuildpackDependencyGraphError { #[error("IO error while finding buildpack directories: {0}")] - FindBuildpackDirectories(std::io::Error), + FindBuildpackDirectories(ignore::Error), #[error("Cannot read buildpack descriptor: {0}")] ReadBuildpackDescriptorError(TomlFileError), #[error("Cannot read package descriptor: {0}")] diff --git a/libcnb-package/src/lib.rs b/libcnb-package/src/lib.rs index 1fc77eb1..855180b2 100644 --- a/libcnb-package/src/lib.rs +++ b/libcnb-package/src/lib.rs @@ -106,38 +106,20 @@ fn create_file_symlink, Q: AsRef>( /// /// # Errors /// -/// Will return an `Err` if any I/O errors happen while walking the file system. -pub fn find_buildpack_dirs(start_dir: &Path, ignore: &[&Path]) -> std::io::Result> { - fn find_buildpack_dirs_recursive( - path: &Path, - ignore: &[&Path], - accumulator: &mut Vec, - ) -> std::io::Result<()> { - if ignore.contains(&path) { - return Ok(()); - } - - let metadata = path.metadata()?; - if metadata.is_dir() { - let entries = fs::read_dir(path)?; - for entry in entries { - let entry = entry?; - let metadata = entry.metadata()?; - if metadata.is_dir() { - find_buildpack_dirs_recursive(&entry.path(), ignore, accumulator)?; - } else if let Some(file_name) = entry.path().file_name() { - if file_name.to_string_lossy() == "buildpack.toml" { - accumulator.push(path.to_path_buf()); - } +/// Will return an `Err` if any I/O errors happen while walking the file system or any parsing errors +/// from reading a gitignore file. +pub fn find_buildpack_dirs(start_dir: &Path) -> Result, ignore::Error> { + let mut buildpack_dirs = vec![]; + for result in ignore::Walk::new(start_dir) { + match result { + Ok(entry) => { + if entry.path().is_dir() && entry.path().join("buildpack.toml").exists() { + buildpack_dirs.push(entry.path().to_path_buf()); } } + Err(error) => return Err(error), } - - Ok(()) } - - let mut buildpack_dirs: Vec = vec![]; - find_buildpack_dirs_recursive(start_dir, ignore, &mut buildpack_dirs)?; Ok(buildpack_dirs) } From 83f8f49eb629cc4592d1e3d17b1862a4a6551fba Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Thu, 14 Sep 2023 11:22:29 -0300 Subject: [PATCH 2/4] PR feedback * updated error message * using iterator instead of loop for finding buildpack dirs logic * added a quick test to ensure that ignores work * updated changelog entry --- CHANGELOG.md | 3 +- libcnb-cargo/tests/integration_test.rs | 41 +++++++++++++++++++ .../src/buildpack_dependency_graph.rs | 2 +- libcnb-package/src/lib.rs | 26 ++++++------ 4 files changed, 57 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db59f78a..697d0e02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,8 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `libcnb-cargo`: - No longer outputs paths for non-libcnb.rs and non-meta buildpacks. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) - Build output for humans changed slightly, output intended for machines/scripting didn't change. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) -- `libcnb-package` - - dropped `ignore` argument from `find_buildpack_dirs` that allowed for paths to be filtered out of the directory traversal. This function will now respect standard ignore files (like `.gitignore`) while traversing. + - When performing buildpack detection, standard ignore files (`.ignore` and `.gitignore`) will be respected ## [0.14.0] - 2023-08-18 diff --git a/libcnb-cargo/tests/integration_test.rs b/libcnb-cargo/tests/integration_test.rs index 6abb7cfe..fe517167 100644 --- a/libcnb-cargo/tests/integration_test.rs +++ b/libcnb-cargo/tests/integration_test.rs @@ -219,6 +219,47 @@ fn package_command_error_when_run_in_project_with_no_buildpacks() { ); } +#[test] +#[ignore = "integration test"] +fn package_command_respects_ignore_files() { + let fixture_dir = copy_fixture_to_temp_dir("multiple_buildpacks").unwrap(); + + // when a git folder is not present, .ignore should be used + let ignore_file = fixture_dir.path().join(".ignore"); + fs::write(&ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap(); + + let output = Command::new(CARGO_LIBCNB_BINARY_UNDER_TEST) + .args(["libcnb", "package", "--release"]) + .current_dir(fixture_dir.path()) + .output() + .unwrap(); + + //assert_ne!(output.status.code(), Some(0)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "šŸšš Preparing package directory...\nšŸ–„\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\nšŸ—\u{fe0f} Building buildpack dependency graph...\nšŸ”€ Determining build order...\nāŒ No buildpacks found!\n" + ); + + fs::remove_file(ignore_file).unwrap(); + + // when a git folder is not present, .gitignore can be used + fs::create_dir(fixture_dir.path().join(".git")).unwrap(); + let ignore_file = fixture_dir.path().join(".gitignore"); + fs::write(ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap(); + + let output = Command::new(CARGO_LIBCNB_BINARY_UNDER_TEST) + .args(["libcnb", "package", "--release"]) + .current_dir(fixture_dir.path()) + .output() + .unwrap(); + + //assert_ne!(output.status.code(), Some(0)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "šŸšš Preparing package directory...\nšŸ–„\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\nšŸ—\u{fe0f} Building buildpack dependency graph...\nšŸ”€ Determining build order...\nāŒ No buildpacks found!\n" + ); +} + fn validate_packaged_buildpack(packaged_buildpack_dir: &Path, buildpack_id: &BuildpackId) { assert!(packaged_buildpack_dir.join("buildpack.toml").exists()); assert!(packaged_buildpack_dir.join("package.toml").exists()); diff --git a/libcnb-package/src/buildpack_dependency_graph.rs b/libcnb-package/src/buildpack_dependency_graph.rs index a696288d..d99b35f0 100644 --- a/libcnb-package/src/buildpack_dependency_graph.rs +++ b/libcnb-package/src/buildpack_dependency_graph.rs @@ -84,7 +84,7 @@ fn build_libcnb_buildpack_dependency_graph_node( #[derive(thiserror::Error, Debug)] pub enum BuildBuildpackDependencyGraphError { - #[error("IO error while finding buildpack directories: {0}")] + #[error("Error while finding buildpack directories: {0}")] FindBuildpackDirectories(ignore::Error), #[error("Cannot read buildpack descriptor: {0}")] ReadBuildpackDescriptorError(TomlFileError), diff --git a/libcnb-package/src/lib.rs b/libcnb-package/src/lib.rs index 855180b2..96aea209 100644 --- a/libcnb-package/src/lib.rs +++ b/libcnb-package/src/lib.rs @@ -109,18 +109,20 @@ fn create_file_symlink, Q: AsRef>( /// Will return an `Err` if any I/O errors happen while walking the file system or any parsing errors /// from reading a gitignore file. pub fn find_buildpack_dirs(start_dir: &Path) -> Result, ignore::Error> { - let mut buildpack_dirs = vec![]; - for result in ignore::Walk::new(start_dir) { - match result { - Ok(entry) => { - if entry.path().is_dir() && entry.path().join("buildpack.toml").exists() { - buildpack_dirs.push(entry.path().to_path_buf()); - } - } - Err(error) => return Err(error), - } - } - Ok(buildpack_dirs) + ignore::Walk::new(start_dir) + .collect::, _>>() + .map(|entries| { + entries + .iter() + .filter_map(|entry| { + if entry.path().is_dir() && entry.path().join("buildpack.toml").exists() { + Some(entry.path().to_path_buf()) + } else { + None + } + }) + .collect() + }) } /// Returns the path of the root workspace directory for a Rust Cargo project. This is often a useful From ad247ff5a32f7be5c41833ce3ffb5d0f8cdf464a Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Thu, 14 Sep 2023 11:30:07 -0300 Subject: [PATCH 3/4] PR feedback * updated error message * using iterator instead of loop for finding buildpack dirs logic * added a quick test to ensure that ignores work * updated changelog entry --- libcnb-cargo/tests/integration_test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcnb-cargo/tests/integration_test.rs b/libcnb-cargo/tests/integration_test.rs index fe517167..dbcbb740 100644 --- a/libcnb-cargo/tests/integration_test.rs +++ b/libcnb-cargo/tests/integration_test.rs @@ -234,7 +234,7 @@ fn package_command_respects_ignore_files() { .output() .unwrap(); - //assert_ne!(output.status.code(), Some(0)); + assert_ne!(output.status.code(), Some(0)); assert_eq!( String::from_utf8_lossy(&output.stderr), "šŸšš Preparing package directory...\nšŸ–„\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\nšŸ—\u{fe0f} Building buildpack dependency graph...\nšŸ”€ Determining build order...\nāŒ No buildpacks found!\n" @@ -253,7 +253,7 @@ fn package_command_respects_ignore_files() { .output() .unwrap(); - //assert_ne!(output.status.code(), Some(0)); + assert_ne!(output.status.code(), Some(0)); assert_eq!( String::from_utf8_lossy(&output.stderr), "šŸšš Preparing package directory...\nšŸ–„\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\nšŸ—\u{fe0f} Building buildpack dependency graph...\nšŸ”€ Determining build order...\nāŒ No buildpacks found!\n" From b5770d6a308c0b054d7c24d6043e66441d8514ef Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Mon, 18 Sep 2023 11:40:55 -0300 Subject: [PATCH 4/4] PR feedback * updated test comments * added PR link to changelog entry --- CHANGELOG.md | 2 +- libcnb-cargo/tests/integration_test.rs | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 697d0e02..45c19f93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `libcnb-cargo`: - No longer outputs paths for non-libcnb.rs and non-meta buildpacks. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) - Build output for humans changed slightly, output intended for machines/scripting didn't change. ([#657](https://github.com/heroku/libcnb.rs/pull/657)) - - When performing buildpack detection, standard ignore files (`.ignore` and `.gitignore`) will be respected + - When performing buildpack detection, standard ignore files (`.ignore` and `.gitignore`) will be respected. ([#673](https://github.com/heroku/libcnb.rs/pull/673)) ## [0.14.0] - 2023-08-18 diff --git a/libcnb-cargo/tests/integration_test.rs b/libcnb-cargo/tests/integration_test.rs index dbcbb740..093b07f4 100644 --- a/libcnb-cargo/tests/integration_test.rs +++ b/libcnb-cargo/tests/integration_test.rs @@ -224,7 +224,8 @@ fn package_command_error_when_run_in_project_with_no_buildpacks() { fn package_command_respects_ignore_files() { let fixture_dir = copy_fixture_to_temp_dir("multiple_buildpacks").unwrap(); - // when a git folder is not present, .ignore should be used + // The `ignore` crate supports `.ignore` files. So this first `cargo libcnb package` execution + // just sanity checks that our ignore rules will be respected. let ignore_file = fixture_dir.path().join(".ignore"); fs::write(&ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap(); @@ -242,7 +243,12 @@ fn package_command_respects_ignore_files() { fs::remove_file(ignore_file).unwrap(); - // when a git folder is not present, .gitignore can be used + // The `ignore` crate supports `.gitignore` files but only if the folder is within a git repository + // which is the default configuration used in our directory traversal. + // https://docs.rs/ignore/latest/ignore/struct.WalkBuilder.html#method.require_git + // + // So this second `cargo libcnb package` execution just sanity checks that our gitignore rules + // in a git repository will be respected. fs::create_dir(fixture_dir.path().join(".git")).unwrap(); let ignore_file = fixture_dir.path().join(".gitignore"); fs::write(ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap();