Skip to content

Commit

Permalink
Fix error messages for buildpack packaging failures
Browse files Browse the repository at this point in the history 
After #666, incorrect error messages were shown for failures that
occurred during buildpack compilation packaging, since the
handling in `TestRunner::build_internal` used the wrong message
text, plus didn't include the error struct in the `panic!` string.

These have been fixed, plus display representations added to
all of the new error variants.

In addition, the usages of `assert!` and `.unwrap()` in functions that
return `Result` have been replaced with new error enum variants.

The change in output can be seen in the new tests added by #717.

There is one last scenario that doesn't yet have a test - the testing
of the cross-compilation output itself. However, I'll add that separately,
since it's going to be more complex to test / there are a few different
options that we'll need to choose between.

Fixes #704.
Fixes #709.
Fixes #710.
GUS-W-14395170.
  • Loading branch information
@edmorley
edmorley committed Nov 6, 2023
1 parent 5ba2781 commit 9504a2c
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 42 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- `libcnb-test`:
- Fixed incorrect error messages being shown for buildpack compilation/packaging failures. ([#720](https://github.com/heroku/libcnb.rs/pull/720))

## [0.15.0] - 2023-09-25

Expand Down
1 change: 1 addition & 0 deletions libcnb-test/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ libcnb-common.workspace = true
libcnb-data.workspace = true
libcnb-package.workspace = true
tempfile = "3.8.0"
thiserror = "1.0.48"

[dev-dependencies]
indoc = "2.0.4"
Expand Down
55 changes: 30 additions & 25 deletions libcnb-test/src/build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use libcnb_package::dependency_graph::{get_dependencies, GetDependenciesError};
use libcnb_package::output::create_packaged_buildpack_dir_resolver;
use libcnb_package::{find_cargo_workspace_root_dir, CargoProfile, FindCargoWorkspaceRootError};
use std::collections::BTreeMap;
use std::fs;
use std::path::{Path, PathBuf};
use std::{fs, io};

/// Packages the current crate as a buildpack into the provided directory.
// TODO: Convert the `assert!` usages to an additional `PackageBuildpackError` variant instead:
Expand All @@ -23,11 +23,11 @@ pub(crate) fn package_crate_buildpack(
) -> Result<PathBuf, PackageBuildpackError> {
let buildpack_toml = cargo_manifest_dir.join("buildpack.toml");

assert!(
buildpack_toml.exists(),
"Could not package directory as buildpack! No `buildpack.toml` file exists at {}",
cargo_manifest_dir.display()
);
if !buildpack_toml.exists() {
return Err(PackageBuildpackError::BuildpackDescriptorNotFound(
buildpack_toml,
));
}

let buildpack_descriptor: BuildpackDescriptor = read_toml_file(buildpack_toml)
.map_err(PackageBuildpackError::CannotReadBuildpackDescriptor)?;
Expand All @@ -53,7 +53,7 @@ pub(crate) fn package_buildpack(
) -> Result<PathBuf, PackageBuildpackError> {
let cargo_build_env = match cross_compile_assistance(target_triple.as_ref()) {
CrossCompileAssistance::HelpText(help_text) => {
return Err(PackageBuildpackError::CrossCompileConfigurationError(
return Err(PackageBuildpackError::CrossCompileToolchainNotFound(
help_text,
));
}
Expand All @@ -75,28 +75,21 @@ pub(crate) fn package_buildpack(

let root_node = buildpack_dependency_graph
.node_weights()
.find(|node| node.buildpack_id == buildpack_id.clone());
.find(|node| &node.buildpack_id == buildpack_id)
.ok_or_else(|| {
PackageBuildpackError::BuildpackIdNotFound(buildpack_id.clone(), workspace_root_path)
})?;

assert!(
root_node.is_some(),
"Could not package directory as buildpack! No buildpack with id `{buildpack_id}` exists in the workspace at {}",
workspace_root_path.display()
);

let build_order = get_dependencies(
&buildpack_dependency_graph,
&[root_node.expect("The root node should exist")],
)
.map_err(PackageBuildpackError::GetDependencies)?;
let build_order = get_dependencies(&buildpack_dependency_graph, &[root_node])
.map_err(PackageBuildpackError::GetDependencies)?;

let mut packaged_buildpack_dirs = BTreeMap::new();
for node in &build_order {
let buildpack_destination_dir = buildpack_dir_resolver(&node.buildpack_id);

// TODO: Convert the `unwrap()` to an additional `PackageBuildpackError` variant instead:
// https://github.com/heroku/libcnb.rs/issues/710
#[allow(clippy::unwrap_used)]
fs::create_dir_all(&buildpack_destination_dir).unwrap();
fs::create_dir_all(&buildpack_destination_dir).map_err(|error| {
PackageBuildpackError::CannotCreateDirectory(buildpack_destination_dir.clone(), error)
})?;

libcnb_package::package::package_buildpack(
&node.path,
Expand All @@ -114,12 +107,24 @@ pub(crate) fn package_buildpack(
Ok(buildpack_dir_resolver(buildpack_id))
}

#[derive(Debug)]
#[derive(thiserror::Error, Debug)]
pub(crate) enum PackageBuildpackError {
#[error("Couldn't find a buildpack.toml file at {0}")]
BuildpackDescriptorNotFound(PathBuf),
#[error("Couldn't find a buildpack with ID '{0}' in the workspace at {1}")]
BuildpackIdNotFound(BuildpackId, PathBuf),
#[error("I/O error creating directory {0}: {1}")]
CannotCreateDirectory(PathBuf, io::Error),
#[error("Couldn't read buildpack.toml: {0}")]
CannotReadBuildpackDescriptor(TomlFileError),
#[error("Couldn't calculate buildpack dependency graph: {0}")]
BuildBuildpackDependencyGraph(BuildBuildpackDependencyGraphError),
CrossCompileConfigurationError(String),
#[error("Couldn't find cross-compilation toolchain.\n\n{0}")]
CrossCompileToolchainNotFound(String),
#[error("Couldn't find Cargo workspace root: {0}")]
FindCargoWorkspaceRoot(FindCargoWorkspaceRootError),
#[error("Couldn't get buildpack dependencies: {0}")]
GetDependencies(GetDependenciesError<BuildpackId>),
#[error("{0}")]
PackageBuildpack(libcnb_package::package::PackageBuildpackError),
}
12 changes: 9 additions & 3 deletions libcnb-test/src/test_runner.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,10 @@ impl TestRunner {
&config.target_triple,
&cargo_manifest_dir,
buildpacks_target_dir.path(),
).expect("Test references crate buildpack, but crate wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences");
)
.unwrap_or_else(|error| {
panic!("Error packaging current crate as buildpack: {error}")
});
pack_command.buildpack(crate_buildpack_dir);
}

Expand All @@ -117,8 +120,11 @@ impl TestRunner {
config.cargo_profile,
&config.target_triple,
&cargo_manifest_dir,
buildpacks_target_dir.path()
).unwrap_or_else(|_| panic!("Test references buildpack `{buildpack_id}`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences"));
buildpacks_target_dir.path(),
)
.unwrap_or_else(|error| {
panic!("Error packaging buildpack '{buildpack_id}': {error}")
});
pack_command.buildpack(buildpack_dir);
}

Expand Down
26 changes: 12 additions & 14 deletions libcnb-test/tests/integration_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,12 @@ fn packaging_failure_missing_buildpack_toml() {
assert_eq!(
err.downcast_ref::<String>().unwrap(),
&format!(
"Could not package directory as buildpack! No `buildpack.toml` file exists at {}",
env::var("CARGO_MANIFEST_DIR").unwrap()
"Error packaging current crate as buildpack: Couldn't find a buildpack.toml file at {}",
env::var("CARGO_MANIFEST_DIR")
.map(PathBuf::from)
.unwrap()
.join("buildpack.toml")
.display()
)
);
}
Expand All @@ -157,7 +161,7 @@ fn packaging_failure_missing_buildpack_toml() {
// TODO: Fix the implementation to return the correct error message (that the buildpack.toml doesn't match the schema):
// https://github.com/heroku/libcnb.rs/issues/708
#[should_panic(
expected = "Could not package directory as buildpack! No buildpack with id `libcnb-test/invalid-buildpack-toml` exists in the workspace at"
expected = "Error packaging buildpack 'libcnb-test/invalid-buildpack-toml': Couldn't find a buildpack with ID 'libcnb-test/invalid-buildpack-toml' in the workspace at"
)]
fn packaging_failure_invalid_buildpack_toml() {
TestRunner::default().build(
Expand All @@ -174,10 +178,8 @@ fn packaging_failure_invalid_buildpack_toml() {

#[test]
#[ignore = "integration test"]
// TODO: Fix the implementation to return the correct error message. Has the same cause as:
// https://github.com/heroku/libcnb.rs/issues/704
#[should_panic(
expected = "Test references buildpack `libcnb-test/composite-missing-package-toml`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences"
expected = "Error packaging buildpack 'libcnb-test/composite-missing-package-toml': Could not read package.toml: IO error while reading/writing TOML file: No such file or directory (os error 2)"
)]
fn packaging_failure_composite_buildpack_missing_package_toml() {
TestRunner::default().build(
Expand All @@ -194,10 +196,8 @@ fn packaging_failure_composite_buildpack_missing_package_toml() {

#[test]
#[ignore = "integration test"]
// TODO: Fix the implementation to return the correct error message. Has the same cause as:
// https://github.com/heroku/libcnb.rs/issues/704
#[should_panic(
expected = "Test references buildpack `libcnb-test/invalid-cargo-toml`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences"
expected = "Error packaging buildpack 'libcnb-test/invalid-cargo-toml': Obtaining Cargo metadata failed: `cargo metadata` exited with an error: "
)]
fn packaging_failure_invalid_cargo_toml() {
TestRunner::default().build(
Expand All @@ -212,10 +212,8 @@ fn packaging_failure_invalid_cargo_toml() {

#[test]
#[ignore = "integration test"]
// TODO: Fix the implementation to return the correct error message. Has the same cause as:
// https://github.com/heroku/libcnb.rs/issues/704
#[should_panic(
expected = "Test references buildpack `libcnb-test/compile-error`, but this directory wasn't packaged as a buildpack. This is an internal libcnb-test error, please report any occurrences"
expected = "Error packaging buildpack 'libcnb-test/compile-error': Building buildpack binaries failed: Failed to build binary target compile-error: Cargo unexpectedly exited with status exit status: 101"
)]
fn packaging_failure_compile_error() {
TestRunner::default().build(
Expand Down Expand Up @@ -246,7 +244,7 @@ fn packaging_failure_non_existent_workspace_buildpack() {
assert_eq!(
err.downcast_ref::<String>().unwrap(),
&format!(
"Could not package directory as buildpack! No buildpack with id `non-existent` exists in the workspace at {}",
"Error packaging buildpack 'non-existent': Couldn't find a buildpack with ID 'non-existent' in the workspace at {}",
// There is currently no env var for determining the workspace root directly:
// https://github.com/rust-lang/cargo/issues/3946
env::var("CARGO_MANIFEST_DIR")
Expand Down Expand Up @@ -320,7 +318,7 @@ fn expected_pack_failure() {
#[test]
#[ignore = "integration test"]
#[should_panic(
expected = "Could not package directory as buildpack! No `buildpack.toml` file exists at"
expected = "Error packaging current crate as buildpack: Couldn't find a buildpack.toml file"
)]
fn expected_pack_failure_still_panics_for_non_pack_failure() {
TestRunner::default().build(
Expand Down

0 comments on commit 9504a2c

Please sign in to comment.