Skip to content

Commit

Permalink
Allow participating buildpacks to opt-out of Node.js build scripts
Browse files Browse the repository at this point in the history
The buildpacks that performs installation for each of the supported package managers (npm, pnpm, Yarn) will execute a preset list of scripts from `package.json` if present:
- `heroku-prebuild`
- `heroku-build` or `build`
- `heroku-postbuild`

This PR adds a new Buildpack Plan called `node_build_scripts` to the package manager installation buildpacks that can be configured by later buildpacks that require it with the following:

```toml
[[requires]]
name = "node_build_scripts"

  [requires.metadata]
  enabled = <bool>
```
  • Loading branch information
colincasey committed Sep 25, 2024
1 parent 93413f4 commit 0985640
Show file tree
Hide file tree
Showing 17 changed files with 582 additions and 34 deletions.
80 changes: 80 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions buildpacks/nodejs-npm-install/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ use commons::output::fmt;
use commons::output::fmt::DEBUG_INFO;
use fun_run::CmdError;
use heroku_nodejs_utils::application;
use heroku_nodejs_utils::buildplan::{
NodeBuildScriptsMetadataError, NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME,
};
use heroku_nodejs_utils::package_json::PackageJsonError;
use indoc::formatdoc;
use std::fmt::Display;
Expand All @@ -27,6 +30,7 @@ pub(crate) enum NpmInstallBuildpackError {
NpmSetCacheDir(CmdError),
NpmVersion(npm::VersionError),
PackageJson(PackageJsonError),
NodeBuildScriptsMetadata(NodeBuildScriptsMetadataError),
}

pub(crate) fn on_error(error: libcnb::Error<NpmInstallBuildpackError>) {
Expand All @@ -44,13 +48,36 @@ fn on_buildpack_error(error: NpmInstallBuildpackError, logger: Box<dyn StartedLo
NpmInstallBuildpackError::Application(e) => on_application_error(&e, logger),
NpmInstallBuildpackError::BuildScript(e) => on_build_script_error(&e, logger),
NpmInstallBuildpackError::Detect(e) => on_detect_error(&e, logger),
NpmInstallBuildpackError::NodeBuildScriptsMetadata(e) => {
on_node_build_scripts_metadata_error(e, logger);
}
NpmInstallBuildpackError::NpmInstall(e) => on_npm_install_error(&e, logger),
NpmInstallBuildpackError::NpmSetCacheDir(e) => on_set_cache_dir_error(&e, logger),
NpmInstallBuildpackError::NpmVersion(e) => on_npm_version_error(e, logger),
NpmInstallBuildpackError::PackageJson(e) => on_package_json_error(e, logger),
}
}

