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

[WIP] compiler: upgrade to Go 1.13 #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] compiler: upgrade to Go 1.13 #50

wants to merge 2 commits into from

Conversation

myitcv
Copy link
Owner

@myitcv myitcv commented Sep 28, 2019

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 myitcv force-pushed the go1.13 branch 5 times, most recently from 8c0a568 to c90d25a Compare September 28, 2019 09:56
@myitcv
Copy link
Owner Author

myitcv commented Sep 29, 2019

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 myitcv force-pushed the go1.13 branch 6 times, most recently from 25f2f08 to 572a8fd Compare September 30, 2019 14:54
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 according to the following
steps:

* 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).

We also then special case internal/reflectlite in the same places that
reflect is special-cased.

To handle the new core vendor of golang.org/x/net/dns/dnsmessage and
friends, we use .ImportMap from the output of go list to resolve the
correct package for any given import within a package.

WIP - we still need to fix the handling of import paths at test time.
@nevkontakte
Copy link

nevkontakte commented Oct 12, 2019

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
Copy link

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
Copy link
Owner Author

myitcv commented Oct 14, 2019

@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
Copy link

nevkontakte commented Oct 14, 2019

@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
Copy link
Owner Author

myitcv commented Oct 14, 2019

@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
Copy link

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
Copy link
Owner Author

myitcv commented Oct 14, 2019

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
Copy link

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

@FrankReh
Copy link

FrankReh commented Feb 3, 2020

FYI.

I followed this thread this morning and then found gopherjs#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.

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

Successfully merging this pull request may close these issues.

3 participants