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 Go 1.13 and Go 1.14 both #964

Open
wants to merge 50 commits into
base: go1.13
from
Open

Conversation

@visualfc
Copy link

@visualfc visualfc commented Feb 8, 2020

Build on Go 1.13 / Go 1.14 and one build support working on Go 1.12 Go 1.13 Go 1.14 three version.

cd goproj/src
git clone https://github.com/visualfc/gopherjs github.com/gopherjs/gopherjs
cd github.com/gopherjs/gopherjs
git checkout go1.13-dev
go install -v
  1. This version build and install on Go 1.13 or Go 1.14. And one build support working on Go 1.12 1.13 1.14 three version and dynamic select internal compiler/natives/src.
  2. This version build and install on Go 1.12 and working on Go 1.12 only.
  3. if change install go version, please use -a flags to force rebuild packages.
    gopherjs build -a -v or gopherjs test -a -v
  • add internal/reflectlite for Go 1.13 Go 1.14
  • update syscall/js API for Go 1.13 Go 1.14 changes
  • support Go Module, build go project check go.mod ( use go env)
  • add -a (--force) flags to force rebuild packages
  • check installed go version for build ReleaseTags, support working Go 1.12 Go 1.13 Go 1.14 three version
@visualfc visualfc requested review from dmitshur and hajimehoshi Feb 8, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 8, 2020

CI is failing because the circle.yml file in this PR doesn't include the fix for issue #937 that was applied in commit 1852f3a. @visualfc Can you include it?

@visualfc visualfc force-pushed the visualfc:go1.13-dev branch from 916b08b to 2452510 Feb 9, 2020
@visualfc
Copy link
Author

@visualfc visualfc commented Feb 9, 2020

CI is failing because the circle.yml file in this PR doesn't include the fix for issue #937 that was applied in commit 1852f3a. @visualfc Can you include it?

update, but tests failed for CI.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 9, 2020

It's failing because the diff -u <(echo -n) <(go list ./compiler/natives/src/...) step is reporting a non-zero diff. You should add a // +build js constraint to the newly added files in that directory.

@chenrui333
Copy link

@chenrui333 chenrui333 commented Jun 20, 2020

Any updates on this PR? Thanks!

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Jul 4, 2020

Hi @dmitshur, @visualfc and everyone else!

I was suddenly made aware that Go 1.15 is practically around the corner, so soon enough we'll have to begin worrying about supporting that :) Addressing Dmitri's question in #964 (comment) on how to proceed with version branches, I'd like to suggest skipping 1.13 entirely and merging this PR as Go 1.14 support.

This PR is pretty large, though, so I would like to offer my help reviewing it, if would help moving along and everyone is ok with that.

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Jul 26, 2020

Since I received no objections, I'll start reviewing this PR in my spare time. It may take a while due to the size of the change, though.

@visualfc do I understand it correctly that #971 is a stcrict subset of this one?

Copy link
Contributor

@nevkontakte nevkontakte left a comment

I'm finished the first pass over the code and decided to publish the comments I have so far :)

As a general comment, this PR attempts to support 3 Go versions simultaneously, but this seems to add a noticeable amount of complexity. Given that the amount of time GopherJS maintainers' seem to have available these days, it seems to set the project up for increased burden in future. I'd suggest dropping support for Go 1.12 and 1.13, and going with just Go 1.14. Given that 1.15 is coming soon, the value of keeping support for the older versions is quickly diminishing.