fn on_node_build_scripts_metadata_error(
error: NodeBuildScriptsMetadataError,
logger: Box<dyn StartedLogger>,
) {
let NodeBuildScriptsMetadataError::InvalidEnabledValue(value) = error;
let value_type = value.type_str();
logger.announce().error(&formatdoc! { "
A participating buildpack has set invalid `[requires.metadata]` for the build plan \
named `{NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME}`.
Expected metadata format:
[requires.metadata]
enabled = <bool>
But was:
[requires.metadata]
enabled = <{value_type}>
"});
}

fn on_package_json_error(error: PackageJsonError, logger: Box<dyn StartedLogger>) {
match error {
PackageJsonError::AccessError(e) => {
Expand Down
44 changes: 32 additions & 12 deletions buildpacks/nodejs-npm-install/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ use commons::output::section_log::{log_step, log_step_stream};
use commons::output::warn_later::WarnGuard;
use fun_run::{CommandWithName, NamedOutput};
use heroku_nodejs_utils::application;
use heroku_nodejs_utils::buildplan::{
read_node_build_scripts_metadata, NodeBuildScriptsMetadata, NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME,
};
use heroku_nodejs_utils::package_json::PackageJson;
use heroku_nodejs_utils::package_manager::PackageManager;
use heroku_nodejs_utils::vrs::Version;
Expand Down Expand Up @@ -53,9 +56,11 @@ impl Buildpack for NpmInstallBuildpack {
.build_plan(
BuildPlanBuilder::new()
.provides("node_modules")
.provides(NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME)
.requires("node")
.requires("npm")
.requires("node_modules")
.requires("node")
.requires(NODE_BUILD_SCRIPTS_BUILD_PLAN_NAME)
.build(),
)
.build()
Expand All @@ -74,6 +79,8 @@ impl Buildpack for NpmInstallBuildpack {
let app_dir = &context.app_dir;
let package_json = PackageJson::read(app_dir.join("package.json"))
.map_err(NpmInstallBuildpackError::PackageJson)?;
let node_build_scripts_metadata = read_node_build_scripts_metadata(&context.buildpack_plan)
.map_err(NpmInstallBuildpackError::NodeBuildScriptsMetadata)?;

run_application_checks(app_dir, &warn_later)?;

Expand All @@ -84,7 +91,12 @@ impl Buildpack for NpmInstallBuildpack {
let logger = section.end_section();

let section = logger.section("Running scripts");
run_build_scripts(&package_json, &env, section.as_ref())?;
run_build_scripts(
&package_json,
&node_build_scripts_metadata,
&env,
section.as_ref(),
)?;
let logger = section.end_section();

let section = logger.section("Configuring default processes");
Expand Down Expand Up @@ -154,6 +166,7 @@ fn run_npm_install(

fn run_build_scripts(
package_json: &PackageJson,
node_build_scripts_metadata: &NodeBuildScriptsMetadata,
env: &Env,
_section_logger: &dyn SectionLogger,
) -> Result<(), NpmInstallBuildpackError> {
Expand All @@ -162,16 +175,23 @@ fn run_build_scripts(
log_step("No build scripts found");
} else {
for script in build_scripts {
let mut npm_run = npm::RunScript { env, script }.into_command();
log_step_stream(
format!("Running {}", fmt::command(npm_run.name())),
|stream| {
npm_run
.stream_output(stream.io(), stream.io())
.and_then(NamedOutput::nonzero_captured)
.map_err(NpmInstallBuildpackError::BuildScript)
},
)?;
if let Some(false) = node_build_scripts_metadata.enabled {
log_step(format!(
"Not running {} as it was disabled by a participating buildpack",
fmt::value(script)
));
} else {
let mut npm_run = npm::RunScript { env, script }.into_command();
log_step_stream(
format!("Running {}", fmt::command(npm_run.name())),
|stream| {
npm_run
.stream_output(stream.io(), stream.io())
.and_then(NamedOutput::nonzero_captured)
.map_err(NpmInstallBuildpackError::BuildScript)
},
)?;
}
}
};
Ok(())
Expand Down
57 changes: 54 additions & 3 deletions buildpacks/nodejs-npm-install/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
// Required due to: https://github.com/rust-lang/rust-clippy/issues/11119
#![allow(clippy::unwrap_used)]

use libcnb_test::{assert_contains, assert_not_contains, PackResult};
use indoc::indoc;
use libcnb::data::buildpack_id;
use libcnb_test::{assert_contains, assert_not_contains, BuildpackReference, PackResult};
use serde_json::json;
use std::path::Path;
use test_support::{
add_build_script, add_package_json_dependency, nodejs_integration_test,
nodejs_integration_test_with_config, update_json_file,
add_build_script, add_package_json_dependency, custom_buildpack, integration_test_with_config,
nodejs_integration_test, nodejs_integration_test_with_config, update_json_file,
};

#[test]
Expand Down Expand Up @@ -218,6 +220,55 @@ fn test_native_modules_are_recompiled_even_on_cache_restore() {
);
}

#[test]
#[ignore = "integration test"]
fn test_skip_build_scripts_from_buildplan() {
integration_test_with_config(
"./fixtures/npm-project",
|config| {
config.app_dir_preprocessor(|app_dir| {
add_build_script(&app_dir, "heroku-prebuild");
add_build_script(&app_dir, "build");
add_build_script(&app_dir, "heroku-postbuild");
});
},
|ctx| {
assert_contains!(
ctx.pack_stdout,
"Not running `heroku-prebuild` as it was disabled by a participating buildpack"
);
assert_contains!(
ctx.pack_stdout,
"Not running `build` as it was disabled by a participating buildpack"
);
assert_contains!(
ctx.pack_stdout,
"Not running `heroku-postbuild` as it was disabled by a participating buildpack"
);
},
&[
BuildpackReference::WorkspaceBuildpack(buildpack_id!("heroku/nodejs")),
BuildpackReference::Other(
custom_buildpack()
.id("test/skip-build-scripts")
.detect(indoc! { r#"
#!/usr/bin/env bash
build_plan="$2"
cat <<EOF >"$build_plan"
[[requires]]
name = "node_build_scripts"
[requires.metadata]
enabled = false
EOF
"# })
.call(),
),
],
);
}

fn add_lockfile_entry(app_dir: &Path, package_name: &str, lockfile_entry: serde_json::Value) {
update_json_file(&app_dir.join("package-lock.json"), |json| {
let dependencies = json["dependencies"].as_object_mut().unwrap();
Expand Down
Loading

0 comments on commit 0985640

Please sign in to comment.