Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Cleanup of non-deterministic files in Node.js, npm, and pnpm layers to prevent unnecessary invalidation of runtime layers for export. ([#1274](https://github.com/heroku/buildpacks-nodejs/pull/1274))

## [5.3.5] - 2026-01-27

### Added
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ toml = "0.9"
toml_edit = "0.23"
tracing = "0.1"
yaml-rust2 = "0.10"
walkdir = "2.5.0"

[dev-dependencies]
insta = "1"
Expand Down
7 changes: 0 additions & 7 deletions crates/test_support/src/snapshot_filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,13 +326,6 @@ pub(super) fn create_snapshot_filters() -> Vec<(String, String)> {
"${1}<NODE-GYP BUILD OUTPUT>",
));

// TODO: investigate why the `...native_modules_are_recompiled...`-based tests (mostly the pnpm version)
// occassionally switch from Adding layer 'heroku/nodejs:xyz' to Reusing layer 'heroku/nodejs:xyz'
filters.push((
r"(?:Adding|Reusing) layer 'heroku/nodejs:virtual",
"<add_or_reusing> layer 'heroku/nodejs:virtual",
));

filters
.into_iter()
.map(|(matcher, replacement)| (matcher.to_string(), replacement.to_string()))
Expand Down
36 changes: 36 additions & 0 deletions src/context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use crate::NodeJsBuildpack;
use crate::layer_cleanup::LayerCleanupTarget;
use std::cell::RefCell;
use std::ops::Deref;

/// Wrapper around libcnb `BuildContext` that tracks layers needing cleanup
/// of non-deterministic build artifacts (Python bytecode, Makefiles)
pub(crate) struct NodeJsBuildContext {
inner: libcnb::build::BuildContext<NodeJsBuildpack>,
cleanup_registry: RefCell<Vec<LayerCleanupTarget>>,
}

impl NodeJsBuildContext {
pub(crate) fn new(inner: libcnb::build::BuildContext<NodeJsBuildpack>) -> Self {
Self {
inner,
cleanup_registry: RefCell::new(Vec::new()),
}
}

pub(crate) fn register_layer_for_cleanup(&self, target: LayerCleanupTarget) {
self.cleanup_registry.borrow_mut().push(target);
}

pub(crate) fn layers_to_cleanup(&self) -> Vec<LayerCleanupTarget> {
self.cleanup_registry.borrow().iter().cloned().collect()
}
}

impl Deref for NodeJsBuildContext {
type Target = libcnb::build::BuildContext<NodeJsBuildpack>;

fn deref(&self) -> &Self::Target {
&self.inner
}
}
98 changes: 98 additions & 0 deletions src/layer_cleanup.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
use bullet_stream::global::print;
use std::fs;
use std::path::{Path, PathBuf};
use walkdir::WalkDir;

#[derive(Debug, Clone)]
pub(crate) enum LayerKind {
/// pnpm virtual store layer (contains native module builds with Makefiles)
Virtual,
}

#[derive(Debug, Clone)]
pub(crate) struct LayerCleanupTarget {
pub(crate) path: PathBuf,
pub(crate) kind: LayerKind,
}

/// Remove Makefile and *.mk files from native module build directories
/// These files have non-deterministic dependency ordering causing layer invalidation
fn remove_build_makefiles(base_path: &Path) -> Result<usize, std::io::Error> {
let mut removed_count = 0;

// Walk directory tree looking for build/Makefile patterns
for entry in WalkDir::new(base_path)
.into_iter()
.filter_map(Result::ok)
.filter(|e| {
if !e.file_type().is_file() {
return false;
}

// Check if this is a Makefile or .mk file in a build/ directory
let path = e.path();
if let Some(parent) = path.parent()
&& parent.file_name() == Some(std::ffi::OsStr::new("build"))
&& let Some(filename) = path.file_name()
{
return filename.to_string_lossy() == "Makefile";
}

false
})
{
fs::remove_file(entry.path())?;
removed_count += 1;
}

Ok(removed_count)
}

/// Clean up non-deterministic build artifacts from a layer
pub(crate) fn cleanup_layer(target: &LayerCleanupTarget) -> Result<(), std::io::Error> {
let path = &target.path;

if !path.exists() {
// Layer doesn't exist, nothing to clean
return Ok(());
}

match target.kind {
LayerKind::Virtual => {
// pnpm virtual store: contains symlinked packages with native module builds
// Clean Makefiles from: virtual/store/*/node_modules/*/build/
print::bullet("Cleaning up pnpm virtual store layer");
let removed = remove_build_makefiles(path)?;
if removed > 0 {
print::sub_bullet(format!("Removed {removed} Makefile artifacts"));
}
}
}

Ok(())
}

#[cfg(test)]
mod tests {
use super::*;
use std::fs;
use tempfile::TempDir;

#[test]
fn test_remove_build_makefiles() {
let temp = TempDir::new().unwrap();
let base = temp.path();

// Create build directory with Makefile
let build_dir = base.join("node_modules/some-package/build");
fs::create_dir_all(&build_dir).unwrap();
fs::write(build_dir.join("Makefile"), b"makefile content").unwrap();
fs::write(build_dir.join("output.o"), b"binary").unwrap(); // Should not be removed

let removed = remove_build_makefiles(base).unwrap();

assert_eq!(removed, 1); // Makefile
assert!(!build_dir.join("Makefile").exists());
assert!(build_dir.join("output.o").exists()); // Not a makefile
}
}
21 changes: 19 additions & 2 deletions src/main.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::buildpack_config::{ConfigValue, ConfigValueSource};
use crate::context::NodeJsBuildContext;
use crate::layer_cleanup::cleanup_layer;
use crate::o11y::*;
use crate::package_manager::InstalledPackageManager;
use crate::utils::error_handling::{ErrorMessage, on_framework_error};
Expand All @@ -16,6 +18,8 @@ use libcnb_test as _;
use toml::Table;

mod buildpack_config;
mod context;
mod layer_cleanup;
mod o11y;
mod package_json;
mod package_manager;
Expand All @@ -25,7 +29,7 @@ mod runtimes;
mod utils;

type BuildpackDetectContext = libcnb::detect::DetectContext<NodeJsBuildpack>;
type BuildpackBuildContext = libcnb::build::BuildContext<NodeJsBuildpack>;
type BuildpackBuildContext = NodeJsBuildContext;
type BuildpackError = libcnb::Error<ErrorMessage>;
type BuildpackResult<T> = Result<T, BuildpackError>;

Expand Down Expand Up @@ -68,8 +72,11 @@ impl libcnb::Buildpack for NodeJsBuildpack {
#[allow(clippy::too_many_lines)]
fn build(
&self,
context: BuildpackBuildContext,
context: libcnb::build::BuildContext<NodeJsBuildpack>,
) -> libcnb::Result<libcnb::build::BuildResult, ErrorMessage> {
// Wrap the context to track layers needing cleanup
let context = NodeJsBuildContext::new(context);

let buildpack_start = print::buildpack(
context
.buildpack_descriptor
Expand Down Expand Up @@ -215,6 +222,16 @@ impl libcnb::Buildpack for NodeJsBuildpack {
}
}

// Clean up non-deterministic build artifacts from registered layers
let layers_to_cleanup = context.layers_to_cleanup();
if !layers_to_cleanup.is_empty() {
for layer_to_cleanup in layers_to_cleanup {
if let Err(e) = cleanup_layer(&layer_to_cleanup) {
print::sub_bullet(format!("- Error during cleanup: {e}"));
}
}
}

print::all_done(&Some(buildpack_start));

build_result_builder.store(store).build()
Expand Down
14 changes: 10 additions & 4 deletions src/package_managers/npm.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::utils::build_env::node_gyp_env;
use crate::utils::error_handling::ErrorType::Internal;
use crate::utils::error_handling::{
ErrorMessage, ErrorType, SuggestRetryBuild, SuggestSubmitIssue, error_message,
Expand Down Expand Up @@ -75,8 +76,8 @@ pub(crate) fn install_npm(
env,
npm_packument,
node_version,
)
.map_err(Into::into)
)?;
Ok(())
}

pub(crate) fn install_npm_dependencies(
Expand Down Expand Up @@ -105,8 +106,13 @@ pub(crate) fn install_npm_dependencies(
.named_output()
.map_err(|e| create_set_npm_cache_directory_command_error(&e))?;

print::sub_stream_cmd(Command::new("npm").args(["ci"]).envs(env))
.map_err(|e| create_npm_install_error(&e))?;
print::sub_stream_cmd(
Command::new("npm")
.args(["ci"])
.envs(env)
.envs(node_gyp_env()),
)
.map_err(|e| create_npm_install_error(&e))?;

Ok(())
}
Expand Down
15 changes: 12 additions & 3 deletions src/package_managers/pnpm.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::layer_cleanup::{LayerCleanupTarget, LayerKind};
use crate::utils::build_env::node_gyp_env;
use crate::utils::error_handling::{
ErrorMessage, ErrorType, SuggestRetryBuild, SuggestSubmitIssue, error_message, file_value,
};
Expand Down Expand Up @@ -44,8 +46,8 @@ pub(crate) fn install_pnpm(
env,
pnpm_packument,
node_version,
)
.map_err(Into::into)
)?;
Ok(())
}

pub(crate) fn install_dependencies(
Expand All @@ -66,7 +68,8 @@ pub(crate) fn install_dependencies(
print::sub_stream_cmd(
Command::new("pnpm")
.args(["install", "--frozen-lockfile"])
.envs(env),
.envs(env)
.envs(node_gyp_env()),
)
.map_err(|e| create_pnpm_install_command_error(&e))?;

Expand Down Expand Up @@ -182,6 +185,12 @@ fn create_virtual_store_directory(context: &BuildpackBuildContext) -> BuildpackR
Err(create_node_modules_symlink_error(&error))?;
}

// Register virtual layer for cleanup of non-deterministic Makefiles
context.register_layer_for_cleanup(LayerCleanupTarget {
path: pnpm_virtual_store_layer.path().clone(),
kind: LayerKind::Virtual,
});

Ok(virtual_store_dir)
}

Expand Down
7 changes: 5 additions & 2 deletions src/package_managers/yarn.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::o11y::*;
use crate::utils::build_env::node_gyp_env;
use crate::utils::error_handling::{
ErrorMessage, ErrorType, SuggestRetryBuild, SuggestSubmitIssue, error_message, file_value,
};
Expand Down Expand Up @@ -57,14 +58,15 @@ pub(crate) fn install_yarn(
yarn_packument: &PackagePackument,
node_version: &Version,
) -> BuildpackResult<()> {
// Note: yarn layer path is returned but not used for cleanup registration
utils::npm_registry::install_package_layer(
layer_name!("yarn"),
context,
env,
yarn_packument,
node_version,
)
.map_err(Into::into)
)?;
Ok(())
}

