Skip to content

feat(npm): detect prebuilt binaries and gate them behind binary/allow_binary field#1415

Closed
Gitjay11 wants to merge 1 commit into
hermetoproject:mainfrom
Gitjay11:issue-1015-npm-prebuildify-detect
Closed

feat(npm): detect prebuilt binaries and gate them behind binary/allow_binary field#1415
Gitjay11 wants to merge 1 commit into
hermetoproject:mainfrom
Gitjay11:issue-1015-npm-prebuildify-detect

Conversation

@Gitjay11

Copy link
Copy Markdown

Description:

This PR implements Solution 2 for issue #1015 by detecting npm packages that ship with prebuilt .node binaries (such as those generated by prebuildify) and gating their installation based on the binary input field.

here is the solution 1 : #1392

Previously, some modules containing .node binaries were bypassing the hermetic build-from-source process. This implementation ensures that:

  • Npm input models now accept binary filters (via NpmBinaryFilters) and migrate legacy allow_binary inputs.
  • During _resolve_npm, downloaded .tgz tarballs are scanned for .node files within prebuilds/ directories.
  • If prebuilt binaries are detected, the package is rejected with a PackageRejected error unless the user has explicitly configured the binary field for that package.
  • It seamlessly mimics the existing binary filtering pattern used across the codebase (e.g., for bundler packages).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a feature to detect prebuilt .node binaries within npm packages and control their installation via a binary configuration field, similar to how it's handled for other package managers. The changes are well-structured, adding the necessary model updates in input.py, implementing the detection logic in npm.py by inspecting package tarballs, and including comprehensive unit tests. The implementation is sound and aligns with the project's goal of ensuring hermetic builds. I have one minor suggestion to improve code clarity by removing a redundant method call.

if not prebuilt_files:
continue

pkg_name = path.path.stem.removesuffix(".tgz")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of .removesuffix(".tgz") is redundant here. The pathlib.Path.stem property already returns the final path component without its last suffix. Using just .stem is cleaner and avoids redundancy.

Suggested change
pkg_name = path.path.stem.removesuffix(".tgz")
pkg_name = path.path.stem
References
  1. The repository style guide (line 20) requires flagging code redundancy. The use of .removesuffix(".tgz") is redundant because path.path.stem already provides the filename without the extension. (link)

@ST3V1K

ST3V1K commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Thank you for the contribution!
Could you please rebase this? Once that's done, we're ready to review it.

@eskultety

Copy link
Copy Markdown
Member

Closing due to inactivity.

@eskultety eskultety closed this Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants