Request cpython if there is a node-gyp dependency

[c0d1ngm0nk3y]

see paketo-buildpacks/nodejs#691

prerequisites:

• libnodejs returns not only the scripts from package.json, but all dependencies so that they can be checked: Marshal dependencies from package.toml libnodejs#33

Summary

If a dependency to node-gyp is found, cpython is requested to provide `python for the build.

Use Cases

For nodejs app containing a dependency to node-gyp, the full stack was required since it is currently the only one containing python needed for the build.

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).

[mhdawson]

@c0d1ngm0nk3y is this draft waiting on something?

[c0d1ngm0nk3y]

@c0d1ngm0nk3y is this draft waiting on something?

It needs #759, so that I can actually check if node-gyp is needed.

[c0d1ngm0nk3y]

I think this is ready for some comments.

Although I listed paketo-buildpacks/nodejs#928 as a prerequisite, I am not so sure anymore. With that change, the detect will fail as soon as you have a dependency do node-gyp. But in that case, build used to fail anyway unless python was already on the stack. So I can even see a small benefit without the change in the meta buildpack. Thoughts?

[mhdawson]

@pacostas I thought we had already added some testing for native modules, just wondering how they would have passed on the ubuntu stack if additional requests for cpython is needed?

[mhdawson]

@c0d1ngm0nk3y for the set of PRs you have to address the overall requirement will this resutlt cpython only being available in the build image versus the run image? Off the top of my head I think that is what's needed to build native modules.

[c0d1ngm0nk3y]

@pacostas I thought we had already added some testing for native modules, just wondering how they would have passed on the ubuntu stack if additional requests for cpython is needed?

The build image would need python, right? So on some stacks, that might have worked. But this would allow any stack as long as the run image can run a nodejs.

[c0d1ngm0nk3y]

@c0d1ngm0nk3y for the set of PRs you have to address the overall requirement will this resutlt cpython only being available in the build image versus the run image? Off the top of my head I think that is what's needed to build native modules.

I thought this is required with

cPythonDependency := packit.BuildPlanRequirement{
			Name: Cpython,
			Metadata: BuildPlanMetadata{
				Build: true,
			},
		}

At runtime, there should not be any additional requirement. At least that is my understanding and it worked when I tested it.

[mhdawson]

We are not going to want to add the cpython layer for ubi, instead we would have the extension install it. This will likely require changes to the cpython buildpack to not install if the requirement is already statisfied when the buildpack runs.

Since that will take landing changes in the cpython library and we know that all of the build images used with Node.js applications and ubi currently include python, what I suggest is an if which wraps this and does not add the requirement if the stack is ubi based.

[mhdawson]

I'll also add that in addition to python most native addons will require the full C/C++ compiler chain. Unless that is on the lighter weight stack already many if not most addons won't compile even if python is present.

As one of the examples listed in the original issue requesting the feature, it includes C/C++ - https://github.com/mscdex/cpu-features/blob/master/src/binding.cc

So as a sanity check, does the base stack that they wanted to use include the C/C++ compiler but not python? Otherwise simply adding python won't be enough.

[c0d1ngm0nk3y]

Since that will take landing changes in the cpython library and we know that all of the build images used with Node.js applications and ubi currently include python, what I suggest is an if which wraps this and does not add the requirement if the stack is ubi based.

But isn't that a generic "problem" with cnb buildpacks? Dependencies are always installed, regardless if they are present on the stack or not. Some for node or jdk or am I missing something?

what I suggest is an if which wraps this and does not add the requirement if the stack is ubi based.

I thought the STACK_ID is deprecated.

Update: @mhdawson IT feels strange that this buildpack should no anything about ubi. Isn't it possible that the ubi extension adds cpython to the buildplan?

[c0d1ngm0nk3y]

Otherwise simply adding python won't be enough.

Good point, but strangely it was enough in the cases I tested. The stack I test with has no compiler chain. I will check.

[c0d1ngm0nk3y]

So as a sanity check, does the base stack that they wanted to use include the C/C++ compiler but not python? Otherwise simply adding python won't be enough.

You are right. This only helps with stack that have a compiler, but no python (e.g. paketo-base). I am not so sure anymore if this has enough benefit or if you should just use a larger stack (e.g. paketo-full) and simply the smaller run-image.

[pacostas]

@c0d1ngm0nk3y I tried building an app with below dependencies

 "dependencies": {
   "leftpad": "~0.0.1",
   "node-gyp": "^10.2.0"
 },

and there was no error. Do you have a sample app to reproduce the error?

[pacostas]

@pacostas I thought we had already added some testing for native modules, just wondering how they would have passed on the ubuntu stack if additional requests for cpython is needed?

Yes, is this one

npm-install/integration/native_modules_test.go

Line 51 in 73106bf

source, err = occam.Source(filepath.Join("testdata", "with_native_modules"))

[c0d1ngm0nk3y]

Do you have a sample app to reproduce the error?

I used this app for testing.

[pacostas]

Ok, managed to reproduce. Probably you already know, the app succeeds to build with the builder-jammy-buildpackless-full but not with the builder-jammy-base as the first one has the python installed on the stacks. On the tests we use the full and that is why the native addons test is passing and the integration tests do not fail. Also on the tutorial https://paketo.io/docs/howto/nodejs/ they use the base which breaks with native addons.

[c0d1ngm0nk3y]

I will close this for now ince the use case is quite limited (see comment)