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

Support Go1.12 #900

Open
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@hajimehoshi
Copy link
Member

hajimehoshi commented Mar 3, 2019

This PR fixes implementation and tests in order to support Go 1.12.

Fixes #887

PTAL

hajimehoshi added some commits Mar 2, 2019

compiler/natives/src/syscall: Rename syscall
On Darwin, syscall was instroduced at golang/go@a3b0144.

This change renames syscall to avoid collision.
Update circle.yml
`go tool vet` is abandoned. Use `go vet` instead.
compiler/natives/src: Fix some test failures
Still work in progress.

@hajimehoshi hajimehoshi requested review from dmitshur , myitcv and neelance Mar 3, 2019

circle.yml Outdated
- run: go tool vet *.go # Go package in root directory.
- run: for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party".
- run: go vet *.go # Go package in root directory.
- run: for d in */; do echo ./$d...; done | grep -v ./doc | grep -v ./test | grep -v ./node | xargs go vet # All subdirectories except "doc", "tests", "node*".

This comment has been minimized.

@neelance

neelance Mar 3, 2019

Member

Please explain this change.

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 4, 2019

Author Member
  • go tool vet is deprecated and go vet is the substitute. (Error)
  • It looked like go vet required the relative paths for local packages. I thought this line tries to go vet against part of the local directories. Is that correct?

This comment has been minimized.

@dmitshur

dmitshur Mar 4, 2019

Member

This change makes sense to me, as @hajimehoshi described above.

A few questions/suggestions:

  1. I suspect the go tool vet *.go line can be simplified even further to just go vet, or go vet ., to run vet on the Go package with the import path github.com/gopherjs/gopherjs. It was *.go previously because go tool vet did not accept import paths, only files and directories.

  2. Does doc directory need to be skipped? It has just 2 .md files, so the import path pattern ./doc/... should not be matching any packages.

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 5, 2019

Author Member

Done.

go vet ./doc/... fails with the exit code non-zero, and I thought I needed to avoid.

Would go vet ./... work btw?

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 5, 2019

Author Member
hajimehoshi@Hajimes-MacBook-Pro ~/go/src/github.com/gopherjs/gopherjs
$ go vet ./doc/...
go: warning: "./doc/..." matched no packages
no packages to vet

hajimehoshi@Hajimes-MacBook-Pro ~/go/src/github.com/gopherjs/gopherjs
$ echo $?
1

This comment has been minimized.

@dmitshur

dmitshur Mar 9, 2019

Member

Would go vet ./... work btw?

I don't think it would, because the intention is to skip "tests" directory from being vetted.

