Request cpython if there is a node-gyp dependency

c0d1ngm0nk3y commented 3 months ago

see https://github.com/paketo-buildpacks/nodejs/issues/691

prerequisites:

libnodejs returns not only the scripts from package.json, but all dependencies so that they can be checked: https://github.com/paketo-buildpacks/libnodejs/pull/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

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

mhdawson commented 3 months ago

@c0d1ngm0nk3y is this draft waiting on something?

c0d1ngm0nk3y commented 2 months ago

@c0d1ngm0nk3y is this draft waiting on something?

It needs https://github.com/paketo-buildpacks/npm-install/pull/759, so that I can actually check if node-gyp is needed.

c0d1ngm0nk3y commented 2 months ago

I think this is ready for some comments.

Although I listed https://github.com/paketo-buildpacks/nodejs/pull/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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

pacostas commented 2 months ago

@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 commented 2 months ago

@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 https://github.com/paketo-buildpacks/npm-install/blob/73106bf80cc1ed4f45d6c15ea7a5c2f3bdc8420a/integration/native_modules_test.go#L51

c0d1ngm0nk3y commented 2 months ago

Do you have a sample app to reproduce the error?

I used this app for testing.

pacostas commented 2 months ago

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 commented 1 month ago

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

paketo-buildpacks / npm-install