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

cmd/distpack: wasm runtime js should be kept in the toolchain package #68024

Closed
Zxilly opened this issue Jun 16, 2024 · 34 comments
Closed

cmd/distpack: wasm runtime js should be kept in the toolchain package #68024

Zxilly opened this issue Jun 16, 2024 · 34 comments
Labels
arch-wasm WebAssembly issues FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Zxilly
Copy link
Contributor

Zxilly commented Jun 16, 2024

Go version

go version go1.22.0 windows/amd64

Output of go env in your module/workspace:

set GO111MODULE=on
set GOARCH=wasm
set GOBIN=
set GOCACHE=C:\Users\zxilly\AppData\Local\go-build
set GOENV=C:\Users\zxilly\AppData\Roaming\go\env
set GOEXE=
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\zxilly\go\pkg\mod
set GONOPROXY=1
set GONOSUMDB=
set GOOS=js
set GOPATH=C:\Users\zxilly\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\zxilly\go\pkg\mod\golang.org\toolchain@v0.0.1-go1.22.4.windows-amd64
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Users\zxilly\go\pkg\mod\golang.org\toolchain@v0.0.1-go1.22.4.windows-amd64\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22.4
set GCCGO=gccgo
set GOWASM=
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=E:\Project\CS_Project\gsv\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-fPIC -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\zxilly\AppData\Local\Temp\go-build2827437537=/tmp/go-build -gno-record-gcc-switches

What did you do?

https://go.dev/wiki/WebAssembly#running-tests-in-the-browser

Try to run wasm test follow the official document. The Go Toolchain overwrites GOROOT env thus the wasmbrowsertest can not find the wasm_exec.js

I also report this to wasmbrowsertest at agnivade/wasmbrowsertest#61

What did you see happen?

FAIL    github.com/Zxilly/go-size-analyzer/internal/disasm      0.071s
open C:\Users\zxilly\go\pkg\mod\golang.org\toolchain@v0.0.1-go1.22.4.windows-amd64/misc/wasm/wasm_exec.js: The system cannot find the path specified.
PS C:\Users\zxilly\go\pkg\mod\golang.org\toolchain@v0.0.1-go1.22.4.windows-amd64> ls

    Directory: C:\Users\zxilly\go\pkg\mod\golang.org\toolchain@v0.0.1-go1.22.4.windows-amd64

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-r--           2024/6/15    18:14                bin
d-r--           2024/6/15    18:14                lib
d-r--           2024/6/15    18:14                pkg
d-r--           2024/6/15    18:14                src
-ar--           2024/6/15    18:14             52 codereview.cfg
-ar--           2024/6/15    18:14           1337 CONTRIBUTING.md
-ar--           2024/6/15    18:14            505 go.env
-ar--           2024/6/15    18:14           1479 LICENSE
-ar--           2024/6/15    18:14           1303 PATENTS
-ar--           2024/6/15    18:14           1455 README.md
-ar--           2024/6/15    18:14            426 SECURITY.md
-ar--           2024/6/15    18:14             35 VERSION

What did you expect to see?

Test success.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 17, 2024
@dmitshur
Copy link
Contributor

dmitshur commented Jun 17, 2024

It looks like an intentional decision in CL 470676 that the module toolchain isn't complete to run all tests, as it doesn't keep the "test" directory either:

https://cs.opensource.google/go/go/+/master:src/cmd/distpack/pack.go;l=206-214;drc=dbe03e4831206d7311e4423c3a988fc48beda2bd

The scripts in the misc/wasm directory can be fetched from the source or binary toolchain distributions.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jun 18, 2024

I agree with this, but misc/wasm is not used only for testing, on the contrary, it is a mandatory runtime file.
Of course we could choose to parse the VERSION file and get the corresponding version from the internet, but that requires additional complexity. And these files are not large enough to make a significant impression on the size of the final archive.

@agnivade
Copy link
Contributor

@dmitshur - Just chiming in here. wasmbrowsertest needs access to the wasm_exec.js to run the wasm runtime. Not having the file included inside GOROOT is a problem for the tool to run the tests.

Downloading the file during running a test isn't a great experience either because one should be able to run tests without any internet connection.

Wondering if this decision can be revisited and the misc/wasm directory can be included? As mentioned before, the file sizes are fairly small and should not make any impact size wise.

cc @rsc for a decision as well.

@dmitshur dmitshur added this to the Backlog milestone Jun 27, 2024
@dmitshur dmitshur added the arch-wasm WebAssembly issues label Jun 27, 2024
@rsc
Copy link
Contributor

rsc commented Jun 28, 2024

misc files should not be included in toolchains, full stop. misc is by definition a dumping ground.

If there are required supporting files that should be included in toolchains, they should be moved somewhere else. Perhaps lib/wasm.

@Zxilly
Copy link
Contributor Author

Zxilly commented Jun 28, 2024

Did this move needs a proposal?