Show resolved Hide resolved compiler/natives/src/syscall/syscall_unix.go Outdated
Show resolved Hide resolved compiler/version_check.go
kv = iter.last
} else {
// Compare the index and the size of the actual key set, and check if the iterator is already exhausted.
if iter.i >= js.Global.Call("$keys", iter.m).Length() {

This comment has been minimized.

@neelance

neelance Mar 3, 2019

Member

Calling $keys again does not look right at first glance. Which bug is this supposed to fix?

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 4, 2019

Author Member

Value tries to access mapiterkeys at the second and later Next call (code, and iter.keys can be already not synced with $keys by deleting keys in a loop.

On second thought, my fix might not work with the below code:

iter := ValueOf(m).MapRange()
for iter.Next() {
    delete(m, iter.Key())
}
if len(m) != 0 {
    panic("not reached")
}

Let me think...

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 5, 2019

Author Member

Fixed.

@hajimehoshi
Copy link
Member Author

hajimehoshi left a comment

Thank you!

circle.yml Outdated
- run: go tool vet *.go # Go package in root directory.
- run: for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party".
- run: go vet *.go # Go package in root directory.
- run: for d in */; do echo ./$d...; done | grep -v ./doc | grep -v ./test | grep -v ./node | xargs go vet # All subdirectories except "doc", "tests", "node*".

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 4, 2019

Author Member
  • go tool vet is deprecated and go vet is the substitute. (Error)
  • It looked like go vet required the relative paths for local packages. I thought this line tries to go vet against part of the local directories. Is that correct?
kv = iter.last
} else {
// Compare the index and the size of the actual key set, and check if the iterator is already exhausted.
if iter.i >= js.Global.Call("$keys", iter.m).Length() {

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 4, 2019

Author Member

Value tries to access mapiterkeys at the second and later Next call (code, and iter.keys can be already not synced with $keys by deleting keys in a loop.

On second thought, my fix might not work with the below code:

iter := ValueOf(m).MapRange()
for iter.Next() {
    delete(m, iter.Key())
}
if len(m) != 0 {
    panic("not reached")
}

Let me think...

Show resolved Hide resolved compiler/natives/src/syscall/syscall_unix.go Outdated
Show resolved Hide resolved compiler/version_check.go
@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 4, 2019

I found another problem: the test doesn't work well on macOS (e.g. go run -tags=gopherjsdev . test --short reflect takes forever). I'll fix this later before merging this.

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 5, 2019

In the current situation:

hajimehoshi@Hajimes-MacBook-Pro ~/go/src/github.com/gopherjs/gopherjs
$ go build . && env GOOS=linux ./gopherjs test -v github.com/gopherjs/gopherjs/tests/main
gopherjs: Source maps disabled. Install source-map-support module for nice stack traces. See https://github.com/gopherjs/gopherjs#gopherjs-run-gopherjs-test.                                                                             
=== RUN   TestNotRunMain
--- PASS: TestNotRunMain (0.00s)
PASS
fatal error: all goroutines are asleep - deadlock!
FAIL    github.com/gopherjs/gopherjs/tests/main 0.268s
hajimehoshi@Hajimes-MacBook-Pro ~/go/src/github.com/gopherjs/gopherjs

$ go build . && env GOOS=darwin ./gopherjs test -v github.com/gopherjs/gopherjs/tests/main
gopherjs: Source maps disabled. Install source-map-support module for nice stack traces. See https://github.com/gopherjs/gopherjs#gopherjs-run-gopherjs-test.                                                                             
warning: system calls not available, see https://github.com/gopherjs/gopherjs/blob/master/doc/syscalls.md
fatal error: all goroutines are asleep - deadlock!
FAIL    github.com/gopherjs/gopherjs/tests/main 0.246s

Now I'm investigating why the tests don't work on Darwin.

EDIT: This was a syscall issue. Fixed!

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Mar 5, 2019

Did you install source-map-support module and syscall module? The ones mentioned at https://github.com/gopherjs/gopherjs#gopherjs-run-gopherjs-test. They used to be more optional but by now I think they're mandatory for some tests.

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 5, 2019

Did you install source-map-support module and syscall module?

No, but I think the cause was that the write implementation was not used on Darwin and I fixed syscall infrastructure for Darwin.

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 5, 2019

Now all the known issues have been solved. Please take another look. Thank you!

@hajimehoshi hajimehoshi requested a review from neelance Mar 5, 2019

@myitcv myitcv referenced this pull request Mar 7, 2019

Merged

compiler: upgrade to Go 1.12 #41

@myitcv

This comment has been minimized.

Copy link
Member

myitcv commented Mar 8, 2019

Just adding some notes based on us applying roughly the same patch to myitcv#41:

  • I think the *_js.go files need to be renamed and build constraints used
  • Because the CircleCI config file specifies the standard library tests to run (as opposed to a set negation approach) this PR is not running tests in some of the new standard library packages, crucially internal/fmtsort, so the fact the tests are passing for this PR is (I think) currently a false positive
$ diff -wu 1.11 1.12
--- 1.11        2019-03-08 18:46:10.181963351 +0000
+++ 1.12        2019-03-08 18:46:02.570274117 +0000
@@ -88,6 +88,8 @@
 index/suffixarray
 internal/bytealg
 internal/cpu
+internal/fmtsort
+internal/goroot
 internal/nettrace
 internal/poll
 internal/race
@@ -99,6 +101,27 @@
 internal/testenv
 internal/testlog
 internal/trace
+internal/x/crypto/chacha20poly1305
+internal/x/crypto/cryptobyte
+internal/x/crypto/cryptobyte/asn1
+internal/x/crypto/curve25519
+internal/x/crypto/hkdf
+internal/x/crypto/internal/chacha20
+internal/x/crypto/poly1305
+internal/x/net/dns/dnsmessage
+internal/x/net/http/httpguts
+internal/x/net/http/httpproxy
+internal/x/net/http2/hpack
+internal/x/net/idna
+internal/x/net/internal/nettest
+internal/x/net/nettest
+internal/x/text/secure
+internal/x/text/secure/bidirule
+internal/x/text/transform
+internal/x/text/unicode
+internal/x/text/unicode/bidi
+internal/x/text/unicode/norm
+internal/xcoff
 io
 io/ioutil
 log
@@ -143,6 +166,7 @@
 runtime/cgo
 runtime/debug
 runtime/internal/atomic
+runtime/internal/math
 runtime/internal/sys
 runtime/pprof
 runtime/pprof/internal/profile
@@ -167,22 +191,3 @@
 unicode/utf16
 unicode/utf8
 unsafe
-vendor/golang_org/x/crypto/chacha20poly1305
-vendor/golang_org/x/crypto/cryptobyte
-vendor/golang_org/x/crypto/cryptobyte/asn1
-vendor/golang_org/x/crypto/curve25519
-vendor/golang_org/x/crypto/internal/chacha20
-vendor/golang_org/x/crypto/poly1305
-vendor/golang_org/x/net/dns/dnsmessage
-vendor/golang_org/x/net/http/httpguts
-vendor/golang_org/x/net/http/httpproxy
-vendor/golang_org/x/net/http2/hpack
-vendor/golang_org/x/net/idna
-vendor/golang_org/x/net/internal/nettest
-vendor/golang_org/x/net/nettest
-vendor/golang_org/x/text/secure
-vendor/golang_org/x/text/secure/bidirule
-vendor/golang_org/x/text/transform
-vendor/golang_org/x/text/unicode
-vendor/golang_org/x/text/unicode/bidi
-vendor/golang_org/x/text/unicode/norm
@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 9, 2019

@myitcv Thank you! For the above diff, the original master's circle.yml doesn't include a lot of tests like internal/cpu. Would it be possible to decide to care such packages in another PR if needed?

I think the *_js.go files need to be renamed and build constraints used

Done.

@myitcv

This comment has been minimized.

Copy link
Member

myitcv commented Mar 9, 2019

@hajimehoshi

For the above diff, the original master's circle.yml doesn't include a lot of tests like internal/cpu. Would it be possible to decide to care such packages in another PR if needed?

I'd suggest switching to the negation-based approach. We can even do that ahead of the Go 1.12 changes.

It's a simple, easy to understand change that means we won't have to manually keep track of new packages. Packages like internal/fmtsort which, per myitcv#41, seem to have caught an issue.

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 9, 2019

I'd suggest switching to the negation-based approach. We can even do that ahead of the Go 1.12 changes.

A blacklist instead of the current whitelist makes sense.

image/color/palette
image/internal/imageutil
internal/cpu
internal/fmtsort

This comment has been minimized.

@myitcv

myitcv Mar 9, 2019

Member

A better starting point for this blacklist is the list of packages that were not tested as part of the 1.11 cycle (hence why I suggested we could/should do that as part of a separate, simple PR).

That would then highlight that you're including internal/fmtsort in this file, i.e. it would be an additional package we're ignoring for Go 1.12.

I still think we should understand why this package in particular is failing, because it's fundamental to the sorting changes in the fmt package and other places:

Package fmtsort provides a general stable ordering mechanism for maps, on behalf of the fmt and text/template packages.

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 9, 2019

Author Member

Sure. As I merged your PR, I'll be investigating why internal/fmtsort tests failed. I agree fmtsort is so fundamental that we need to care.

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 9, 2019

Author Member

The root cause is that getting pointer values from a slice is now counterintuitive

https://gopherjs.github.io/playground/#/jvnZoFsINA

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 9, 2019

Author Member

Ha, nice, my guess is that conversion an integer from a pointer (in reflect?) causes this issue

https://gopherjs.github.io/playground/#/fN4zm6Wj4W

This comment has been minimized.

This comment has been minimized.

@hajimehoshi

hajimehoshi Mar 9, 2019

Author Member

I made the tests pass with these fixes:

  • For pointers and channels, the current GopherJS cannot treat them well for sorting. Skip this.
  • GopherJS cannot treat nil key in a map well. Skip this.

myitcv added a commit to myitcv/gopherjs that referenced this pull request Mar 9, 2019

ci: use black list instead of white list for standard library tests
Using a black list is a more effective mechanism for expressing those
standard library packages we don't want to test because it means we
don't need to manually check whether new standard library packages have
been added that would otherwise need to be added to the white list.

See the discussion in gopherjs#900 for
more context.

myitcv added a commit to myitcv/gopherjs that referenced this pull request Mar 9, 2019

ci: upgrade to Go 1.11.5
So that we can then see the true diff that the Go 1.12 changes in
gopherjs#900 will introduce.

@myitcv myitcv referenced this pull request Mar 9, 2019

Merged

ci: upgrade to Go 1.11.5 #903

hajimehoshi added a commit that referenced this pull request Mar 9, 2019

ci: use black list instead of white list for standard library tests (#…
…902)

Using a black list is a more effective mechanism for expressing those
standard library packages we don't want to test because it means we
don't need to manually check whether new standard library packages have
been added that would otherwise need to be added to the white list.

See the discussion in #900 for
more context.

myitcv added a commit to myitcv/gopherjs that referenced this pull request Mar 9, 2019

ci: upgrade to Go 1.11.5
So that we can then see the true diff that the Go 1.12 changes in
gopherjs#900 will introduce.

myitcv added a commit that referenced this pull request Mar 9, 2019

ci: upgrade to Go 1.11.5 (#903)
So that we can then see the true diff that the Go 1.12 changes in
#900 will introduce.

hajimehoshi added some commits Mar 9, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

hajimehoshi commented Mar 18, 2019

Friendly ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.