npm: set npm_config_build_from_source=true to handle prebuildify packages#1392
npm: set npm_config_build_from_source=true to handle prebuildify packages#1392Gitjay11 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly adds the npm_config_build_from_source=true environment variable to force npm to build native addons from source, which is a good step towards ensuring hermetic builds. My review includes a couple of suggestions to improve code maintainability by reducing magic strings and code duplication in tests, in line with the repository's style guide.
There was a problem hiding this comment.
Code Review
This pull request correctly implements the proposed solution to force npm packages with native addons to be built from source by setting the npm_config_build_from_source environment variable. This is a good improvement for ensuring hermetic builds.
I've added a couple of suggestions to improve maintainability by reducing the use of magic strings and avoiding code duplication in tests, in line with the repository's style guide.
| env_vars = [ | ||
| EnvironmentVariable( | ||
| name="npm_config_build_from_source", | ||
| value="true", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
To improve readability and maintainability, it's better to define hardcoded strings like environment variable names as module-level constants. This avoids magic strings and makes the configuration easier to manage.
You could add the following at the top of the file:
NPM_BUILD_FROM_SOURCE_ENV_VAR = "npm_config_build_from_source"And then use it here:
env_vars = [
EnvironmentVariable(
name=NPM_BUILD_FROM_SOURCE_ENV_VAR,
value="true",
),
]References
- The style guide emphasizes maintaining readable code. (link)
| "environment_variables": [ | ||
| EnvironmentVariable( | ||
| name="npm_config_build_from_source", | ||
| value="true", | ||
| ), | ||
| ], |
There was a problem hiding this comment.
There's some code duplication here and in the next test case (lines 889-894) for creating the EnvironmentVariable object. The repository style guide encourages reducing code duplication.
To follow the DRY (Don't Repeat Yourself) principle, you could define this as a constant at the top of the test file or within the test class and reuse it in both parameterized tests.
For example:
BUILD_FROM_SOURCE_ENV_VAR = EnvironmentVariable(
name="npm_config_build_from_source",
value="true",
)
# ... inside the test parameterization
"environment_variables": [BUILD_FROM_SOURCE_ENV_VAR],References
- The style guide recommends detecting and flagging code duplication. (link)
|
|
||
| env_vars = [ | ||
| EnvironmentVariable( | ||
| name="npm_config_build_from_source", |
There was a problem hiding this comment.
When this triggers node-gyp, won't it try to fetch Node.js headers from nodejs.org at build time?
How would that work in a hermetic/offline build?
There was a problem hiding this comment.
Actually when node-gyp compiles from source, it needs Node.js C/C++ headers. However node-gyp first checks for locally available headers (via the Node.js installations include/ directory or the npm_config_nodedir env var) before attempting to download from nodejs.org. In typical hermetic build environments (e.g., Konflux), Node.js is pre-installed in the base image with headers already available, so node-gyp finds them locally without needing network access.
well, happy to add a note in the code or docs if it's needed.
farhann-saleem
left a comment
There was a problem hiding this comment.
Does yarn / yarn-classic need the same treatment? node-gyp-build checks this env var regardless of the package manager.
|
I'd say second option is necessary since it adds visibility. Users who are fine with binaries could then set the variable to true then. |
|
@farhann-saleem Yes, that's right. node-gyp-build checks this env var regardless of the package manager. I'll update this PR to add the same env var to yarn and yarn-classic as well. edit : I have added the changes now. |
|
@a-ovchinnikov Yeah, I know solution 2 adds important visibility. This PR was scoped to Solution 1 as a minimal first step since the issue says either or both. I will implement Solution 2 soon and raise a follow-up PR for it. edit: I have raised a follow up PR for solution 2. |
bb390b7 to
222c0bc
Compare
|
@Gitjay11 are you going to still work on this? There are merge conflicts to resolve. |
@eskultety yes, I have resolved the conflicts. |
…ages (hermetoproject#1015) Signed-off-by: Gitjay11 <newajay.11r@gmail.com> Assisted-by: Gemini
222c0bc to
ddf4012
Compare
| import pytest | ||
|
|
||
| from hermeto import APP_NAME | ||
| from hermeto.core.models.output import EnvironmentVariable, RequestOutput |
| @mock.patch( | ||
| "hermeto.core.package_managers.javascript.npm.main.create_backend_annotation", | ||
| return_value=None, | ||
| ) | ||
| @mock.patch("hermeto.core.package_managers.javascript.npm.main._resolve_npm") | ||
| def test_fetch_npm_source_sets_build_from_source_env_var( |
There was a problem hiding this comment.
I think this unit test is in contradiction with what we are trying to do in #1588
fixes #1015
Description
Resolves the first proposed solution from #1015.
Packages using prebuildify ship precompiled native
.nodebinaries under aprebuilds/directory. At install time,node-gyp-buildchecks for thenpm_config_build_from_sourceenvironment variable — if set, it compiles native addons from source instead of using the prebuilt binaries.Without this fix, hermetic builds silently use prebuilt platform-specific binaries, undermining the hermetic build guarantee.
This PR injects
npm_config_build_from_source=trueinto the build configuration output for all npm packages, ensuring native addons are always compiled from source.How to test
As noted in the issue, we should do either or both of the proposed solutions. This PR implements Solution 1 as a standalone fix. Solution 2 (detecting and gating prebuilt packages) can be added as a follow-up PR if the maintainers agree it's needed.