@@ -17,7 +17,7 @@ import (

var sizes32 = &types.StdSizes{WordSize: 4, MaxAlign: 8}
var reservedKeywords = make(map[string]bool)
var _ = ___GOPHERJS_REQUIRES_GO_VERSION_1_13___ // Compile error on other Go versions, because they're not supported.
var _ = ___GOPHERJS_REQUIRES_GO_VERSION_1_12___ // Compile error on other Go versions, because they're not supported.

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

Should this be ___GOPHERJS_REQUIRES_GO_VERSION_1_13___ ?

// // Zero returns a Value representing the zero value for the specified type.
// // The result is different from the zero value of the Value struct,
// // which represents no value at all.
// // For example, Zero(TypeOf(42)) returns a Value with Kind Int and value 0.
// // The returned value is neither addressable nor settable.
// func Zero(typ Type) Value {
// if typ == nil {
// panic("reflect: Zero(nil)")
// }
// t := typ.(*rtype)
// fl := flag(t.Kind())
// if ifaceIndir(t) {
// return Value{t, unsafe_New(t), fl | flagIndir}
// }
// return Value{t, nil, fl}
// }

// // ToInterface returns v's current value as an interface{}.
// // It is equivalent to:
// // var i interface{} = (v's underlying value)
// // It panics if the Value was obtained by accessing
// // unexported struct fields.
// func ToInterface(v Value) (i interface{}) {
// return valueInterface(v)
// }

// type EmbedWithUnexpMeth struct{}

// func (EmbedWithUnexpMeth) f() {}

// type pinUnexpMeth interface {
// f()
// }

// var pinUnexpMethI = pinUnexpMeth(EmbedWithUnexpMeth{})

// func FirstMethodNameBytes(t Type) *byte {
// _ = pinUnexpMethI

// ut := t.uncommon()
// if ut == nil {
// panic("type has no methods")
// }
// m := ut.methods()[0]
// mname := t.(*rtype).nameOff(m.name)
// if *mname.data(0, "name flag field")&(1<<2) == 0 {
// panic("method name does not have pkgPath *string")
// }
// return mname.bytes
// }

// type Buffer struct {
// buf []byte
// }
Comment on lines +36 to +89

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

Any reason to keep this code?

// func StructOf(fields []StructField) Type {
// jsFields := make([]*js.Object, len(fields))
// fset := map[string]struct{}{}
// for i, f := range fields {
// if f.Type == nil {
// panic("reflect.StructOf: field " + strconv.Itoa(i) + " has no type")
// }

// name := f.Name
// if name == "" {
// // Embedded field
// if f.Type.Kind() == Ptr {
// // Embedded ** and *interface{} are illegal
// elem := f.Type.Elem()
// if k := elem.Kind(); k == Ptr || k == Interface {
// panic("reflect.StructOf: illegal anonymous field type " + f.Type.String())
// }
// name = elem.String()
// } else {
// name = f.Type.String()
// }
// }

// if _, dup := fset[name]; dup {
// panic("reflect.StructOf: duplicate field " + name)
// }
// fset[name] = struct{}{}

// jsf := js.Global.Get("Object").New()
// jsf.Set("prop", name)
// jsf.Set("name", name)
// jsf.Set("exported", true)
// jsf.Set("typ", jsType(f.Type))
// jsf.Set("tag", f.Tag)
// jsFields[i] = jsf
// }
// return reflectType(js.Global.Call("$structType", "", jsFields))
// }
Comment on lines +403 to +440

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

Same here.

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/hkdf
vendor/golang.org/x/crypto/internal/chacha20
vendor/golang.org/x/crypto/internal/subtle
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/nettest
vendor/golang.org/x/net/route
vendor/golang.org/x/sys/cpu
vendor/golang.org/x/text/secure/bidirule
vendor/golang.org/x/text/transform
vendor/golang.org/x/text/unicode/bidi
vendor/golang.org/x/text/unicode/norm
Comment on lines +68 to +87

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

This is a bit strange. To my understanding, this list should only cover standard library, so seeing vendored packages is a bit unexpected.

"github.com/fsnotify/fsnotify"
"github.com/gopherjs/gopherjs/compiler"
"github.com/gopherjs/gopherjs/compiler/gopherjspkg"
"github.com/gopherjs/gopherjs/compiler/natives"
"github.com/neelance/sourcemap"
"github.com/shurcooL/httpfs/vfsutil"
"golang.org/x/tools/go/buildutil"

"github.com/visualfc/fastmod"

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

As a general observation, lack of comments makes understanding this PR considerably harder, in particular in the part that's related to modules support, the fastmod package godocs are empty.

This, unfortunately, can be said about GopherJS codebase in general too :-(

Archives map[string]*compiler.Archive
Types map[string]*types.Package
Watcher *fsnotify.Watcher
}

func (s *Session) checkMod(pkg *PackageData) (err error) {

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

I think I commented on this on the other PR… Unless I'm misunderstanding the purpose of this function it facilitates passing some module-related information through a side channel, which is a generally problematic pattern (for all the same reasons why global variables are to be avoided).

Aside from that, it creates a risk of hard to debug errors if there's a codepath in which this function is not called.

for _, src := range build.Default.SrcDirs() {
dir := filepath.Join(src, importPath)
if _, err := os.Stat(dir); err == nil {
return dir
}
}
Comment on lines +27 to +32

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

Why is this needed? I'd expect build.Import() to find packages in standard locations. Same below.

@@ -0,0 +1,1048 @@
// +build js

This comment has been minimized.

@nevkontakte

nevkontakte Aug 2, 2020
Contributor

So far I tool only cursory look at reflectlite code. I assume it is mostly derived from GopherJS version of reflect?

Is it possible to somehow reduce the amount of code duplication between the two?

This comment has been minimized.

@visualfc

visualfc Aug 9, 2020
Author

reflectlite clone from goperhjs reflect and comment unused code.

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Aug 9, 2020

I'm getting a feeling like I'm talking to the void here :-/ @hajimehoshi @dmitshur @visualfc Is there any interest in merging this PR at all? Any way I can help advancing this?

@pjebs
Copy link
Contributor

@pjebs pjebs commented Aug 9, 2020

You are correct in your deduction. The project is dead and the maintainers don't care (they have other priorities). I have an ignored PR too but I've known it's been dead for 1+ years.

It's really time for a hard fork for people such as you and I that actually use it in prod. They'll never transfer ownership of repo. I am the owner of https://github.com/rocketlaunchr/react repo which I use in prod and which relies on gopherjs. If I understood the internals, I would hard-fork it in a second.

What I consider to be disrespectful is that the maintainers don't declare that project is dead in the README, leading people like you to spend many many days/weeks issuing PR's, when they know they will be ignored.

Finally it's hard/unwise to call them out because some of them are integral members of the Go team.

@inliquid
Copy link

@inliquid inliquid commented Aug 9, 2020

I'm getting a feeling like I'm talking to the void here :-/ @hajimehoshi @dmitshur @visualfc Is there any interest in merging this PR at all? Any way I can help advancing this?

Seems pretty much so.

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Aug 9, 2020

To be clear, I recognize that GopherJS maintainers may be busy with other things (work, life, etc) and have little or no time to dedicate to the project at this time, but I'd like to hear at least some statement of intentions related to this PR. A hard fork would be very unfortunate and I'd very much prefer to avoid it.

Cheers!

@hajimehoshi
Copy link
Member

@hajimehoshi hajimehoshi commented Aug 9, 2020

Sorry but currently I don't have bandwidth to care this project. Moreover, I'm not familiar with the internal details of GopherJS compared to @dmitshur and the other maintainers.

It's really time for a hard fork for people such as you and I that actually use it in prod.

Actually there are forks like https://github.com/myitcv/gopherjs, which has already merged some PRs in this project.

@visualfc
Copy link
Author

@visualfc visualfc commented Aug 9, 2020

I'm getting a feeling like I'm talking to the void here :-/ @hajimehoshi @dmitshur @visualfc Is there any interest in merging this PR at all? Any way I can help advancing this?

this pr work for go1.14, but not support go1.15. seem support go1.15 is hard.

@aubelsb2
Copy link

@aubelsb2 aubelsb2 commented Aug 9, 2020

@hajimehoshi would it be possible to nominate a successor fork in the readme?

@benma
Copy link

@benma benma commented Aug 10, 2020

I also use this in production. @dmitshur can you please bring some clarity in here? 🙏

@hajimehoshi
Copy link
Member

@hajimehoshi hajimehoshi commented Aug 10, 2020

@aubelsb2 I'm fine to add links to other forks, though my concern is whether this could be fare or not. What do you think, @dmitshur?

CC @myitcv

@flimzy
Copy link
Member

@flimzy flimzy commented Aug 10, 2020

@hajimehoshi would it be possible to nominate a successor fork in the readme?

I think it would be better to see if @neelance and @dmitshur would be willing to promote one or two new owners for this project. WDYT?

@neelance
Copy link
Member

@neelance neelance commented Aug 10, 2020

As a general comment, this PR attempts to support 3 Go versions simultaneously, but this seems to add a noticeable amount of complexity. Given that the amount of time GopherJS maintainers' seem to have available these days, it seems to set the project up for increased burden in future. I'd suggest dropping support for Go 1.12 and 1.13, and going with just Go 1.14. Given that 1.15 is coming soon, the value of keeping support for the older versions is quickly diminishing.

I fully agree with this. The project is already quite complex in what it does, it is better to avoid any additional complexity.

@nevkontakte I haven't talked to @dmitshur yet, but would you be open to being a maintainer of this project? You seem to be invested in GopherJS and I appreciate your code review.

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Aug 10, 2020

@neelance thanks for the offer. The amount of spare time I have is also limited, but I do indeed have a few projects in production that rely on GopherJS. If there's no better volunteer available, I'm happy to join :)

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Sep 17, 2020

Any news? I have multiple reasons to prefer GopherJS over Go/WASM, but at some point being able to use an up to date version will outweigh the advantages of GopherJS and the headache of migration :-(

@pjebs
Copy link
Contributor

@pjebs pjebs commented Sep 17, 2020

If someone hard forks, I'm happy to migrate over

@inliquid
Copy link

@inliquid inliquid commented Sep 18, 2020

@nevkontakte this is from Slack:
изображение

@flimzy
Copy link
Member

@flimzy flimzy commented Sep 18, 2020

:( Sounds like a fork is in order, then.

@neelance
Copy link
Member

@neelance neelance commented Sep 18, 2020

Yes, feel free to create a fork. If it is well maintained for a while, then we can endorse it or maybe even consider a merge at some point. But first Dmitri and I would like to see that the maintenance situation turns out better than what we are currently able to provide. :)

@pjebs
Copy link
Contributor

@pjebs pjebs commented Sep 18, 2020

what should it be called? jsgopher or gojs?

@flimzy
Copy link
Member

@flimzy flimzy commented Sep 18, 2020

what should it be called? jsgopher or gojs?

I suggest we either open a new issue, and/or open a slack conversation about this, and other housekeeping details (which organization, who has commit access, etc, etc) so we don't clutter this PR, and so we have a canonical place to point people for posterity. WDYT?

@nevkontakte
Copy link
Contributor

@nevkontakte nevkontakte commented Sep 18, 2020

@inliquid thanks for bringing the clarity.

@flimzy I suppose #894 is an appropriate place for this kind of discussion. Let's continue it there, shall we?

@visualfc
Copy link
Author

@visualfc visualfc commented Oct 13, 2020

@pjebs
Copy link
Contributor

@pjebs pjebs commented Oct 13, 2020

@visualfc Can you merge it to master in your fork so I can use it? I think it's time to adopt yours as the "official" gopherjs.
If you want, you can create an organization. Perhaps called: "gojs". And then transfer the repo under that organization.

Thanks for the amazing work.

@visualfc
Copy link
Author

@visualfc visualfc commented Oct 15, 2020

https://github.com/goplusjs/gopherjs for supported Go 1.12~1.15

go get -u github.com/goplusjs/gopherjs

all test on macOS
Go1.12
Go1.13
Go1.14
Go1.15

@pjebs
Copy link
Contributor

@pjebs pjebs commented Oct 15, 2020

Does it pass all the go unit tests?

@visualfc
Copy link
Author

@visualfc visualfc commented Oct 30, 2020

Does it pass all the go unit tests?

https://github.com/goplusjs/gopherjs
test pass on macOS Go1.12~Go1.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.