Allow custom version of `npm` #688

c0d1ngm0nk3y · 2024-05-17T10:39:24Z

based on #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 paketo-buildpacks/nodejs#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

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

c0d1ngm0nk3y · 2024-05-17T10:54:47Z

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 · 2024-05-17T12:25:48Z

Test failure related to #689

mhdawson · 2024-05-27T17:24:31Z

build.go

+		logger.Process("Checking custom npm version")
+		npmVersion, found := environment.Lookup("BP_NPM_VERSION")
+		if found {
+			args := []string{"install", "-g", fmt.Sprintf("npm@%s", npmVersion)}


@c0d1ngm0nk3y do you know where all the globall installed npm version later used? I know there are some cases where a global install has issues on linux install and I'm wondering if it can be installed non-global and then either direct paths or path search order can be used to get the installed non-global version?

We changed it to a non global version

c0d1ngm0nk3y · 2024-06-18T15:21:25Z

@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 · 2024-06-19T15:09:56Z

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

mhdawson · 2024-06-19T21:42:36Z

build.go

@@ -79,6 +82,24 @@ func Build(entryResolver EntryResolver,
 			return packit.BuildResult{}, err
 		}

+		logger.Process("Checking custom npm version")


I think we should move this into the if found, and log something like "Using a custom npm versions set with BP_NPM_VERSION"

That would reduce the updates needed to the tests, both in this repo but more importantly in my mind possibly to tests which existing in the other buildpack repos.

In principal, I find it a bit worrying that you should not add a log message because it can break tests. Those tests are way to fragile anyway. But you have a point and probably the log message is better suited in the if anyway.

mhdawson · 2024-06-19T21:51:44Z

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

mhdawson · 2024-06-19T21:55:33Z

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 · 2024-06-19T21:58:57Z

@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 · 2024-06-20T12:43:25Z

needs to be updated to 18

I have opened #713 for this and picked the commit in the meantime.

c0d1ngm0nk3y · 2024-06-20T14:12:18Z

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

pacostas · 2024-06-20T15:07:22Z

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

integration/caching_test.go

mhdawson · 2024-06-20T22:28:58Z

build.go

@@ -155,7 +176,7 @@ func Build(entryResolver EntryResolver,
 					layer.BuildEnv.Default("NPM_CONFIG_GLOBALCONFIG", globalNpmrcPath)
 				}
 				path := filepath.Join(layer.Path, "node_modules", ".bin")
-				layer.BuildEnv.Append("PATH", path, string(os.PathListSeparator))
+				layer.BuildEnv.Prepend("PATH", path, string(os.PathListSeparator))


I'm still wondering a bit about this part of the change. With the append, installed packages could add a binary but not replace one of the system ones. With Prepend they can replace system ones which might be a security concern.

I assume it would be possible to pre-pend just npm, with something like

path := filepath.Join(layer.Path, "node_modules", ".bin")
layer.BuildEnv.Prepend("PATH", filepath.Join(path, "npm"), path, string(os.PathListSeparator))
layer.BuildEnv.Append("PATH", path, string(os.PathListSeparator))

which would still let us use the newer npm, but not change anything else?

Sounds good. I will adapt it.

which would still let us use the newer npm, but not change anything else

I did the change like proposed, but now many integration test fail. Only prepend seem to be used and not the combination of prepend and append. Is there a reason why not both can be used at the same time?

Can you post the code you tried? I don't see any reason why it should not be possible to do both

We had a look and there is is a bug in packit (see code). It should work and "only" the logs are wrong, but I guess it is still worth fixing in packit first.

The problem is that the log emitter assumes that an environment variable is either prepended or appended. Not both.

Proposed fix: paketo-buildpacks/packit#581

@c0d1ngm0nk3y thanks for chasing that down.

I'm still wondering a bit about this part of the change. With the append, installed packages could add a binary but not replace one of the system ones. With Prepend they can replace system ones which might be a security concern.

I assume it would be possible to pre-pend just npm, with something like

path := filepath.Join(layer.Path, "node_modules", ".bin") layer.BuildEnv.Prepend("PATH", filepath.Join(path, "npm"), path, string(os.PathListSeparator)) layer.BuildEnv.Append("PATH", path, string(os.PathListSeparator))

which would still let us use the newer npm, but not change anything else?

We ran into a problem with that one. It worked for ubi, but the shell of the paketo-builder doesn't like binaries in the PATH, only directories. So we created a folder containing only a symlink to npm (unfortunately one more symlink) and prepended this folder to PATH. WDYT?

c0d1ngm0nk3y · 2024-07-19T06:34:27Z

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

mhdawson · 2024-07-19T13:14:35Z

@c0d1ngm0nk3y thanks for the update.

c0d1ngm0nk3y · 2024-07-24T06:41:18Z

paketo-buildpacks/packit#581 (comment)

c0d1ngm0nk3y · 2024-08-16T06:43:13Z

@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 · 2024-08-20T15:43:53Z

@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?

c0d1ngm0nk3y · 2024-09-05T11:25:05Z

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?

I am normally running them with the env variable DOCKER_DEFAULT_PLATFORM=linux/amd64 which seems to work fine. Also for the ubi builder.

c0d1ngm0nk3y · 2024-09-06T13:11:00Z

The tests are fine locally...

c0d1ngm0nk3y · 2024-09-06T13:30:15Z

@mhdawson It was "only" the node dependencies which could not be downloaded. I have the same problem locally from time to time. Seems not to be too reliable.

Co-authored-by: Ralf Pannemans <[email protected]> Co-authored-by: Pavel Busko <[email protected]>

Co-authored-by: Ralf Pannemans <[email protected]>

c0d1ngm0nk3y · 2024-09-13T08:39:15Z

I checked the posix standard

PATH

This variable shall represent the sequence of path prefixes that certain functions and utilities apply in searching for an executable file. The prefixes shall be separated by a (':'). If the pathname being sought contains no ('/') characters, and hence is a filename, the list shall be searched from beginning to end, applying the filename to each prefix and attempting to resolve the resulting pathname (see 4.16 Pathname Resolution ), until an executable file with appropriate execution permissions is found. When a non-zero-length prefix is applied to this filename, a shall be inserted between the prefix and the filename if the prefix did not end in . A zero-length prefix is a legacy feature that indicates the current working directory. It appears as two adjacent characters ("::"), as an initial preceding the rest of the list, or as a trailing following the rest of the list. A strictly conforming application shall use an actual pathname (such as .) to represent the current working directory in PATH . If the pathname being sought contains any characters, the search through the path prefixes shall not be performed and the pathname shall be resolved as described in 4.16 Pathname Resolution . If PATH is unset or is set to null, or if a path prefix in PATH contains a character ('%'), the path search is implementation-defined.

ubi does it a bit differently and also accepts binaries, but to make it compatible I guess the approach with the extra directory containing only the npm binary was needed.

c0d1ngm0nk3y · 2024-09-13T08:53:10Z

@paketo-buildpacks/nodejs-maintainers Is there any chance to get this merged? Or is there anything missing?

I checked again and there should be no possibility for any regression since the folder .bin_local is only created if BP_NPM_VERSION is set and adding a non-existing path to PATH should not have any influence.

All checks have passed

mhdawson

LGTM

c0d1ngm0nk3y requested a review from a team as a code owner May 17, 2024 10:39

c0d1ngm0nk3y requested review from thitch97 and TisVictress May 17, 2024 10:39

c0d1ngm0nk3y force-pushed the custom-npm-version branch 2 times, most recently from 8b49ded to 635ccec Compare May 17, 2024 10:43

c0d1ngm0nk3y added the semver:minor A change requiring a minor version bump label May 17, 2024

c0d1ngm0nk3y force-pushed the custom-npm-version branch from 635ccec to 18530da Compare May 17, 2024 10:44

c0d1ngm0nk3y mentioned this pull request May 21, 2024

Fix flaky unit test: linked_module_resolver_test #689

Merged

5 tasks

mhdawson reviewed May 27, 2024

View reviewed changes

c0d1ngm0nk3y force-pushed the custom-npm-version branch from 18530da to ad8e9b9 Compare June 4, 2024 12:31

c0d1ngm0nk3y force-pushed the custom-npm-version branch from ad8e9b9 to ef52a74 Compare June 18, 2024 15:18

mhdawson reviewed Jun 19, 2024

View reviewed changes

c0d1ngm0nk3y force-pushed the custom-npm-version branch 2 times, most recently from a05b703 to 1a8e569 Compare June 20, 2024 12:14

c0d1ngm0nk3y mentioned this pull request Jun 20, 2024

Testdata must not use NodeJS 16 anymore #713

Merged

5 tasks

c0d1ngm0nk3y force-pushed the custom-npm-version branch from 1a8e569 to b1160e3 Compare June 20, 2024 12:48

mhdawson force-pushed the custom-npm-version branch from 499da6c to edc677a Compare June 20, 2024 15:09

mhdawson reviewed Jun 20, 2024

View reviewed changes

integration/caching_test.go Outdated Show resolved Hide resolved

mhdawson reviewed Jun 20, 2024

View reviewed changes

c0d1ngm0nk3y force-pushed the custom-npm-version branch from edc677a to 95f8714 Compare June 21, 2024 14:29

nicolasbender mentioned this pull request Jun 24, 2024

Fix override of existing values in prepend & append paketo-buildpacks/packit#581

Merged

5 tasks

c0d1ngm0nk3y force-pushed the custom-npm-version branch 5 times, most recently from 26d6f61 to 99ee0f5 Compare August 15, 2024 14:42

c0d1ngm0nk3y mentioned this pull request Sep 5, 2024

Issues tagged for discussion in bi-weekly maintainer meeting paketo-buildpacks/nodejs#910

Open

c0d1ngm0nk3y force-pushed the custom-npm-version branch from 99ee0f5 to c1d851a Compare September 5, 2024 11:21

c0d1ngm0nk3y force-pushed the custom-npm-version branch from c1d851a to 66063e7 Compare September 5, 2024 12:04

c0d1ngm0nk3y enabled auto-merge (squash) September 5, 2024 12:07

nicolasbender and others added 5 commits September 13, 2024 10:28

Allow custom version of npm to be installed

adbdf8a

Co-authored-by: Ralf Pannemans <[email protected]> Co-authored-by: Pavel Busko <[email protected]>

Adapt unit test to expect prepend of .bin

ebe8877

Co-authored-by: Ralf Pannemans <[email protected]>

Address review feedback

a511192

Co-authored-by: Ralf Pannemans <[email protected]>

Fix integration tests

1036f0d

Co-authored-by: Ralf Pannemans <[email protected]>

link local npm version to separate folder to add it to the path

50aee96

Co-authored-by: Ralf Pannemans <[email protected]>

c0d1ngm0nk3y force-pushed the custom-npm-version branch from 61fc834 to 50aee96 Compare September 13, 2024 08:34

mhdawson approved these changes Sep 13, 2024

View reviewed changes

c0d1ngm0nk3y merged commit 046b41a into main Sep 13, 2024
10 checks passed

c0d1ngm0nk3y deleted the custom-npm-version branch September 13, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom version of `npm` #688

Allow custom version of `npm` #688

c0d1ngm0nk3y commented May 17, 2024 •

edited

Loading

c0d1ngm0nk3y commented May 17, 2024

c0d1ngm0nk3y commented May 17, 2024

mhdawson May 27, 2024

c0d1ngm0nk3y Jun 4, 2024

c0d1ngm0nk3y commented Jun 18, 2024

c0d1ngm0nk3y commented Jun 19, 2024

mhdawson Jun 19, 2024

c0d1ngm0nk3y Jun 20, 2024

mhdawson commented Jun 19, 2024

mhdawson commented Jun 19, 2024

mhdawson commented Jun 19, 2024

c0d1ngm0nk3y commented Jun 20, 2024

c0d1ngm0nk3y commented Jun 20, 2024

pacostas commented Jun 20, 2024

mhdawson Jun 20, 2024

c0d1ngm0nk3y Jun 21, 2024

c0d1ngm0nk3y Jun 21, 2024

mhdawson Jun 21, 2024

c0d1ngm0nk3y Jun 24, 2024

nicolasbender Jun 24, 2024

mhdawson Jun 24, 2024

c0d1ngm0nk3y Aug 15, 2024

c0d1ngm0nk3y commented Jul 19, 2024

mhdawson commented Jul 19, 2024

c0d1ngm0nk3y commented Jul 24, 2024

c0d1ngm0nk3y commented Aug 16, 2024

mhdawson commented Aug 20, 2024

c0d1ngm0nk3y commented Sep 5, 2024

c0d1ngm0nk3y commented Sep 6, 2024

c0d1ngm0nk3y commented Sep 6, 2024

c0d1ngm0nk3y commented Sep 13, 2024

c0d1ngm0nk3y commented Sep 13, 2024

mhdawson left a comment

Allow custom version of npm #688

Allow custom version of npm #688

Conversation

c0d1ngm0nk3y commented May 17, 2024 • edited Loading

Summary

Use Cases

Details

Checklist

c0d1ngm0nk3y commented May 17, 2024

c0d1ngm0nk3y commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c0d1ngm0nk3y commented Jun 18, 2024

c0d1ngm0nk3y commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jun 19, 2024

mhdawson commented Jun 19, 2024

mhdawson commented Jun 19, 2024

c0d1ngm0nk3y commented Jun 20, 2024

c0d1ngm0nk3y commented Jun 20, 2024

pacostas commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c0d1ngm0nk3y commented Jul 19, 2024

mhdawson commented Jul 19, 2024

c0d1ngm0nk3y commented Jul 24, 2024

c0d1ngm0nk3y commented Aug 16, 2024

mhdawson commented Aug 20, 2024

c0d1ngm0nk3y commented Sep 5, 2024

c0d1ngm0nk3y commented Sep 6, 2024

c0d1ngm0nk3y commented Sep 6, 2024

c0d1ngm0nk3y commented Sep 13, 2024

c0d1ngm0nk3y commented Sep 13, 2024

mhdawson left a comment

Choose a reason for hiding this comment

Allow custom version of `npm` #688

Allow custom version of `npm` #688

c0d1ngm0nk3y commented May 17, 2024 •

edited

Loading