Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support build options via npmrc #633

Merged
merged 9 commits into from
Jun 19, 2024
Merged

feat: support build options via npmrc #633

merged 9 commits into from
Jun 19, 2024

Conversation

aminya
Copy link
Member

@aminya aminya commented Jun 18, 2024

This makes it possible to specify the build options of zeromq through npmrc.

They are documented here:
https://github.com/zeromq/zeromq.js/tree/opts#draft-support

Related to #529

@rotu
Copy link
Contributor

rotu commented Jun 18, 2024

I'm not too familiar with building native node modules but this gives a reference for how to target multiple architectures at once.

https://sharp.pixelplumbing.com/install#cross-platform
https://pnpm.io/package_json#pnpmsupportedarchitectures

I think I saw a reference for specifying universal builds for macOS in Cmake via CMAKE_OSX_ARCHITECTURES=arm64;x86_64, but I'm not sure if node-gyp supports an analogous setting.

@aminya aminya force-pushed the opts branch 2 times, most recently from 557ff8b to 7ae51ce Compare June 19, 2024 05:06
@aminya
Copy link
Member Author

aminya commented Jun 19, 2024

I'm not too familiar with building native node modules but this gives a reference for how to target multiple architectures at once.

We probably don't need that for prebuilds, but if someone desires that now it's possible via the arch option in .npmrc

script/build.ts Outdated Show resolved Hide resolved
@rotu
Copy link
Contributor

rotu commented Jun 19, 2024

We probably don't need that for prebuilds, but if someone desires that now it's possible via the arch option in .npmrc

Yes, how does this interact with pnpm and yarn supportedArchitectures if they list multiple configs? Will this use the parent project's settings? Or will it clobber it with the current platform's architecture?

https://yarnpkg.com/configuration/yarnrc#supportedArchitectures
https://pnpm.io/package_json#pnpmsupportedarchitectures

@aminya
Copy link
Member Author

aminya commented Jun 19, 2024

We probably don't need that for prebuilds, but if someone desires that now it's possible via the arch option in .npmrc

Yes, how does this interact with pnpm and yarn supportedArchitectures if they list multiple configs? Will this use the parent project's settings? Or will it clobber it with the current platform's architecture?

yarnpkg.com/configuration/yarnrc#supportedArchitectures pnpm.io/package_json#pnpmsupportedarchitectures

That's a feature that we don't support yet, and we might look into it. It's quite complicated, and unless a package provides it, I am not sure if it is worth it.
https://github.com/evanw/esbuild/blob/fc37c2fa9de2ad77476a6d4a8f1516196b90187e/lib/npm/node-platform.ts#L113

@rotu
Copy link
Contributor

rotu commented Jun 19, 2024

That's a feature that we don't support yet, and we might look into it. It's quite complicated, and unless a package provides it, I am not sure if it is worth it.

I don’t know what you mean “unless a package provides it”.

The way esbuild supports being installed on multiple architectures is based on splitting the prebuilds across several packages. It would be complicated even if they didn’t support multiple architectures.

I believe the way pnpm and yarn support this is by calling the npm rebuild hooks for each architecture. Basically doing what sharp suggests doing manually:

npm install --cpu=x64 --os=darwin sharp
npm install --cpu=arm64 --os=darwin sharp

@aminya
Copy link
Member Author

aminya commented Jun 19, 2024

We're using Node's native way, which is the arch and target_arch variables.

I don’t know what you mean “unless a package provides it”.

The code for handling what Esbuild does. There's a lot of management to be done if we want to support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants