Allow custom version of `npm`

c0d1ngm0nk3y commented 6 months ago

Fixes https://github.com/paketo-buildpacks/nodejs/issues/270

based on https://github.com/paketo-buildpacks/npm-install/pull/752

Summary

Instead of always using the npm installed by the node-engine buildpack, the user can specify BP_NPM_VERSION to change to the specified version.

Use Cases

See https://github.com/paketo-buildpacks/nodejs/issues/270 for details, but this allows users to use a specific version of npm rather than the one packaged with nodejs, e.g. in case of a known bug.

Details

If BP_NPM_VERSION is provided, it will install this version of npm first before installing the node_modules. It will be installed within node_modules (NOT globally).

Unfortunately, there are shells that do not allow binaries to the PATH, but only directories. So in order to only prepend this specific binary (see comment), we have to create a separate folder .bin_local and symlink the npm binary from .bin. So that .bin_local can be prepended to PATH in order to use it instead of the already installed npm. In order to make it accessible, it

Modifies the PATH env of the current process, so that the following npm call (e.g. npm install) will use this instead.
Modifies the PATH in the BuildEnv, so that other buildpacks use the same npm
Modifies the PATH in the LaunchEnv, so that other using npm at launch will use the same version.

Checklist

[X] I have viewed, signed, and submitted the Contributor License Agreement.
[X] I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
[x] I have added an integration test, if necessary.
[X] I have reviewed the styleguide for guidance on my code quality.
[X] I'm happy with the commit history on this PR (I have rebased/squashed as needed).

c0d1ngm0nk3y commented 6 months ago

Tests are still missing: Reason that the current BuildFunc is kind of hard to test and we didn't want to add any more parameters.

Idea: We would refactor BuildProcessResolver to ProcessResolver and instead of Resolve, there would be ResolveBuildProcess,ResolvePruneProcess and ResolveUpdateNPMProcess.

This would not only keep the BuildFunc testable, but also simplify the signature by reming the need for the PruneProcess.

@paketo-buildpacks/nodejs-maintainers WDYT?

c0d1ngm0nk3y commented 6 months ago

c0d1ngm0nk3y commented 5 months ago

@mhdawson We changed it to not install the global version. Hence we had to prepend the .bin to the PATH instead of append. Could you have another look?

c0d1ngm0nk3y commented 5 months ago

Integration tests are failing, so I guess I have to make them run on ARM :(

mhdawson commented 5 months ago

@c0d1ngm0nk3y made one suggestion otherwise it looks good to me.

mhdawson commented 5 months ago

I think some of the failures were related to the removal of support for 16.x, restarted the tests, although it might need a rebase to pick up the change I landed today for that.

mhdawson commented 5 months ago

@c0d1ngm0nk3y actually this was not one of the repos where I saw a failure and fixed. If you search for 16 in the tests and replace with 18 that may fix up some of the test failures.

Across a number of the repos the tests were coded to request Node.js 16 and since that was removed from node-engine the request needs to be updated to 18.

c0d1ngm0nk3y commented 5 months ago

needs to be updated to 18

I have opened https://github.com/paketo-buildpacks/npm-install/pull/713 for this and picked the commit in the meantime.

c0d1ngm0nk3y commented 5 months ago

The integration tests are driving me nuts. They are still failing for an unrelated reason. Seem that they are very fragile :(

pacostas commented 5 months ago

The integration tests are driving me nuts. They are still failing for an unrelated reason. Seem that they are very fragile :(

I think this error is due to too many requests on the github registry for fetching the buildpack images. Try rerunning them after 10-20 minutes

c0d1ngm0nk3y commented 4 months ago

The logger was fixed in packit, we have to wait for a release > 2.14.0 to be released and consumed.

mhdawson commented 4 months ago

@c0d1ngm0nk3y thanks for the update.

c0d1ngm0nk3y commented 4 months ago

https://github.com/paketo-buildpacks/packit/pull/581#issuecomment-2238670896

c0d1ngm0nk3y commented 3 months ago

@mhdawson Is there any way to get the ubi extension run on arm? Normally, I just skip the ubi builder locally, but the current error in the tests only occur for ubi.

When I run it locally, it fails with

            [extender (build)]   Resolving Node Engine version
            [extender (build)]   Node no longer requested by plan, satisfied by extension
            [extender (build)]   Setting up launch layer for environment variables
            [extender (build)]   Configuring build environment
            [extender (build)]     NODE_ENV     -> "production"
            [extender (build)]     NODE_HOME    -> ""
            [extender (build)]     NODE_OPTIONS -> "--use-openssl-ca"
            [extender (build)]     NODE_VERBOSE -> "false"
            [extender (build)]
            [extender (build)]   Configuring launch environment
            [extender (build)]     NODE_ENV     -> "production"
            [extender (build)]     NODE_HOME    -> ""
            [extender (build)]     NODE_OPTIONS -> "--use-openssl-ca"
            [extender (build)]     NODE_VERBOSE -> "false"
            [extender (build)]
            [extender (build)]     Writing exec.d/0-optimize-memory
            [extender (build)]       Calculates available memory based on container limits at launch time.
            [extender (build)]       Made available in the MEMORY_AVAILABLE environment variable.
            [extender (build)]     Writing exec.d/1-inspector
            [extender (build)]
            [extender (build)] Paketo Buildpack for NPM Install 1.2.3
            [extender (build)]   Resolving installation process
            [extender (build)]     Process inputs:
            [extender (build)]       node_modules      -> "Found"
            [extender (build)]       npm-cache         -> "Found"
            [extender (build)]       package-lock.json -> "Found"
            [extender (build)]
            [extender (build)]     Selected NPM build process: 'npm ci'
            [extender (build)]
            [extender (build)]   Executing launch environment install process
            [extender (build)]     Running 'npm ci --unsafe-perm --cache /layers/paketo-buildpacks_npm-install/npm-cache'
            [extender (build)]       npm ERR! code 1

Should the ubi builder work with arm?

mhdawson commented 3 months ago

@c0d1ngm0nk3y introduction of arm support is new on some stacks and none of the buildpacks are built for arm either. So short answer is that I don't think any of the components for the Node.js buildpack support arm.

There is no reason that if you build the buildpacks they would not build on arm, but you would have to build all of them.

I'm surprised that you can run any of the tests with arm? Is it that you are trying to use a M based OSX machine?