netlify / build-image

This is the build image used for running automated builds
MIT License
497 stars 196 forks source link

feat: add pnpm to build image #845

Closed lukasholzer closed 2 years ago

lukasholzer commented 2 years ago

๐ŸŽ‰ Thanks for submitting a pull request! ๐ŸŽ‰

Summary

Fixes https://github.com/netlify/build/issues/1633 Fixes #789

Add pnpm support to the build image via the new corepack feature of node as @ascorbic requested to have corepack enabled

Latest buildbot image with it: e3838a70621b55be5e32e035882193d2a1a220a4-focal

The matching Xenial PR that implements the breaking change in the install_dependencies function: https://github.com/netlify/build-image/pull/846

https://app.netlify.com/teams/lukas-holzer/builds/634931c9058c2417c7f14c5a#L34-L52 CleanShot 2022-10-14 at 11 56 51

if run with an too old node version we error out: CleanShot 2022-10-14 at 12 00 16


For us to review and ship your PR efficiently, please perform the following steps:

A picture of a cute animal (not mandatory, but encouraged)

kitop commented 2 years ago

I tried this with yarn and an older node version (14.18.3) and it fails due to the lack of corepack: https://app.netlify.com/sites/eloquent-kringle-e5fd85/deploys/6346c5554bb47610f3b2d67f

CleanShot 2022-10-12 at 15 37 39@2x

This could break a lot of builds. How should we deal with this case?

ascorbic commented 2 years ago

Yeah, I think this needs to ensure it's using a compatible version of node before trying to use corepack. It was added in v16.9.0 and v14.19.0

lukasholzer commented 2 years ago

@kitop I'm currently questioning why it should be possible to run with a lower node minor version ๐Ÿค” If you specify node 14 compatible you should run always on the latest minor we support and not on an old version.

But other than that I would suggest reverting the install of yarn back to not use corepack and just install corepack and pnpm if your node version is greater equal than 14.19.0

JGAntunes commented 2 years ago

I would suggest reverting the install of yarn back to not use corepack

Yeah I don't think we can tie yarn's usage with corepack without introducing a breaking change or going through extra trouble setting it up. We should leave outside of the scope of this change.

kitop commented 2 years ago

Are you saying to only use corepack for pnpm and keep the old method for yarn? That'd be a good way to unblock, if there are no other tradeoffs. We'll still need to support yarn for Node 12 and lower, anyway so this might simplify at the moment?

I've had a chat with Lukas about the node version and using latest minor, and that could be it's own thing.

lukasholzer commented 2 years ago

@kitop and @JGAntunes changed the installing

lukasholzer commented 2 years ago

Now we are using by default corepack and have yarn + pnpm installed over corepack.

When a user decides to use a old node version (where corepack is not supported) we install yarn then with the old method.

If the user uses a newer node version we can switch the version with corepack

This was required as having corepack installed + trying always to use the old way to install yarn does not work (you cannot uninstall yarn then if corepack is enabled and yarn got installed even over the old method)

lukasholzer commented 2 years ago

@kitop

on old node version it works with yarn fine as well: CleanShot 2022-10-14 at 12 03 47

kitop commented 2 years ago

Tried a few cases and all working for me! Would love some extra ๐Ÿ‘€ on the code tho

lukasholzer commented 2 years ago

Just an unused function leftover I think. Other than that LGTM ๐Ÿ‘

oh yea missed that already deleted