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

tools: make undici updater build wasm from src #54128

Closed
wants to merge 2 commits into from

Conversation

mhdawson
Copy link
Member

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 31, 2024
@mhdawson
Copy link
Member Author

@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.

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (f9f9a42) to head (3b724e2).
Report is 7 commits behind head on main.

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     

see 24 files with indirect coverage changes

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member

@mhdawson sorry for my ignorance, but would that make undici builds reproducible?

@marco-ippolito
Copy link
Member

Cc @nodejs/undici for visibility

@mhdawson
Copy link
Member Author

@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.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a 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.

Copy link
Member

@mertcanaltin mertcanaltin left a 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 <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Aug 6, 2024

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.

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 6, 2024

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.

@marco-ippolito
Copy link
Member

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.

  • we can hotfix on nodejs directly without a undici release
  • decouple the npm package from node, so undici is more free to experiment
  • download undici sources from github release rather than from npm package
  • use the same docker image as other dependency updates for repeatable build

These are some pros I could think of.
What are the cons?

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 6, 2024

But this PR is not about decoupling undici but only the wasm binaries 🤷.

Copy link
Contributor

@Uzlopak Uzlopak left a 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?!

@mhdawson
Copy link
Member Author

mhdawson commented Aug 6, 2024

@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.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@mhdawson
Copy link
Member Author

mhdawson commented Aug 6, 2024

@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.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member Author

mhdawson commented Aug 7, 2024

CI is green so unless there is additional discussion by end of day today I'll go ahead and land

Copy link
Contributor

@Uzlopak Uzlopak left a 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

mhdawson added a commit that referenced this pull request Aug 7, 2024
- 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 <[email protected]>
PR-URL: #54128
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Aug 7, 2024

Landed in e9deab9

@mhdawson mhdawson closed this Aug 7, 2024
targos pushed a commit that referenced this pull request Aug 14, 2024
- 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 <[email protected]>
PR-URL: #54128
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants