-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
tools: make undici updater build wasm from src #54128
Conversation
Review requested:
|
@marco-ippolito as discussed, update to undici updater to make it build wasm from source. One commit is for the update to the updater, the other commit updates undici to the latest version to show the changes in what's updated/stored. I can remove the commit for the undici update after you've taken a look if we want to land the change to the updater on its own. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54128 +/- ##
==========================================
- Coverage 87.08% 87.07% -0.01%
==========================================
Files 643 643
Lines 181581 181582 +1
Branches 34897 34891 -6
==========================================
- Hits 158122 158110 -12
- Misses 16747 16750 +3
- Partials 6712 6722 +10 |
@mhdawson sorry for my ignorance, but would that make undici builds reproducible? |
Cc @nodejs/undici for visibility |
@RafaelGSS I don't think it goes all the way to making the reproducible because the container used to build the WASM may change, but that is something that I plan to work on as a follow on as part of the work I've mentioned in the security-wg. |
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
This is actually amazing.
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
- modify updater to include all files required to rebuild wasm from what's stored in the deps/undici directory - modify updater to build wasm from source while updating Signed-off-by: Michael Dawson <mdawson@devrus.com>
Signed-off-by: Michael Dawson <mdawson@devrus.com>
3b724e2
to
a315067
Compare
Rebased since there had been an undici update, now just adds the files which would be added by the updater but which were not being stored. |
I actually dont understand what the benefit is. I proposed nodejs/undici#3173 to ensure the integrity of the wasm files in the undici repo. This PR basically says, that nodejs does not trust the wasm files and thus needs to recompile them. |
These are some pros I could think of. |
But this PR is not about decoupling undici but only the wasm binaries 🤷. |
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.
I wonder: Is this right? Doesnt we need to also generate the wasm.js files, where the wasm file content is stored as base64?!
@Uzlopak I believe all of the WASM files are generated. It may not be obvious in the updated PR since it's not updating to a new version of undici and the wasm files seem to generate to the same thing unless either the undici code is changed or the tools used to generation change neither of which has occured in this update. |
@Uzlopak the other thing this PR fixes is that in a previous conversation the ask was that the source needed to build undici be in the deps, that was not the case (hence the files being added under deps/undici) and the best way to validate that is to make sure to build all of the components in the updater. |
CI is green so unless there is additional discussion by end of day today I'll go ahead and land |
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.
I tested it locally and validated to be working technically correct.
LGTM
- modify updater to include all files required to rebuild wasm from what's stored in the deps/undici directory - modify updater to build wasm from source while updating Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #54128 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
Landed in e9deab9 |
- modify updater to include all files required to rebuild wasm from what's stored in the deps/undici directory - modify updater to build wasm from source while updating Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #54128 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
- modify updater to include all files required to rebuild wasm from what's stored in the deps/undici directory - modify updater to build wasm from source while updating Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #54128 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
No description provided.