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

chore: use common WASM builder #3791

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Oct 31, 2024

This relates to...

See discussion in #3714

Note that with the updated common/container the same WASM seems to be generated so there should be
no functional changes.

The container itself is build using an approach that was based off of what undici was doing.

Rationale

#3714 includes the rational and discussion agreeing that I should submit a PR.

Changes

Updates the build process to use the Node.js common wasm-builder container.

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@mhdawson mhdawson force-pushed the use-common-wasm-builder branch from 463ef95 to f1438c4 Compare October 31, 2024 19:32
@mhdawson
Copy link
Member Author

Rebased

@mhdawson
Copy link
Member Author

Since the WASM did not change I don't think any of the failures are related to this PR.

This issue seems to say there is a lot of flakiness and probably confirms that - #3787

@metcoder95 metcoder95 requested a review from mcollina November 1, 2024 09:00
@@ -1,13 +1,14 @@
'use strict'

const WASM_BUILDER_CONTAINER = 'ghcr.io/nodejs/wasm-builder@sha256:975f391d907e42a75b8c72eb77c782181e941608687d4d8694c3e9df415a0970' // v0.0.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there plans to provide regular updates to it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been thinking a bit about that, I think we want to update regularly but not necessarily too often. At least a month before every new Node.js major but I could see more often making sense as well.

The repo where it is maintained has release please and all of the automation to generate a new vesion so it's as simple as a PR to update the dockerfile used to build the container and once that lands the rest is pretty much automated.

@metcoder95 how often do you think an update would make sense ? How often was undici updating?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot develop a cadence, as I'd say that should be up to the things added to the new version. If it contains security fixes, maybe faster than usual, otherwise we can make a check every time we prepare a new undici's release.

@mcollina @ronag wdyt?

@ronag ronag merged commit 20b9c83 into nodejs:main Nov 7, 2024
30 of 36 checks passed
flakey5 pushed a commit to flakey5/undici that referenced this pull request Nov 14, 2024
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
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.

3 participants