myitcv / gopherjs

A compiler from Go to JavaScript for running Go code in a browser
BSD 2-Clause "Simplified" License
21 stars 0 forks source link

[WIP] compiler: upgrade to Go 1.13 #50

Open myitcv opened 5 years ago

myitcv commented 5 years ago

This is an initial cut of 1.13 support. internal/reflectlite is a new package in Go 1.13 that presents a subset of the API of reflect.

internal/reflectlite support has been added by:

myitcv commented 5 years ago

The main outstanding work here:

type ImportContext struct {
    Packages map[string]*types.Package
    Import   func(path string) (*Archive, error)
}

~to:~

type ImportContext struct {
    Packages map[string]*types.Package
    Import   func(path, dir string) (*Archive, error)
}

~i.e. more akin to go/types.ImporterFrom. This will require returning the ImportMap from go list for each package~

nevkontakte commented 4 years ago

I'm starting figuring out what's broken and what needs fixing. What I did so far:

  1. Checked out your branch (and skimmed through the diff).
  2. Built gopherjs tool.
  3. Attempted gopherjs test fmt, which gave me a printout of build.listPackage and ../../../../../../../usr/local/go/src/flag/flag.go:72:2: could not import fmt (Config.Importer.ImportFrom(fmt, /usr/local/go/src/flag, 0) returned nil but no error).

I suppose this is what you were referring to when saying that the test subcommand is broken. I have a couple of questions to ask, in case you've figured answers out already.

  1. Do you know what changes in Go side led to this breakage? Might be easier to fix the error when we know what was the intent behind changes in Go itself.
  2. Earlier this year Go team was saying that they intend to provide a package for Go tooling developers that would hide all the complexities of build system and changes within it (a.k.a. modules support). I think that package is https://godoc.org/golang.org/x/tools/go/packages. Can we switch to it for source loading and solve the problem once and for all?
nevkontakte commented 4 years ago

It also seems that https://godoc.org/golang.org/x/tools/go/packages would give us modules support for free. The only unclear aspect is how to plug in gopherjs-specific augmentations, but it doesn't seem impossible.

myitcv commented 4 years ago

@nevkontakte thanks for taking a look. In response to your points:

FWIW, I'm using gopherjs test bytes as my starting point because it also has external tests

I suppose this is what you were referring to when saying that the test subcommand is broken.

Exactly.

It also seems that https://godoc.org/golang.org/x/tools/go/packages would give us modules support for free.

You're absolutely correct.

The main reason I didn't use go/packages is that it didn't exist at the time I first made these changes :)

Also, it's somewhat overkill for what we need. We could happily and easily use it, but accessing the transitive imports would then require us to walk the result of packages.Load. Not impossible, but the go list approach is simple enough too.

  1. Do you know what changes in Go side led to this breakage?

I'm not totally clear on this point. To be honest, the GopherJS code has been rather aggressively patched up over the last few years, so it's not exactly in the cleanest state 😄

At this stage I'm in two minds:

  1. re-write github.com/gopherjs/gopherjs/build completely
  2. try to complete my debugging of what's going wrong

The stage I've currently reached with approach 2 is close; I'm just debugging where and why the external test package is getting some incorrect .go files.

I'll try and find a bit of time to dig a bit further then push any updates with comments.

nevkontakte commented 4 years ago

@myitcv thanks for your answers! I was actually thinking to take a shot at rewriting gopherjs/build with go/packages, which might leave us with less code to maintain and some forward compatibility guarantees expectations :)

I have an extra hidden interest in that, since at work I maintain integration between GopherJS and our build system and I had to resort to some inelegant patches and workarounds to achieve that.

Do you think it would be worth for me to try the option # 1 in parallel to you, or try to focus on # 2 together?

myitcv commented 4 years ago

@nevkontakte

Do you think it would be worth for me to try the option # 1 in parallel to you, or try to focus on # 2 together?

Very much so. Please feel free to ping any questions you might have here, and I'll do my best at answering them.

I have an extra hidden interest in that, since at work I maintain integration between GopherJS and our build system and I had to resort to some inelegant patches and workarounds to achieve that.

I'd be interested in hearing more about this. Please drop me an email (in my bio) if you'd prefer not sharing details about that here.

nevkontakte commented 4 years ago

Do you think it would be worth for me to try the option # 1 in parallel to you, or try to focus on # 2 together?

Very much so. Please feel free to ping any questions you might have here, and I'll do my best at answering them.

My English parser has encountered an "undefined behavior" error :) To double check: do you prefer me attempting to switch to go/packages altogether, or try to help you patching up the existing implementation?

myitcv commented 4 years ago

Do you think it would be worth for me to try the option # 1 in parallel to you, or try to focus on # 2 together?

Very much so. Please feel free to ping any questions you might have here, and I'll do my best at answering them.

My English parser has encountered an "undefined behavior" error :) To double check: do you prefer me attempting to switch to go/packages altogether, or try to help you patching up the existing implementation?

Ha! Sorry, I only read the first part of the sentence 😄

So just to clarify: please do attempt to switch to go/packages entirely, effectively dropping use of go/build.

nevkontakte commented 4 years ago

Gotcha :) I'll give it a shot and report back if I find it promising or not.

FrankReh commented 4 years ago

FYI.

I followed this thread this morning and then found https://github.com/gopherjs/gopherjs/pull/959 which includes code for module handling (via a fastmod package) and a gopherjs implementation of reflectlite.

I built from that pull request and can confirm that building gopherjs with go1.13.7, the gopherjs tests pass as do the go1.13.7 "bytes" package unit tests, up to the same point as the gopherjs1.12 was able to, where system calls are required to continue.