-
Notifications
You must be signed in to change notification settings - Fork 570
Standardize on a single GOOS/GOARCH and deprecate node-syscall module. #1111
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flimzy
reviewed
Apr 19, 2022
nevkontakte
added a commit
to nevkontakte/gopherjs
that referenced
this pull request
Apr 19, 2022
See gopherjs#1111. Co-authored-by: Jonathan Hall <flimzy@flimzy.com>
All relevant tests are passing and a stub for FS functions is provided when running in an environment without Node `fs` module or compatible.
- Fix skip condition in tests that should be run under normal Go. - TestNativesDontImportExtraPackages loads original packages with `js/wasm` build target.
- Skip tests that rely on GC finalizers. - Skip tests that rely on exact function names in stack traces. - Tweak goroutine leak detection to work with GopherJS stack traces. - Use fake networking transport for http client tests under nodejs.
…sts. The removed packages either actually pass all tests, or skip incompatible tests for `js/wasm` by default, which generally suits us well.
Use the wasm implementation from the standard library instead. It has more features and supports browsers with streaming unavailable. We still keep `XMLHttpRequest` transport in case GopherJS users rely on it.
Most GopherJS users target browser and likely don't want to pay the cost of file system polyfill they can't use in a browser anyway, so minimizing it is worth the effort. In addition, I converted the polyfill into ES5 syntax, since we haven't formally abandoned it yet.
GopherJS will use this target going forward. The values where chosen by the following logic: - GOOS describes the targeted API environment, which is generally the same as when building wasm: both execute under a browser or nodejs. Build tag `js` is shared by GopherJS and wasm and can be used to target the general environment. - GOARCH describes the low-level emitted code, which is currently EcmaScript 5. Using `ecmascript` here differentiates from `wasm`, while not referencing GopherJS explicitly. It also leaves us a different paths for future upgrades to ES6+ by either changing GOARCH or keeping it and making this change implicit. At the same time, we use fixed js/wasm target when building standard library packages, which resolves a lot of concerns discussed in gopherjs#693. Using js/ecmascript is not feasible because it'll require us to maintain a lot more overlays and/or rewrite build tags, which leads to the exactly same outcome. In addition, we always set `gopherjs` build tag to make it easy to guard sources using GopherJS-specific features. This is similar to `gc` and `gccgo` build tags respective compilers set. This change is potentially breaking for users that may have relied on GOOS=linux in their sources. To make transition easier for them, we support using GOOS/GOARCH from environment. However, they will be only applied to the user code. Standard library will still be built with js/wasm.
GopherJS doesn't support CGo. This was originally enabled to address gopherjs#215 by providing a better error when attempting to build a CGo package (instead of a compiler panic). However, this approach makes GopherJS refuse to build packages, which have a CGo and purego versions guarded by the `cgo` build tag: GopherJS woudl try to load the cgo version and error out. After this change GopherJS won't attempt to load Go sources with `import "C"`, and it will load sources with `//go:build !cgo` instead. For packages that require CGo one of the following errors will be returned: - "No buildable sources" if all Go sources import C. - "Undefined symbol" for symbols defined in the Go sources that import C and that are referenced by other Go source files. - "Unknown import path C" if you try to `gopherjs run` a source with import C directly.
To make transition somewhat less dramatic, we can provide the old implementations for `syscall.Syscall` and its friends in case someone actually wanted to make arbitrary syscalls from GopherJS running under Node. I can't really think of any valid reason to, but who knows? To enable this, a `legacy_syscall` build tag must be passed to GopherJS. This shim is mostly intended for non-standard library code that was using syscall package directly and it is more restrictive than previously (e.g. the set of `syscall` functions still remains that of `js/wasm`), but this seems like a reasonable compromise between complexity and flexibility. I am fairly confident that our support for file system access and a few other syscalls, emulated via Node APIs is more than sufficient for all our users, so after a release or two we will be able to delete this code path entirely.
We could remove our overlay entirely, but our implementation seems to be more advanced than the one of `js/wasm` in the standard library, so we keep it for now.
We only build standard library for `js/wasm` now, which works for us as-is, except for a couple of `go:linkname` directives we need to keep.
After we switched to building standard library for the `js/wasm` target, some of the overlays we had to maintain for other GOOS values are no longer necessary: - `crypto/x509` - `debug/elf` - `internal/syscall/linux` - `internal/testenv` - `net` - "noat" implementation of `os.RemoveAll`.
- Reduce code duplication. - Ensure that it uses the same loading context as the compiler. - Ensure it uses the same test variants of packages as the compiler.
They were used as a side channel to extract GOROOT and GOPATH from `build.Session`, which is an incorrect use of options. Eventually all logic that requires these paths should be moved to the `build` package, but for now we just provide an additional method to create a source mapping callback.
Embedded file systems use unix-style slashes, which was causing errors on Windows, when two styles were mixed. Converting all VFS paths to unix-style slashes resolves the issue. Fixes gopherjs#1049.
There are environments like AMD, which expose `require()` function, but do not provide `fs` module. In such cases we should fall back to our stub (see gopherjs#693 (comment)).
Ideally we'd do the complete set of tests, but there is a fair amount of edge cases fixing which would require a significant effort. For now we simply ensure that GopherJS can build, run tests and passes its own tests.
See gopherjs#1111. Co-authored-by: Jonathan Hall <flimzy@flimzy.com>
flimzy
approved these changes
Apr 19, 2022
Merged
dmitshur
added a commit
that referenced
this pull request
Jan 3, 2023
GopherJS 1.18 has standardized on a single value for GOOS and GOARCH in PR #1111. Update the README accordingly. (There has been agreement in issue #1169 that it's okay to be updating the top-level README for GopherJS 1.18-specific changes by now.) Also reword advice in compatibility.md to be more general. The upstream Go WebAssembly port (GOOS=js GOARCH=wasm) is also a 32-bit environment, and and the 64-bit variant (i.e., GOARCH=wasm64) doesn't exist yet, so it doesn't make for a good example. But a reminder that some types vary in size by architecture is probably worth including anyway.
dmitshur
added a commit
that referenced
this pull request
Jan 3, 2023
GopherJS 1.18 has standardized on a single value for GOOS and GOARCH in PR #1111. Update the README accordingly. (There has been agreement in issue #1169 that it's okay to be updating the top-level README for GopherJS 1.18-specific changes by now.) Also reword advice in compatibility.md to be more general. The upstream Go WebAssembly port (GOOS=js GOARCH=wasm) is also a 32-bit environment, and the 64-bit variant (i.e., GOARCH=wasm64) doesn't exist yet, so it's not a good example. But a reminder that some types vary in size based on the architecture is probably worth including anyway.
dmitshur
added a commit
that referenced
this pull request
Jan 3, 2023
GopherJS 1.18 has standardized on a single value for GOOS and GOARCH in PR #1111. Update the README accordingly. (There has been agreement in issue #1169 that it's okay to be updating the top-level README for GopherJS 1.18-specific changes by now.) Also reword advice in compatibility.md to be more general. The upstream Go WebAssembly port (i.e., GOOS=js GOARCH=wasm, and notably not wasm64) is limited to 4 GiB of linear memory, it just happens to at this time use 64-bit variables for int/uint/uintptr types. So maybe it's not a great example to refer to. But a reminder that some types vary in size based on the architecture or implementation is probably worth including anyway.
dmitshur
added a commit
that referenced
this pull request
Jan 3, 2023
GopherJS 1.18 has standardized on a single value for GOOS and GOARCH in PR #1111. Update the README accordingly. (There has been agreement in issue #1169 that it's okay to be updating the top-level README for GopherJS 1.18-specific changes by now.) Also reword advice in compatibility.md to be more general. The upstream Go WebAssembly port (i.e., GOOS=js GOARCH=wasm, and notably not wasm64) is limited to 4 GiB of linear memory, it just happens to at this time use 64-bit variables for int/uint/uintptr types. So maybe it's not a great example to refer to. But a reminder that some types vary in size based on the architecture or implementation is probably worth including anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a large change, which changes the approach GopherJS takes to building standard library. The core objective is to use a single, fixed
GOOS/GOARCH
pair when building standard library, regardless of the host OS. The main benefit of this change is reducing a number of environments we need to support, thus reducing the effort required for new Go version adoption and diagnosing issues for users on operating systems other than Linux. As a side benefit, we can reuse a lot more code standard library has forwasm
, and we no longer need thenode-syscall
extension for basic stuff like file system access.GOOS=js
andGOARCH=wasm
, regardless ofGOOS/GOARCH
set by the user.GOOS=js
andGOARCH=ecmascript
. This makesGOOS=js
equal between GopherJS and Go WebAssembly, whileGOARCH
allows to distinguish the two. In future we may use different values ofGOARCH
likeecmascript6
if we decide to output code using the newer syntax.GOOS
and/orGOARCH
environment variables are explicitly set and not empty, GopherJS will build user code with them. This is a compatibility option with the previous behavior when GopherJS usedGOOS
from the host system. I hope to remove it after a couple of major releases.gopherjs
build tag is always set, allowing to target GopherJS regardless of theGOOS/GOARCH
.node-syscall
module, we now use the same code paths as Go WebAssembly for interaction with the os. In particular, we emulate file system syscalls using Node.jsfs
API if it is available. I expect that for most usersnode-syscall
module will now become unnecessary.node-syscall
code paths by building the code withlegacy_syscall
build tag, which provides old implementations forSyscall
,Syscall6
,RawSyscall
andRawSyscall6
functions in thesyscall
package. However, the standard library will still be built withGOOS=js GOARCH=wasm
, so this option is mostly left for the users who used the syscall package directly for operations that can't be emulated using Node.js APIs. I hope we will be able to remove this completely in future.In the long term this change unlocks various improvements to the GopherJS build system, including adoption of
golang.org/x/tools/go/packages
instead ofgo/build
. Additional notes and context on this change can be found here.Fixes #693.