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

Updating to work with latest Go 1.19 version #75

Merged
merged 11 commits into from
Nov 23, 2022
Merged

Updating to work with latest Go 1.19 version #75

merged 11 commits into from
Nov 23, 2022

Conversation

donpui
Copy link
Contributor

@donpui donpui commented Nov 9, 2022

Updating to work with latest Go 1.19 version.

Copy wasm_exec.js was taken from GOROOT/misc/wasm folder of version 1.19.2

Ticket #OP2300

@donpui donpui requested a review from JustusFT November 9, 2022 09:49
@donpui
Copy link
Contributor Author

donpui commented Nov 9, 2022

@JustusFT this will require your look, as we don't pass E2E. Running locally also seems we fail with worker, when we try to use it.

@meejah
Copy link
Collaborator

meejah commented Nov 9, 2022

Having our own vendored copy of wasm_exec.js is bad; can we not import it from Go "directly" or something?

@donpui
Copy link
Contributor Author

donpui commented Nov 10, 2022

Having our own vendored copy of wasm_exec.js is bad; can we not import it from Go "directly" or something?

My initial thought the same, however I am not sure if it is possible as application and worker runs on client side where is no go environment. Seems this file has to be copied https://github.com/golang/go/wiki/WebAssembly#webassembly-in-chrome. This part requires more investigation.

@meejah
Copy link
Collaborator

meejah commented Nov 10, 2022

Seems this file has to be copied [..]

Having a copy is the artifacts is fine, but can't we just grab it from the "go root" at build time? (i.e. instead of burning it into this repository?)

@vu3rdd
Copy link
Contributor

vu3rdd commented Nov 17, 2022

@donpui May be once LeastAuthority/wormhole-william#92 is merged in, you may update the submodule to the latest commit in this PR itself, so all the Go 1.19 changes would be visible from this PR.

@donpui
Copy link
Contributor Author

donpui commented Nov 17, 2022

I have updated with w-w changes and also moved wasm_exec.js. Now it will be copied during the build with small change in file. @JustusFT take a look, maybe it can be done even better.

After all changes, seems we have working solution with Go-1.19

execSync(
"cp \"$(go env GOROOT)/misc/wasm/wasm_exec.js\" src/worker"
);
// export as Go module
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, however I'm not sure we need to modify the file with an export anymore. Maybe we can change from import Go from "wasm_exec"; to import "wasm_exec"; since loading the file already sets Go as a global variable. Though we would need some changes to the test suite since some tests import go as well. But this PR is good already. We could update the import and tests in a separate PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated and all tests pass, so merging.

@donpui donpui merged commit e1cd3b8 into main Nov 23, 2022
@donpui donpui deleted the update-go19 branch November 23, 2022 12:51
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.

4 participants