@Zxilly Zxilly changed the title cmd/distpack: misc/wasm folder should be kept in the toolchain package cmd/distpack: wasm runtime js should be kept in the toolchain package Jun 28, 2024
@agnivade
Copy link
Contributor

agnivade commented Jul 1, 2024

If there are required supporting files that should be included in toolchains, they should be moved somewhere else. Perhaps lib/wasm.

Happy to do that.

cc @neelance for alignment on a decision. Richard, please see the thread above for some context.

@johanbrandhorst
Copy link
Member

Moving wasm_exec.js sounds good to me. Please submit a CL if you can Agniva.

@agnivade
Copy link
Contributor

Oh, there was a thumbs up from Richard. Never saw that until now. 😄 Will do.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/604696 mentions this issue: misc/wasm: move wasm runtime files to lib/wasm

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/604119 mentions this issue: WebAssembly: update support file location

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/604835 mentions this issue: all: add support for lib/wasm folder

@johanbrandhorst
Copy link
Member

I'm a little concerned that this will make any posts explaining how to use Wasm with Go outdated. We'll at the very least want to update the official wasi blog post (https://go.dev/blog/wasi) and Wasm wiki (https://go.dev/wiki/WebAssembly) to coincide with the release of this change. There might be other places too. I'm not sure how to best communicate this change to the Go Wasm community at large though.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 12, 2024

Maybe we could keep a soft link in misc pointing to lib/wasm?

@johanbrandhorst
Copy link
Member

If that's something we can support, maybe, but I don't know if we have precedence for that sort of thing, or even capability for all releases.

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Aug 12, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 12, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/604916 mentions this issue: wasi: update wasm runtime file location

@cherrymui
Copy link
Member

(I commented on the CL. Mention here for discussion)

I agree that wasm_exec.js and perhaps wasm_exec_node.js are "runtime files" because they provide the necessary JS host support to run Go js/wasm modules.

But I'm less sure about the exec wrappers. They are convenient tools for "go run" and "go test", but in actual uses the wasm module may be invoked in a different way (e.g. embedded in another program). wasm_exec.html is also more of an example. Most users probably won't use it as is.

So maybe we include wasm_exec.js and wasm_exec_node.js in the toolchain release but not the others?

@dmitshur
Copy link
Contributor

dmitshur commented Aug 19, 2024

Not moving wasm_exec.html, since it is an example rather than something to be used directly, makes sense to me.

I think there may be some value in making the go_*_*_exec helpers available: it would make it possible to use downloaded versions of the Go toolchain (i.e., selected via "go" directive in go.mod, or GOTOOLCHAIN env var, etc.) for the js/wasm port no different than it is possible for other ports. Although the original report didn't say this directly, I understood this to be the main problem that was intended to be resolved here.


Suppose a user has a module that includes Go code intended to be built for and tested with GOOS=js GOARCH=wasm (in addition to another port, say, darwin/arm64):

$ ls         
go.mod			p_darwin_test.go	p_js_test.go

$ cat *
module example

go 1.22.6
package p

import "testing"

func TestDarwin(t *testing.T) {
	if 1 + 1 != 2 {
		t.Error("some problem")
	}
}
package p

import ("testing"; "syscall/js")

func TestJS(t *testing.T) {
	if js.Global().IsNull() {
		t.Error("some problem")
	}
}

Both darwin/arm64 and js/wasm ports work with a local toolchain:

$ go version
go version go1.22.6 darwin/arm64

$ go test -v
=== RUN   TestDarwin
--- PASS: TestDarwin (0.00s)
PASS
ok  	example	0.198s

$ export PATH="$PATH:$(go env GOROOT)/misc/wasm"
$ GOOS=js GOARCH=wasm go test -v
=== RUN   TestJS
--- PASS: TestJS (0.00s)
PASS
ok  	example	1.515s

But once a toolchain upgrade happens, only one of the two ports can be tested as before:

$ go get go@1.23.0                              
go: updating go.mod requires go >= 1.23.0; switching to go1.23.0
go: downloading go1.23.0 (darwin/arm64)
go: upgraded go 1.22.6 => 1.23.0

$ go version
go version go1.23.0 darwin/arm64

$ go test -v                                    
=== RUN   TestDarwin
--- PASS: TestDarwin (0.00s)
PASS
ok  	example	0.225s

$ export PATH="$PATH:$(go env GOROOT)/misc/wasm"
$ GOOS=js GOARCH=wasm go test -v       
fork/exec /tmp/go-build4130996875/b001/example.test: exec format error
FAIL	example	0.001s

@cherrymui
Copy link
Member

for the js/wasm port no different than it is possible for other ports

I don't think that's the case. Currently we don't provide an exec wrapper for any cross-compiled ports.

Even if we put them in the release package, one still needs to add (or copy or link) them to PATH. Even with PATH="$PATH:$(go env GOROOT)/misc/wasm", it depends where/when the command is run. Given this, I'm not sure it helps much to add the exec wrappers.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 20, 2024

My first reaction was to agree with you, but some other points came to mind when I was operating git.

While Go Toolchain enabled, operation like PATH="$PATH:$(go env GOROOT)/misc/wasm" is impossible since GOROOT has pointed to the root directory of the toolchain package now. $(go env GOROOT)/misc" dir didn't exist in the toolchain pack.

@johanbrandhorst
Copy link
Member

I agree that unless it's strictly necessary it doesn't need to be in the toolchain. Only wasm_exec.js and wasm_exec_node.js are strictly necessary to execute the Go-compiled Wasm. The exec wrappers are only necessary to execute the code in specific environments. They are not necessary in the browser, for example. Adding these files would be a convenience, not a necessity.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 20, 2024

I'm still confused about how go toolchain users can access these exec wrappers.

@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Aug 20, 2024
@johanbrandhorst
Copy link
Member

The exec wrappers are just conveniences. Users can write their own exec wrappers if they need something. Not every user will need an exec wrapper to execute the compiled code.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 20, 2024

So we need to updates many document to indicate that these scripts are not avaiable while using GoToolChain?

@johanbrandhorst
Copy link
Member

Uses of export PATH=$PATH:$(go env GOROOT)/misc/wasm go run will not work reliably when using other Go toolchains, yeah. I think that's probably fine.

@agnivade
Copy link
Contributor

Hey all, just wanted to check in on this. Thank you @Zxilly for dilligently addressing the review comments.

@cherrymui - It sounds like adding further documentation updates for users depending on the default exec wrappers should be acceptable. Can we move forward with the changes then?

@dmitshur
Copy link
Contributor

dmitshur commented Aug 21, 2024

I moved this issue to NeedsDecision to better reflect its latest state, given the discussion here and on CL 604696. To expand on that: it seems there's agreement that the wasm_exec.js and wasm_exec_node.js files should be included in toolchain zips, and that the wasm_exec.html example should stay in misc/wasm (and not be included in toolchain zips). The remaining question is about the go_*_wasm_exec programs that help with running GOARCH=wasm code via go run and testing it via go test.

There are two paths forward being currently discussed, as I understand them:

  1. Leave the exec programs out of toolchain zips. Update documentation that refers to those exec programs to clearly say that they're not supported with downloaded toolchains (https://go.dev/doc/toolchain#download), only with local toolchains (e.g., GOTOOLCHAIN=local) or toolchains found in path (e.g., GOTOOLCHAIN=local+path).

  2. Make the exec programs available in toolchain zips, in the lib/wasm directory.

Let's get to a decision here, and then the CL can be updated to reflect that. Thanks.

@agnivade
Copy link
Contributor

Thanks Dmitri. Sounds to me like approach 1 is just going to make life a bit more painful for folks working with downloaded toolchains? What exactly are we gaining by not including the exec wrappers? I thought the original issue was more about renaming misc/wasm to lib/wasm.

To me, it feels like we have little to lose, but more to gain by including those files. But will defer to others here.

@Zxilly
Copy link
Contributor Author

Zxilly commented Aug 21, 2024

I chose option 2 because GoToolchain is more of an implicit mechanism. If a user doesn't know about GoToolchain, it will be difficult for him to understand why files present in his Go installation are not accessible.

@cherrymui
Copy link
Member

It seems most people think it helpful to include the exec wrappers. I think that is fine.

One difference for the exec wrappers are that, unlike most things included in the release which are ready to be used on production, the exec wrappers are mostly intended for local uses. I guess it is not much of a problem given the nature of the exec wrappers? And perhaps the convenience of using the exec wrapper with automatic toolchain switch overweighs.

@agnivade
Copy link
Contributor

Yep exactly. IMO, it's an overall net gain.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 27, 2024

Based on discussion above, it sounds like there's agreement: for Go 1.24, the .js files are moved to lib/wasm, as well as the exec scripts. The .html file isn't moved. It's a bit unusual to have executables in a lib directory but it shouldn't cause problems (and if there's a good reason to find a better long term location for them, that can still happen in a future issue). This makes it possible to refer to files in $(go env GOROOT)/lib/wasm directory for the currently selected Go toolchain even after using https://go.dev/doc/toolchain#download.

Let's give this a bit more time and move it to NeedsFix if there aren't new concerns.

@eliben
Copy link
Member

eliben commented Aug 29, 2024

I'm a little concerned that this will make any posts explaining how to use Wasm with Go outdated. We'll at the very least want to update the official wasi blog post (https://go.dev/blog/wasi) and Wasm wiki (https://go.dev/wiki/WebAssembly) to coincide with the release of this change. There might be other places too. I'm not sure how to best communicate this change to the Go Wasm community at large though.

Once this change lands, we can address the documentation concern by creating a 1.24 release blocking issue to update the documentation along with the release (bloc post, wiki, etc).

What's more concerning is that this doesn't work right now

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 30, 2024
@dmitshur
Copy link
Contributor

Filed #69175 to track the remaining work of updating relevant documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants