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

Ability to externalize WASM was broken #3345

Closed
mhdawson opened this issue Jun 19, 2024 · 13 comments · Fixed by #3421
Closed

Ability to externalize WASM was broken #3345

mhdawson opened this issue Jun 19, 2024 · 13 comments · Fixed by #3421
Labels
bug Something isn't working

Comments

@mhdawson
Copy link
Member

mhdawson commented Jun 19, 2024

Bug Description

This PR introduced the ability to externalize WASM, which is needed by distros to be able to rebuild all components shipped: #2643

PR #3074 broke this by removing some of the implementation needed.

Reproducible By

Build as specified in the undici CONTRIBUTING.md - https://github.com/nodejs/undici/blob/main/CONTRIBUTING.md#building-for-externally-shared-node-builtins

and validate that the wasm is not in the expected place

Expected Behavior

After building as described in the undici CONTRIBUTING.md - https://github.com/nodejs/undici/blob/main/CONTRIBUTING.md#building-for-externally-shared-node-builtins

the wasm binary should be in the expected place

Logs & Screenshots

Environment

Additional context

@mhdawson mhdawson added the bug Something isn't working label Jun 19, 2024
@ronag
Copy link
Member

ronag commented Jun 19, 2024

@Uzlopak

@mhdawson
Copy link
Member Author

mhdawson commented Jun 19, 2024

There is some discussion in the PR after it landed, goal is to add back the parts that were removed but still needed while keeping what was optimized in #3074

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 19, 2024

Yes, we discussed this, I kind of lost track of it.

@mcollina
Copy link
Member

Should we revert?

@mhdawson how can we test for this in a meaningful way so we don't regress?

@mhdawson
Copy link
Member Author

I was talking about that a bit with Richard. I don't know the undici tests well enough to know off the top of my head, but building with the option enabled and validating that the wasm file is were it's expected afterwards was what we thought might work.

@mcollina
Copy link
Member

@mhdawson if any of you can drop a PR for this, it would be very useful to prevent further issues.

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 20, 2024

Actually it just needs like 5 more lines, I promise I will provide a PR tonight at latest midnight (German Time),

@mhdawson
Copy link
Member Author

@Uzlopak do you mean you will include the testing in your PR?

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 20, 2024

I will also provide a test.

@khardix
Copy link
Contributor

khardix commented Jul 3, 2024

@Uzlopak Any updates?

@mhdawson
Copy link
Member Author

mhdawson commented Jul 9, 2024

@Uzlopak just wondering if there is something I can do to help this along? If you have the right way in mind to fix but just don't have time maybe we can get together to do some knowledge transfer and either myself or @richardlau can help out?

@richardlau
Copy link
Member

I'm working on this and should be able to open a PR either later today or tomorrow (trying to get a working GitHub Actions flow in place so future regressions can be detected).

@richardlau
Copy link
Member

PR: #3421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants