-
Notifications
You must be signed in to change notification settings - Fork 565
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
fix: restore externalized Node.js dep compatibility #3421
Conversation
.github/workflows/nodejs.yml
Outdated
|
||
- name: Run tests | ||
working-directory: ./node-${{ steps.release.outputs.result }} | ||
run: tools/test.py -p dots --flaky-tests=dontcare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to take about 10 mins on GitHub runners. We could run less, but I wasn't able to easily identify a subset of Node.js tests that cover the functionality that Undici provides to Node.js.
.github/workflows/nodejs.yml
Outdated
runs-on: ubuntu-latest | ||
timeout-minutes: 120 | ||
steps: | ||
# Checkout into a subdirectory otherwise Node.js tests will break due to finding Undici's package.json in a parent directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW Node.js tests that break if a package.json
is found in a parent directory of Node.js' tests:
- Node.js 20:
test-policy-*
. The policy feature is removed from Node.js 22 and main so the tests don't exist there. - Node.js 22:
test-node-run
.
@@ -156,7 +156,8 @@ If you are packaging `undici` for a distro, this might help if you would like to | |||
an unbundled version instead of bundling one in `libnode.so`. | |||
|
|||
To enable this, pass `EXTERNAL_PATH=/path/to/global/node_modules/undici` to `build/wasm.js`. | |||
You shall also pass this path to `--shared-builtin-undici/undici-path` in Node.js's `configure.py`. | |||
Pass this path with `loader.js` appended to `--shared-builtin-undici/undici-path` in Node.js's `configure.py`. | |||
If building on a non-Alpine Linux distribution, you may need to also set the `WASM_CC`, `WASM_CFLAGS`, `WASM_LDFLAGS` and `WASM_LDLIBS` environment variables before running `build/wasm.js`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact value of what to set the environment variables to differ by Linux distribution.
See the workflow file for the Ubuntu 22.04 settings, and https://src.fedoraproject.org/rpms/nodejs-undici/blob/rawhide/f/nodejs-undici.spec for Fedora.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create a separate action for this? It'll be easy to backport and maintain long term. (main is currently targeting v7, what will hopefully be included in Node v23). |
Restore the ability to build Undici compatible with Node.js' `configure --shared-builtin-undici/undici-path ...` build option. Scopes the `hasApk` conditional to only cover the part that requires `apk`. Makes the WASM optimizer (binaryen) optional to allow building on Linux distributions that do not package `binaryen` and must be able to rebuild everything (including tooling) from source.
Add a workflow to test building Undici in a way compatible with Node.js built with `configure --shared-builtin-undici/undici-path ...`. This configuration is used by downstream Node.js packagers (e.g. Fedora) who require the ability to be able to build everything from source.
@mcollina I've split off the added CI into a separate workflow and commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@richardlau do you need a release of v6 with this change? So it can easily be backported down to Node.js v20 and v22? |
If possible, yes, otherwise @khardix is going to have to float a patch for Fedora/RHEL. |
* fix: restore externalized Node.js dep compatibility Restore the ability to build Undici compatible with Node.js' `configure --shared-builtin-undici/undici-path ...` build option. Scopes the `hasApk` conditional to only cover the part that requires `apk`. Makes the WASM optimizer (binaryen) optional to allow building on Linux distributions that do not package `binaryen` and must be able to rebuild everything (including tooling) from source. * ci: add workflow for externalized Node.js dep Add a workflow to test building Undici in a way compatible with Node.js built with `configure --shared-builtin-undici/undici-path ...`. This configuration is used by downstream Node.js packagers (e.g. Fedora) who require the ability to be able to build everything from source.
This relates to...
Fixes: #3345
Refs: #2643
Rationale
Changes were previously accepted to allow Undici to be built to work with Node.js'
--shared-builtin-undici/undici-path
configure
option. This allows Undici to exist on disk instead of being compiled into the Node.js binary. This is used by downstream Node.js packagers (e.g. Linux distributions, such as Fedora) that require the ability to build everything from source (including all dependencies and tooling).This functionality was regressed by #3074 and was not noticed as nothing in this repository broke.
Changes
Restore the ability to build Undici compatible with Node.js'
configure --shared-builtin-undici/undici-path ...
build option and adds a workflow to test this configuration.Scopes the
hasApk
conditional to only cover the part that requiresapk
. This allowsbuild/wasm.js
to be run on a non-Alpine based Linux distribution.Makes the WASM optimizer (binaryen) optional to allow building on Linux distributions that do not package
binaryen
and must be able to rebuild everything (including tooling) from source.Features
N/A
Bug Fixes
configure --shared-builtin-undici/undici-path ...
.Breaking Changes and Deprecations
N/A
Status