gopherjs icon indicating copy to clipboard operation
gopherjs copied to clipboard

[WIP] compiler: upgrade to Go 1.13

Open myitcv opened this issue 5 years ago • 10 comments

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:

  • in the build step, override the .GoFiles for internal/reflectlite to be empty, i.e. we will only take the native (GopherJS) implementation and won't augment any GOROOT definitions
  • taking the current native (GopherJS) implementation of reflect
  • taking the Go 1.13 implementation of reflect
  • adding all files into natives/src/internal/reflectlite (marking the Go 1.13 reflect files as _original.go)
  • progressively removing definitions from the *_original.go files until we get back to a package that compiles. This involves removing certain superfluous definitions (that only exist in the API of reflect) that bring in unwanted imports, e.g. strconv).

myitcv avatar Sep 28 '19 09:09 myitcv

The main outstanding work here:

  • ~Debug failures in github.com/gopherjs/gopherjs/tests that appear to be related to listing of methods for internal/reflectlist.Type values~
  • ~Change github.com/gopherjs/gopherjs/compiler.ImportContext from:~
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~

  • ~We might also need to check that we have the correct cache/lookup keys~ - done
  • Remove all uses of s.wd in build/build.go - they are hacks, and need to be properly replaced with the directory of the package
  • Fix test-related import problems

myitcv avatar Sep 29 '19 12:09 myitcv

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 avatar Oct 12 '19 16:10 nevkontakte

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.

nevkontakte avatar Oct 12 '19 17:10 nevkontakte

@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.

myitcv avatar Oct 14 '19 08:10 myitcv

@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?

nevkontakte avatar Oct 14 '19 18:10 nevkontakte

@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.

myitcv avatar Oct 14 '19 19:10 myitcv

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?

nevkontakte avatar Oct 14 '19 19:10 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.

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.

myitcv avatar Oct 14 '19 20:10 myitcv

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

nevkontakte avatar Oct 14 '19 20:10 nevkontakte

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.

FrankReh avatar Feb 03 '20 19:02 FrankReh