diff --git a/CHANGELOG.md b/CHANGELOG.md index c79df78d..0748649a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,11 +9,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `libcnb-package`: + - Added the `output::create_packaged_buildpack_dir_resolver` helper which contains all the information on how compiled buildpack directories are structured returns a function that can be invoked with `BuildpackId` to produce the output path for a buildpack. ([#632](https://github.com/heroku/libcnb.rs/pull/632)) + ### Changed - `libcnb-package`: - Added `find_cargo_workspace_root_dir` which provides a convenient starting point for locating buildpacks for packaging and testing purposes. ([#629](https://github.com/heroku/libcnb.rs/pull/629)) - Changed the `ReadBuildpackDataError` and `ReadBuildpackageDataError` enums from struct to tuple format to be consistent with other error enums in the package. ([#631](https://github.com/heroku/libcnb.rs/pull/631)) + - Changed `buildpack_dependency::rewrite_buildpackage_local_dependencies` to accept a `&BuildpackOutputDirectoryLocator` instead of `&HashMap<&BuildpackId, PathBuf>`. ([#632](https://github.com/heroku/libcnb.rs/pull/632)) + - Moved `default_buildpack_directory_name` to `output::default_buildpack_directory_name`. ([#632](https://github.com/heroku/libcnb.rs/pull/632)) + +### Removed + +- `libcnb-package`: + - `get_buildpack_package_dir` has been removed in favor of `output::create_packaged_buildpack_dir_resolver` for building output paths to compiled buildpacks. ([#632](https://github.com/heroku/libcnb.rs/pull/632)) ## [0.14.0] - 2023-08-18 diff --git a/libcnb-cargo/src/package/command.rs b/libcnb-cargo/src/package/command.rs index ab6bcf48..48d01be7 100644 --- a/libcnb-cargo/src/package/command.rs +++ b/libcnb-cargo/src/package/command.rs @@ -11,16 +11,25 @@ use libcnb_package::buildpack_dependency::{ use libcnb_package::buildpack_package::{read_buildpack_package, BuildpackPackage}; use libcnb_package::cross_compile::{cross_compile_assistance, CrossCompileAssistance}; use libcnb_package::dependency_graph::{create_dependency_graph, get_dependencies}; +use libcnb_package::output::create_packaged_buildpack_dir_resolver; use libcnb_package::{ - assemble_buildpack_directory, find_buildpack_dirs, find_cargo_workspace_root_dir, - get_buildpack_package_dir, CargoProfile, + assemble_buildpack_directory, find_buildpack_dirs, find_cargo_workspace_root_dir, CargoProfile, }; -use std::collections::HashMap; +use std::ffi::OsString; use std::path::{Path, PathBuf}; type Result = std::result::Result; +#[allow(clippy::too_many_lines)] pub(crate) fn execute(args: &PackageArgs) -> Result<()> { + let target_triple = args.target.clone(); + + let cargo_profile = if args.release { + CargoProfile::Release + } else { + CargoProfile::Dev + }; + eprintln!("🔍 Locating buildpacks..."); let current_dir = std::env::current_dir().map_err(Error::GetCurrentDir)?; @@ -43,19 +52,6 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<()> { let buildpack_packages_graph = create_dependency_graph(buildpack_packages)?; - let target_directories_index = buildpack_packages_graph - .node_weights() - .map(|buildpack_package| { - let id = buildpack_package.buildpack_id(); - let target_dir = if contains_buildpack_binaries(&buildpack_package.path) { - buildpack_package.path.clone() - } else { - get_buildpack_package_dir(id, &package_dir, args.release, &args.target) - }; - (id, target_dir) - }) - .collect::>(); - let buildpack_packages_requested = buildpack_packages_graph .node_weights() .filter(|buildpack_package| { @@ -75,13 +71,15 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<()> { let build_order = get_dependencies(&buildpack_packages_graph, &buildpack_packages_requested)?; + let packaged_buildpack_dir_resolver = + create_packaged_buildpack_dir_resolver(&package_dir, cargo_profile, &target_triple); + let lookup_target_dir = |buildpack_package: &BuildpackPackage| { - target_directories_index - .get(&buildpack_package.buildpack_id()) - .ok_or(Error::TargetDirectoryLookup { - buildpack_id: buildpack_package.buildpack_id().clone(), - }) - .map(std::clone::Clone::clone) + if contains_buildpack_binaries(&buildpack_package.path) { + buildpack_package.path.clone() + } else { + packaged_buildpack_dir_resolver(buildpack_package.buildpack_id()) + } }; let mut current_count = 1; @@ -91,17 +89,27 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<()> { "📦 [{current_count}/{total_count}] Building {}", buildpack_package.buildpack_id() ); - let target_dir = lookup_target_dir(buildpack_package)?; + let target_dir = lookup_target_dir(buildpack_package); match buildpack_package.buildpack_data.buildpack_descriptor { BuildpackDescriptor::Single(_) => { if contains_buildpack_binaries(&buildpack_package.path) { eprintln!("Not a libcnb.rs buildpack, nothing to compile..."); } else { - package_single_buildpack(buildpack_package, &target_dir, args)?; + package_single_buildpack( + buildpack_package, + &target_dir, + cargo_profile, + &target_triple, + args.no_cross_compile_assistance, + )?; } } BuildpackDescriptor::Meta(_) => { - package_meta_buildpack(buildpack_package, &target_dir, &target_directories_index)?; + package_meta_buildpack( + buildpack_package, + &target_dir, + &packaged_buildpack_dir_resolver, + )?; } } current_count += 1; @@ -111,14 +119,14 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<()> { build_order .into_iter() .map(lookup_target_dir) - .collect::>>()?, + .collect::>(), ); print_requested_buildpack_output_dirs( buildpack_packages_requested .into_iter() .map(lookup_target_dir) - .collect::>>()?, + .collect::>(), ); Ok(()) @@ -127,16 +135,10 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<()> { fn package_single_buildpack( buildpack_package: &BuildpackPackage, target_dir: &Path, - args: &PackageArgs, + cargo_profile: CargoProfile, + target_triple: &str, + no_cross_compile_assistance: bool, ) -> Result<()> { - let cargo_profile = if args.release { - CargoProfile::Release - } else { - CargoProfile::Dev - }; - - let target_triple = &args.target; - let cargo_metadata = MetadataCommand::new() .manifest_path(&buildpack_package.path.join("Cargo.toml")) .exec() @@ -145,25 +147,7 @@ fn package_single_buildpack( source: e, })?; - let cargo_build_env = if args.no_cross_compile_assistance { - vec![] - } else { - eprintln!("Determining automatic cross-compile settings..."); - match cross_compile_assistance(target_triple) { - CrossCompileAssistance::Configuration { cargo_env } => cargo_env, - - CrossCompileAssistance::NoAssistance => { - eprintln!("Could not determine automatic cross-compile settings for target triple {target_triple}."); - eprintln!("This is not an error, but without proper cross-compile settings in your Cargo manifest and locally installed toolchains, compilation might fail."); - eprintln!("To disable this warning, pass --no-cross-compile-assistance."); - vec![] - } - - CrossCompileAssistance::HelpText(help_text) => { - Err(Error::CrossCompilationHelp { message: help_text })? - } - } - }; + let cargo_build_env = get_cargo_build_env(target_triple, no_cross_compile_assistance)?; eprintln!("Building binaries ({target_triple})..."); @@ -198,7 +182,7 @@ fn package_single_buildpack( fn package_meta_buildpack( buildpack_package: &BuildpackPackage, target_dir: &Path, - target_dirs_by_buildpack_id: &HashMap<&BuildpackId, PathBuf>, + packaged_buildpack_dir_resolver: &impl Fn(&BuildpackId) -> PathBuf, ) -> Result<()> { eprintln!("Writing buildpack directory..."); @@ -219,7 +203,7 @@ fn package_meta_buildpack( .map(|buildpackage_data| &buildpackage_data.buildpackage_descriptor) .ok_or(Error::MissingBuildpackageData) .and_then(|buildpackage| { - rewrite_buildpackage_local_dependencies(buildpackage, target_dirs_by_buildpack_id) + rewrite_buildpackage_local_dependencies(buildpackage, packaged_buildpack_dir_resolver) .map_err(std::convert::Into::into) }) .and_then(|buildpackage| { @@ -319,6 +303,31 @@ fn contains_buildpack_binaries(dir: &Path) -> bool { .all(|path| path.is_file()) } +fn get_cargo_build_env( + target_triple: &str, + no_cross_compile_assistance: bool, +) -> Result> { + if no_cross_compile_assistance { + Ok(vec![]) + } else { + eprintln!("Determining automatic cross-compile settings..."); + match cross_compile_assistance(target_triple) { + CrossCompileAssistance::Configuration { cargo_env } => Ok(cargo_env), + + CrossCompileAssistance::NoAssistance => { + eprintln!("Could not determine automatic cross-compile settings for target triple {target_triple}."); + eprintln!("This is not an error, but without proper cross-compile settings in your Cargo manifest and locally installed toolchains, compilation might fail."); + eprintln!("To disable this warning, pass --no-cross-compile-assistance."); + Ok(vec![]) + } + + CrossCompileAssistance::HelpText(help_text) => { + Err(Error::CrossCompilationHelp(help_text))? + } + } + } +} + fn get_default_package_dir(workspace_root_path: &Path) -> Result { MetadataCommand::new() .manifest_path(&workspace_root_path.join("Cargo.toml")) diff --git a/libcnb-cargo/src/package/error.rs b/libcnb-cargo/src/package/error.rs index e5d89c51..4d636af4 100644 --- a/libcnb-cargo/src/package/error.rs +++ b/libcnb-cargo/src/package/error.rs @@ -28,11 +28,8 @@ pub(crate) enum Error { #[error("Could not create package directory: {0}\nError: {1}")] CreatePackageDirectory(PathBuf, std::io::Error), - #[error("Could not determine a target directory for buildpack with id `{buildpack_id}`")] - TargetDirectoryLookup { buildpack_id: BuildpackId }, - - #[error("{message}")] - CrossCompilationHelp { message: String }, + #[error("{0}")] + CrossCompilationHelp(String), #[error("No environment variable named `CARGO` is set")] GetCargoBin(#[from] std::env::VarError), @@ -79,9 +76,6 @@ pub(crate) enum Error { #[error("Failed to locate buildpack with id `{0}`")] BuildpackPackagesLookup(BuildpackId), - #[error("Failed to lookup target directory for dependency with id `{0}`")] - RewriteLocalDependencyTargetDirectoryLookup(BuildpackId), - #[error("Could not convert path into buildpackage file uri: {0}")] InvalidPathDependency(PathBuf), @@ -201,9 +195,6 @@ impl From> for Error { impl From for Error { fn from(value: RewriteBuildpackageLocalDependenciesError) -> Self { match value { - RewriteBuildpackageLocalDependenciesError::TargetDirectoryLookup(id) => { - Error::RewriteLocalDependencyTargetDirectoryLookup(id) - } RewriteBuildpackageLocalDependenciesError::InvalidDependency(path) => { Error::InvalidPathDependency(path) } diff --git a/libcnb-cargo/tests/integration_test.rs b/libcnb-cargo/tests/integration_test.rs index 68a7b20a..887772b9 100644 --- a/libcnb-cargo/tests/integration_test.rs +++ b/libcnb-cargo/tests/integration_test.rs @@ -5,7 +5,8 @@ use libcnb_data::buildpack::BuildpackId; use libcnb_data::buildpack_id; use libcnb_data::buildpackage::BuildpackageDependency; -use libcnb_package::{read_buildpack_data, read_buildpackage_data}; +use libcnb_package::output::create_packaged_buildpack_dir_resolver; +use libcnb_package::{read_buildpack_data, read_buildpackage_data, CargoProfile}; use std::io::ErrorKind; use std::path::{Path, PathBuf}; use std::process::Command; @@ -26,7 +27,7 @@ fn package_buildpack_in_single_buildpack_project() { let packaged_buildpack_dir = create_packaged_buildpack_dir_resolver( &fixture_dir.path().join(DEFAULT_PACKAGE_DIR_NAME), - true, + CargoProfile::Release, X86_64_UNKNOWN_LINUX_MUSL, )(&buildpack_id); @@ -51,7 +52,7 @@ fn package_single_meta_buildpack_in_monorepo_buildpack_project() { let packaged_buildpack_dir_resolver = create_packaged_buildpack_dir_resolver( &fixture_dir.path().join(DEFAULT_PACKAGE_DIR_NAME), - true, + CargoProfile::Release, X86_64_UNKNOWN_LINUX_MUSL, ); @@ -111,7 +112,7 @@ fn package_single_buildpack_in_monorepo_buildpack_project() { let packaged_buildpack_dir = create_packaged_buildpack_dir_resolver( &fixture_dir.path().join(DEFAULT_PACKAGE_DIR_NAME), - true, + CargoProfile::Release, X86_64_UNKNOWN_LINUX_MUSL, )(&buildpack_id); @@ -141,7 +142,7 @@ fn package_all_buildpacks_in_monorepo_buildpack_project() { let packaged_buildpack_dir_resolver = create_packaged_buildpack_dir_resolver( &fixture_dir.path().join(DEFAULT_PACKAGE_DIR_NAME), - true, + CargoProfile::Release, X86_64_UNKNOWN_LINUX_MUSL, ); @@ -274,22 +275,6 @@ fn validate_packaged_meta_buildpack( ); } -fn create_packaged_buildpack_dir_resolver( - package_dir: &Path, - release: bool, - target_triple: &str, -) -> impl Fn(&BuildpackId) -> PathBuf { - let package_dir = PathBuf::from(package_dir); - let target_triple = target_triple.to_string(); - - move |buildpack_id| { - package_dir - .join(&target_triple) - .join(if release { "release" } else { "debug" }) - .join(buildpack_id.as_str().replace('/', "_")) - } -} - fn copy_fixture_to_temp_dir(name: &str) -> Result { let fixture_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("fixtures") diff --git a/libcnb-package/src/buildpack_dependency.rs b/libcnb-package/src/buildpack_dependency.rs index 67fa33b8..43e581ff 100644 --- a/libcnb-package/src/buildpack_dependency.rs +++ b/libcnb-package/src/buildpack_dependency.rs @@ -1,7 +1,5 @@ use libcnb_data::buildpack::{BuildpackId, BuildpackIdError}; use libcnb_data::buildpackage::{Buildpackage, BuildpackageDependency}; -use std::collections::HashMap; -use std::hash::BuildHasher; use std::path::{Path, PathBuf}; /// Buildpack dependency type @@ -80,9 +78,9 @@ pub fn get_local_buildpackage_dependencies( /// * the given `buildpackage` contains a local dependency with an invalid [`BuildpackId`] /// * there is no entry found in `buildpack_ids_to_target_dir` for a local dependency's [`BuildpackId`] /// * the target path for a local dependency is an invalid URI -pub fn rewrite_buildpackage_local_dependencies( +pub fn rewrite_buildpackage_local_dependencies( buildpackage: &Buildpackage, - buildpack_ids_to_target_dir: &HashMap<&BuildpackId, PathBuf, S>, + packaged_buildpack_dir_resolver: &impl Fn(&BuildpackId) -> PathBuf, ) -> Result { let local_dependency_to_target_dir = |target_dir: &PathBuf| { BuildpackageDependency::try_from(target_dir.clone()).map_err(|_| { @@ -99,14 +97,10 @@ pub fn rewrite_buildpackage_local_dependencies( BuildpackDependency::External(buildpackage_dependency) => { Ok(buildpackage_dependency) } - BuildpackDependency::Local(buildpack_id, _) => buildpack_ids_to_target_dir - .get(&buildpack_id) - .ok_or( - RewriteBuildpackageLocalDependenciesError::TargetDirectoryLookup( - buildpack_id, - ), - ) - .and_then(local_dependency_to_target_dir), + BuildpackDependency::Local(buildpack_id, _) => { + let output_dir = packaged_buildpack_dir_resolver(&buildpack_id); + local_dependency_to_target_dir(&output_dir) + } }) .collect() }) @@ -120,7 +114,6 @@ pub fn rewrite_buildpackage_local_dependencies( /// An error for [`rewrite_buildpackage_local_dependencies`] #[derive(Debug)] pub enum RewriteBuildpackageLocalDependenciesError { - TargetDirectoryLookup(BuildpackId), InvalidDependency(PathBuf), GetBuildpackDependenciesError(BuildpackIdError), } @@ -186,11 +179,12 @@ mod tests { get_local_buildpackage_dependencies, rewrite_buildpackage_local_dependencies, rewrite_buildpackage_relative_path_dependencies_to_absolute, }; + use crate::output::create_packaged_buildpack_dir_resolver; + use crate::CargoProfile; use libcnb_data::buildpack_id; use libcnb_data::buildpackage::{ Buildpackage, BuildpackageBuildpackReference, BuildpackageDependency, Platform, }; - use std::collections::HashMap; use std::path::PathBuf; #[test] @@ -209,17 +203,19 @@ mod tests { #[test] fn test_rewrite_buildpackage_local_dependencies() { let buildpackage = create_buildpackage(); - let buildpack_id = buildpack_id!("buildpack-id"); - let buildpack_ids_to_target_dir = HashMap::from([( - &buildpack_id, - PathBuf::from("/path/to/target/buildpacks/buildpack-id"), - )]); - let new_buildpackage = - rewrite_buildpackage_local_dependencies(&buildpackage, &buildpack_ids_to_target_dir) - .unwrap(); + let packaged_buildpack_dir_resolver = create_packaged_buildpack_dir_resolver( + &PathBuf::from("/path/to/target"), + CargoProfile::Dev, + "arch", + ); + let new_buildpackage = rewrite_buildpackage_local_dependencies( + &buildpackage, + &packaged_buildpack_dir_resolver, + ) + .unwrap(); assert_eq!( new_buildpackage.dependencies[0].uri.to_string(), - "/path/to/target/buildpacks/buildpack-id" + "/path/to/target/arch/debug/buildpack-id" ); } diff --git a/libcnb-package/src/lib.rs b/libcnb-package/src/lib.rs index e60c447b..8f6f55f9 100644 --- a/libcnb-package/src/lib.rs +++ b/libcnb-package/src/lib.rs @@ -10,6 +10,7 @@ pub mod buildpack_package; pub mod config; pub mod cross_compile; pub mod dependency_graph; +pub mod output; use crate::build::BuildpackBinaries; use libcnb_data::buildpack::{BuildpackDescriptor, BuildpackId}; @@ -188,15 +189,6 @@ fn create_file_symlink, Q: AsRef>( std::os::windows::fs::symlink_file(original.as_ref(), link.as_ref()) } -/// Construct a good default filename for a buildpack directory. -/// -/// This function ensures the resulting name is valid and does not contain problematic characters -/// such as `/`. -#[must_use] -pub fn default_buildpack_directory_name(buildpack_id: &BuildpackId) -> String { - buildpack_id.replace('/', "_") -} - /// Recursively walks the file system from the given `start_dir` to locate any folders containing a /// `buildpack.toml` file. /// @@ -248,7 +240,7 @@ pub fn get_buildpack_package_dir( package_dir .join(target_triple) .join(if is_release { "release" } else { "debug" }) - .join(default_buildpack_directory_name(buildpack_id)) + .join(output::default_buildpack_directory_name(buildpack_id)) } /// Returns the path of the root workspace directory for a Rust Cargo project. This is often a useful diff --git a/libcnb-package/src/output.rs b/libcnb-package/src/output.rs new file mode 100644 index 00000000..17d55272 --- /dev/null +++ b/libcnb-package/src/output.rs @@ -0,0 +1,65 @@ +use crate::CargoProfile; +use libcnb_data::buildpack::BuildpackId; +use std::path::{Path, PathBuf}; + +/// Create a function that can construct the output location for a buildpack. +pub fn create_packaged_buildpack_dir_resolver( + package_dir: &Path, + cargo_profile: CargoProfile, + target_triple: &str, +) -> impl Fn(&BuildpackId) -> PathBuf { + let package_dir = PathBuf::from(package_dir); + let target_triple = target_triple.to_string(); + + move |buildpack_id| { + package_dir + .join(&target_triple) + .join(match cargo_profile { + CargoProfile::Dev => "debug", + CargoProfile::Release => "release", + }) + .join(default_buildpack_directory_name(buildpack_id)) + } +} + +/// Construct a good default filename for a buildpack directory. +/// +/// This function ensures the resulting name is valid and does not contain problematic characters +/// such as `/`. +#[must_use] +pub fn default_buildpack_directory_name(buildpack_id: &BuildpackId) -> String { + buildpack_id.replace('/', "_") +} + +#[cfg(test)] +mod tests { + use crate::output::create_packaged_buildpack_dir_resolver; + use crate::CargoProfile; + use libcnb_data::buildpack_id; + use std::path::PathBuf; + + #[test] + fn test_get_buildpack_target_dir() { + let buildpack_id = buildpack_id!("some-org/with-buildpack"); + let package_dir = PathBuf::from("/package"); + let target_triple = "x86_64-unknown-linux-musl"; + + let dev_packaged_buildpack_dir_resolver = + create_packaged_buildpack_dir_resolver(&package_dir, CargoProfile::Dev, target_triple); + + let release_packaged_buildpack_dir_resolver = create_packaged_buildpack_dir_resolver( + &package_dir, + CargoProfile::Release, + target_triple, + ); + + assert_eq!( + dev_packaged_buildpack_dir_resolver(&buildpack_id), + PathBuf::from("/package/x86_64-unknown-linux-musl/debug/some-org_with-buildpack") + ); + assert_eq!( + release_packaged_buildpack_dir_resolver(&buildpack_id), + PathBuf::from("/package/x86_64-unknown-linux-musl/release/some-org_with-buildpack") + ); + } +}