#[instrument(skip_all)]
Expand Down Expand Up @@ -283,6 +285,7 @@ pub(crate) fn install_dependencies(
print::bullet("Installing dependencies");
let mut yarn_install_command = Command::new("yarn");
yarn_install_command.envs(env);
yarn_install_command.envs(node_gyp_env());
yarn_install_command.arg("install");
if version.major() == 1 {
yarn_install_command.args(["--production=false", "--frozen-lockfile"]);
Expand Down
11 changes: 11 additions & 0 deletions src/utils/build_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,14 @@ pub(crate) fn set_default_env_var(

Ok(())
}

pub(crate) fn node_gyp_env() -> Vec<(String, String)> {
vec![
// If this is set to a non-empty string, Python won’t try to write .pyc files on the import of source modules.
// see https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE
//
// This is used to prevent node-gyp from generating files which invalidate runtime layers whenever
// native modules are compiled as `node-gyp` uses Python as part of the compilation process.
("PYTHONDONTWRITEBYTECODE".to_string(), "1".to_string()),
]
}
6 changes: 3 additions & 3 deletions src/utils/npm_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use libcnb::layer_env::Scope;
use serde::{Deserialize, Serialize};
use std::collections::HashMap;
use std::fs::File;
use std::path::Path;
use std::path::{Path, PathBuf};
use std::{fs, io};

const NPMJS_ORG_HOST: &str = "https://registry.npmjs.org";
Expand Down Expand Up @@ -335,7 +335,7 @@ pub(crate) fn install_package_layer(
env: &mut Env,
package_packument: &PackagePackument,
node_version: &Version,
) -> Result<(), InstallPackageLayerError> {
) -> Result<PathBuf, InstallPackageLayerError> {
let package_name = &package_packument.name;
let package_version = &package_packument.version;

Expand Down Expand Up @@ -433,7 +433,7 @@ pub(crate) fn install_package_layer(

env.clone_from(&layer_env.apply(Scope::Build, env));

Ok(())
Ok(install_package_layer.path().clone())
}

pub(crate) enum InstallPackageLayerError {
Expand Down
Loading