Go 1.9 support (version bump to GopherJS 1.9-1). #651
Conversation
|
A very common test failure right now is: I've investigated it. The cause is that the However, GopherJS doesn't support |
|
The 4 commits above have fixed the issue in #651 (comment). Some small packages, including Next up, individual package test failures. I'm seeing problems because |
|
The 6 commits above implement an initial version of I saw that That's up next. |
|
I've narrowed down the In the 5 commits above, I've implemented missing functions in packages I think |
|
Strange, But fail in CI: (Disabling minification in CI is already coming in handy!) It might be a macOS vs Linux difference. Needs further investigation. |
|
All tests are now passing after the 4 commits above, except the mysterious index out of range panic in I guess I'll spin up a Linux VM and see if I can reproduce it there. |
|
I've investigated the The panic was happening because there's an off-by-one bug in the error handling of that new test. I've reported it and plan to send a CL upstream to fix it. I think I'll add |
|
All tests are passing! ✅ 🎉 There are known TODOs in the PR that can be looked at next. There are some things that require investigation. Help with that is always appreciated. Aside from that, there are no known issues. I plan to start using and testing this new version in order to catch any problems. Help with testing is also welcome! Please report any issues you find. @neelance, now that CI is passing, you could start reviewing the code here (please see commit messages of each individual commit for explanations of changes). |
|
Awesome work! 🚀 I'll try to find some time for a review soon. |
|
Friendly ping @neelance, in case it's helpful. Go 1.9 is expected to be released in August 2017. |
compiler/natives/src/sync/sync.go
Outdated
| var runtime_SemacquireMutex = runtime_Semacquire | ||
| func runtime_SemacquireMutex(s *uint32, lifo bool) { | ||
| // TODO: Use lifo if needed/possible. | ||
| if *s == 0 { |
There was a problem hiding this comment.
Maybe just call runtime_Semacquire here?
There was a problem hiding this comment.
I was a little worried about increasing the call stack by 1 more level in something so low-level, which is why I copied the implementation with a comment. But runtime_SemacquireMutex should be rarely used, since mutex profiling isn't supported. So it should be fine. Done.
compiler/natives/src/time/time.go
Outdated
| n := runtimeNano() | ||
| return n / int64(Second), int32(n % int64(Second)) | ||
| return n / int64(Second), int32(n % int64(Second)), 0 |
There was a problem hiding this comment.
I'm surprised that returning zero works. Is there a test that actually depends on it?
| @@ -491,7 +491,8 @@ func makechan(typ *rtype, size uint64) (ch unsafe.Pointer) { | |||
| return unsafe.Pointer(js.Global.Get("$Chan").New(jsType(ctyp.elem), size).Unsafe()) | |||
| } | |||
|
|
|||
| func makemap(t *rtype) (m unsafe.Pointer) { | |||
| func makemap(t *rtype, cap int) (m unsafe.Pointer) { | |||
| // TODO: Use cap if needed/possible. | |||
There was a problem hiding this comment.
I think there is no way to properly use cap here, so the comment can be removed.
| package signal | ||
|
|
||
| // Package signal is not implemented for GOARCH=js. | ||
| // TODO: Implement if possible. |
There was a problem hiding this comment.
I don't see any reason why we would need to implement it. We don't have signals in the browser.
|
|
||
| func callerName(skip int) string { | ||
| // TODO: Implement if possible. | ||
| return "caller" |
There was a problem hiding this comment.
Maybe better return something along "<unknown>".
compiler/natives/src/math/big/big.go
Outdated
| // Remove after that issue is resolved. | ||
| type Word uintptr | ||
|
|
||
| // TODO: Consider using math_big_pure_go build tag for this package, instead of |
circle.yml
Outdated
| @@ -18,7 +18,7 @@ test: | |||
| - go tool vet *.go # Go package in root directory. | |||
| - for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party". | |||
| - > | |||
| gopherjs test --short --minify | |||
| gopherjs test --short | |||
There was a problem hiding this comment.
Minification can sometimes break the code, for example if there are missing semicolons. CI should cover that.
There was a problem hiding this comment.
As discussed, I'll run a short test for minification to cover it. Say, the fmt package:
- gopherjs test -v --minify fmt # Minification should work.Edit: Done in 2587a1c.
compiler/natives/src/time/time.go
Outdated
| n := runtimeNano() | ||
| return n / int64(Second), int32(n % int64(Second)), 0 | ||
| return n / int64(Second), int32(n % int64(Second)), n |
There was a problem hiding this comment.
Okay, this answers my earlier comment.
There was a problem hiding this comment.
Go 1.9 Release Candidate 1 came out today. I've updated the go1.9 branch to be compatible (there were a few minor changes that needed to be accounted for).
I've also addressed all of the review comments.
PTAL.
I'm going to announce this WIP version so that everyone who's interested in using Go 1.9 RC 1 and also help test out GopherJS 1.9-wip can do so. I'm using this as my primary GopherJS version.
Edit: Tweeted out https://twitter.com/GopherJS/status/889948789037047809.
compiler/natives/src/sync/sync.go
Outdated
| var runtime_SemacquireMutex = runtime_Semacquire | ||
| func runtime_SemacquireMutex(s *uint32, lifo bool) { | ||
| // TODO: Use lifo if needed/possible. | ||
| if *s == 0 { |
There was a problem hiding this comment.
I was a little worried about increasing the call stack by 1 more level in something so low-level, which is why I copied the implementation with a comment. But runtime_SemacquireMutex should be rarely used, since mutex profiling isn't supported. So it should be fine. Done.
| @@ -491,7 +491,8 @@ func makechan(typ *rtype, size uint64) (ch unsafe.Pointer) { | |||
| return unsafe.Pointer(js.Global.Get("$Chan").New(jsType(ctyp.elem), size).Unsafe()) | |||
| } | |||
|
|
|||
| func makemap(t *rtype) (m unsafe.Pointer) { | |||
| func makemap(t *rtype, cap int) (m unsafe.Pointer) { | |||
| // TODO: Use cap if needed/possible. | |||
| package signal | ||
|
|
||
| // Package signal is not implemented for GOARCH=js. | ||
| // TODO: Implement if possible. |
|
|
||
| func callerName(skip int) string { | ||
| // TODO: Implement if possible. | ||
| return "caller" |
circle.yml
Outdated
| @@ -18,7 +18,7 @@ test: | |||
| - go tool vet *.go # Go package in root directory. | |||
| - for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party". | |||
| - > | |||
| gopherjs test --short --minify | |||
| gopherjs test --short | |||
There was a problem hiding this comment.
As discussed, I'll run a short test for minification to cover it. Say, the fmt package:
- gopherjs test -v --minify fmt # Minification should work.Edit: Done in 2587a1c.
|
Go 1.9 final release is out! I've updated this PR to target it. I'll be doing some final checks and merging this to As usual, a |
circle.yml
Outdated
| - > | ||
| gopherjs test --short --minify | ||
| gopherjs test -v --short |
There was a problem hiding this comment.
Why drop the minify here?
It actually helps to catch errors in the compiled Javascript (if changes are being made to the GopherJS compiler).
There was a problem hiding this comment.
Explained in b827aae and discussed with @neelance at #651 (comment), with the follow up done in 2587a1c.
What are your thoughts after seeing that?
There was a problem hiding this comment.
@neelance makes the same point about the --minify flag essentially being a means of ensuring the generated Javascript code is valid in #651 (comment)
By only testing --minify with fmt we are assuming that we get complete coverage of generated output... I'm not sure I could easily reason that that's the case.
I take you point about the stack traces... if there's no particular time constraints we could do two test runs, one without --minify followed by one with. That way we get the best of both worlds.
Thoughts?
There was a problem hiding this comment.
In b827aae, I wrote:
I don't see a strong reason to use minification during tests. Output
size is not a concern.
You said:
By only testing
--minifywithfmtwe are assuming that we get complete coverage of generated output... I'm not sure I could easily reason that that's the case.
I agree. I think that's a good reason to keep --minify flag.
It is, after all, the de-facto standard of how majority of production GopherJS code is generated, so testing it is more important than non-minified code. It's not worth sacrificing minified code test coverage that for the sake of convenience of having nicer CI error reports in the (rare) case of errors.
I'll revert that change. Thanks.
I take you point about the stack traces... if there's no particular time constraints we could do two test runs, one without
--minifyfollowed by one with. That way we get the best of both worlds.
I don't quite like it because it'd mean you'd see CI build results slower. It's already quite slow at 17~ minutes. Also, it'd feels like a waste of resources, given that the expected behavior is that --minify does not change behavior, and I would like that to be testable without running all tests.
Let's leave this for later.
There was a problem hiding this comment.
I don't quite like it because it'd mean you'd see CI build results slower.
It ought to be easy to run the non-minified tests only in the case of a failure of the minified tests, to pay that penalty only in the rare case of failure.
There was a problem hiding this comment.
It ought to be easy to run the non-minified tests only in the case of a failure of the minified tests, to pay that penalty only in the rare case of failure.
I was thinking that could be close to ideal behavior. But it doesn't seem very easy to me. If you think it's easy, it'd be helpful if you could open an issue and provide details of how to accomplish it.
There was a problem hiding this comment.
@shurcooL we can easily speed up the tests; they're effectively bound to a single core the way we run them. I've got some rather hacky code locally to runs gopherjs test X concurrently up to the number of cores. I'll try and polish that up and submit a PR. It works in much the same way as the runner in tests/run.go.
| @@ -107,6 +109,7 @@ test: | |||
| net/rpc/jsonrpc | |||
| net/textproto | |||
| net/url | |||
| os/user | |||
There was a problem hiding this comment.
Would this list be better maintained as an exclusion list? Not that there are many new packages added to core... but still.
There was a problem hiding this comment.
I agree, that'd be better. Not as part of this PR, but let's make a follow up enhancement PR after this one is merged.
Edit: Filed #686 to track it.
circle.yml
Outdated
| @@ -127,3 +130,4 @@ test: | |||
| unicode/utf16 | |||
| unicode/utf8 | |||
| - go test -v -race ./... | |||
| - gopherjs test -v --minify fmt # Minification should work. | |||
There was a problem hiding this comment.
See comment above.
Running with --minify on the standard library increases the test surface area for minified code which is a good thing.
|
I'm currently investigating the |
In Go 1.9, runtime.panicwrap uses throw. So it needs to be accessible from Go code now. Fixes: ../src/runtime/error.go:134:3: undeclared name: throw ../src/runtime/error.go:144:3: undeclared name: throw ../src/runtime/error.go:148:3: undeclared name: throw ../src/runtime/error.go:153:3: undeclared name: throw ../src/runtime/error.go:156:3: undeclared name: throw
In Go 1.9, runtime.panicwrap uses CallersFrames. So it needs to be implemented (as well as the types it uses). Create a noop implementation for now to resolve an undeclared name error. A more functioning implementation may be possible, and needs to be investigated (see Caller implementation for inspiration). Fixes: ../src/runtime/error.go:136:12: undeclared name: CallersFrames
…res. These take extra parameters now. Add a TODO comment to make use of them if needed/possible. Requires investigation. Fixes: $GOROOT/src/sync/mutex.go:201:33: too many arguments $GOROOT/src/sync/mutex.go:211:31: too many arguments $GOROOT/src/sync/waitgroup.go:94:32: too many arguments $GOROOT/src/sync/rwmutex.go:73:38: too many arguments $GOROOT/src/sync/rwmutex.go:126:37: too many arguments
Fixes: $GOROOT/src/time/time.go:1049:21: cannot initialize 3 variables with 2 values $GOROOT/src/time/zoneinfo_read.go:202:15: cannot initialize 3 variables with 2 values
chanrecv, chansend no longer accept t *rtype first parameter. Conveniently, it was unused anyway. makemap now accepts an additional cap int paraameter. A TODO comment is added to make use of it, if needed/possible. structField's offset field was renamed/combined with anonymous bit to become offsetAnon. The least significant bit is the anonymous bit, and byte offset of field is stored shifted left by 1 bit.
go generate ./...
Package testing now imports os/signal as of Go 1.9, which it didn't before. A no-op implementation fixes the build. Fixes: Error: runtime error: native function not implemented: os/signal.signal_enable
All the packages under ./compiler/natives/src/... are expected to have GopherJS-specific overrides for native Go packages. So, they should all have // +build js build constraint. Add a check for this to CI so it doesn't regress.
go generate ./...
go generate ./...
This is a fixup for 9a32def. That commit tried to introduce a CI check for catching when .go files in compiler/natives/src are accidentally missing // +buid js build constraints. However, due to the weird Circle CI setup with symlinks, that CI check wasn't actually going to catch any issues, as proven by test commit a774498 and that it passed that check. However, test commit 7aebc06 showed that relative import path patterns work okay with the Circle CI setup, so use that instead.
This changes fixes GopherJS ability to build os/user package. In turn,
this change also fixes crypto/x509 and net/http on darwin. The goal is
to ensure that os/user builds without errors, because it may end up
being imported by some stdlib packages (e.g., crypto/x509 on darwin
in Go 1.9).
This is accomplished by disabling cgo for these standard library
packages. Normally, GopherJS build context enables cgo for the purpose
of catching when a user package uses cgo and reporting that:
CgoEnabled: true, // detect `import "C"` to throw proper error
...
if len(pkg.CgoFiles) > 0 {
return nil, &ImportCError{path}
}
But we don't want to use cgo for standard library packages. They have
support for building without cgo by using !cgo build constraints, which
we want to activate. Setting bctxt.CgoEnabled to false achieves that.
Add to CI a test of os/user package for gopherjs, as well as ensure
net/http builds successfully. We can't run its tests because there
are too many and they require the HTTP server, but only client is
implemented in GopherJS.
Fixes #664.
Found the root cause. It was suboptimal. I've restored the previous behavior (to avoid an unnecessary and unintended change of behavior) with commit 853e795. Also rebased on top of latest master (it was 2 commits behind), in preparations of the final merge. |
If its code were to be generated, package unsafe ends up largely empty:
$packages["unsafe"] = (function() {
var $pkg = {}, $init;
$init = function() {
$pkg.$init = function() {};
...
};
$pkg.$init = $init;
return $pkg;
})();
It contains an empty (aside from goroutine/blocking bookkeeping code) init
function, and that's the only thing that gets called by other packages.
So, we don't want to start including it. This change restores the previous
behavior (with Go 1.8) of skipping it.
Helps gopherjs/gopherjs.github.io#67 (comment).
Related to https://golang.org/cl/37694.
This largely reverts commit b9c0354. It was originally done for convenience, to improve readability of stack traces during failures. However, it's more important to prioritize doing test coverage of the same build mode as used in production (minification on). That's a sufficient reason to keep the --minify flag for gopherjs test of all packages.
The copied importer is an old package from Go 1.5 times. It was a copy of x/tools/go/importer package. It does not support type aliases added in Go 1.9. Encountering a type alias declaration caused a panic. Use the gcimporter15 package instead. It supports a more modern binary export format and includes support for type aliases. The gcimporter15 package is marked as deprecated and said to be deleted in October 2017. However, the replacement package does not expose the functionality suitable for GopherJS current needs. That means we'll need to find a resolution by the time it's deleted. It will either be a feature request to add the needed functionality, a rewrite of GopherJS code to use available functionality, or vendoring/copying the code. Updates #278.
There's no need to consider type aliases when looking to compute method sets. Type aliases cannot have a different method set than the aliased type. Even methods defined on the type alias are still associated with the aliased type. This is a refactor that shouldn't change behavior, just clean up and better organize the code.
|
The release has been delayed a bit because I found issues with type aliases in the last minute. Using type aliases in a package main caused the compiler to panic, because the exporter it used didn't expect that a type declaration type could be anything other than I've fixed the issue in be67827, and further simplified some type alias code in I found it hard to write good tests for type aliases because the issues were relevant to the package export data, so it matters whether its a command/library, and whether the package is being compiled or an archive being loaded. Type alias tests will be helpful and needed, but I don't necessarily want to block the 1.9-1 release on them. I think it'd be better to get it out sooner to unblock other PRs; we can still address any bugs people find after the initial release, and I think that'd be more efficient. I believe everything should be ready for the final release, but because of the unexpected last minute fixes, I want to give some time for further testing. Also, a review of those 2 commits would be highly appreciated. |
compiler/package.go
Outdated
| @@ -468,39 +468,38 @@ func Compile(importPath string, files []*ast.File, fileSet *token.FileSet, impor | |||
| c.Printf(`%s = $newType(%d, %s, "%s.%s", %t, "%s", %t, %s);`, lhs, size, typeKind(o.Type()), o.Pkg().Name(), o.Name(), o.Name() != "", o.Pkg().Path(), o.Exported(), constructor) | |||
| }) | |||
| d.MethodListCode = c.CatchOutput(0, func() { | |||
| if _, isInterface := o.Type().Underlying().(*types.Interface); isInterface { | |||
| if o.IsAlias() { | |||
There was a problem hiding this comment.
Shouldn't this filter be already at a higher level, e.g. at line 436? As far as I can see, type aliases only exist at the source level, they should not occur in the compiler output any more.
There was a problem hiding this comment.
Quite possibly. However, I can't answer with confidence, it would require further investigation.
I am quite confident in its current location because of my understanding of methods (a type alias may not have any additional methods; they're always associated with the aliased type instead), and also because I've inspected the generated code and confirmed it looked good.
One thing to find out, if this is moved higher level, is what happens regarding DCE when a type is only ever referenced via its type alias. E.g.:
package p
type T int
type A = Tpackage main
import "p"
func main() {
var a p.A = 123
fmt.Printf("%T %v\n", a, a)
}There was a problem hiding this comment.
I wouldn't anticipate there being a problem in this situation, because everything is done based on the derived type of expressions. Aliases have no impact on that calculation because they've effectively already been erased.
There was a problem hiding this comment.
e.g.
package main
import (
"fmt"
"go/ast"
"go/importer"
"go/parser"
"go/token"
"go/types"
"log"
)
const hello = `package main
import "bytes"
type T = bytes.Buffer
var x T
func main() {
}
`
func main() {
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "hello.go", hello, 0)
if err != nil {
log.Fatal(err)
}
conf := types.Config{Importer: importer.Default()}
pkg, err := conf.Check("cmd/hello", fset, []*ast.File{f}, nil)
if err != nil {
log.Fatal(err)
}
fmt.Printf("x: %s\n", pkg.Scope().Lookup("x").Type())
}gives:
x: bytes.Buffer
hence the dependency on bytes.Buffer is derived.
There was a problem hiding this comment.
Fixed typo in my closing sentence...
There was a problem hiding this comment.
That sounds reasonable. I don't have objections to trying it out. The basic tests I've tried all pass.
Edit: Pushed 34e2783.
There was a problem hiding this comment.
Cool! I prefer this solution because it is less likely to hide bugs (e.g. some bad runtime reference to a type alias).
The other named type compiler output shouldn't be needed for type aliases.
|
GopherJS 1.9-1 is now released and available on See blog post about it here: https://medium.com/gopherjs/gopherjs-1-9-1-is-released-e4d808e58f19. See tweet here: https://twitter.com/GopherJS/status/901611150420344832. Thanks everyone! 🎉 |
|
Just done a quick spot check; looks like my minimal |
|
If you don't tell us its absolute size, it's not possible to know whether 20 KB is a significant change. Let's continue generated code size discussion in #136. It's normal and unavoidable to see an increase after an upgrade to a new release of Go due to the upstream issue golang/go#6853. |
|
Sorry that's my bad; 20KB on top of ~740KB. But you're right; this time it's almost certainly a result of upstream changes. We don't track size at all as part of the CI build; I think it would be worth adding some sort of output so we can trace things over time, if only to spot significant changes as a result of non upstream changes. |
This is a WIP PR to track work to support Go 1.9.
go1.9beta1has been released.Each commit is bite-sized, with rationale and description, and fixes a specific issue.
It's at a very early stage, I've only just started making the neccessary updates. It's functional enough to compile a
fmt.Println("hello")program, but many things remain to be done and fixed. Some packages will fail to compile, many tests will fail. I'll post updates as I make progress.