Skip to content

Add support for vendoring gopherjs #678

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add support for vendoring gopherjs #678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ GopherJS compiles Go code ([golang.org](https://golang.org/)) to pure JavaScript
Give GopherJS a try on the [GopherJS Playground](http://gopherjs.github.io/playground/).

### What is supported?
Nearly everything, including Goroutines ([compatibility table](https://github.com/gopherjs/gopherjs/blob/master/doc/packages.md)). Performance is quite good in most cases, see [HTML5 game engine benchmark](https://ajhager.github.io/engi/demos/botmark.html). Cgo is not supported. Using a vendored copy of GopherJS is currently not supported, see [#415](https://github.com/gopherjs/gopherjs/issues/415).
Nearly everything, including Goroutines ([compatibility table](https://github.com/gopherjs/gopherjs/blob/master/doc/packages.md)). Performance is quite good in most cases, see [HTML5 game engine benchmark](https://ajhager.github.io/engi/demos/botmark.html). Cgo is not supported.

### Installation and Usage
Get or update GopherJS and dependencies with:
Expand Down
45 changes: 43 additions & 2 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ func Import(path string, mode build.ImportMode, installSuffix string, buildTags
return importWithSrcDir(path, wd, mode, installSuffix, buildTags)
}

// TODO: figure out how we can determine this per project/per operation so that it can be set and unset properly rather
// than maintaining it as package-level variable state.
var vendoredGopherJS string

func importWithSrcDir(path string, srcDir string, mode build.ImportMode, installSuffix string, buildTags []string) (*PackageData, error) {
buildContext := NewBuildContext(installSuffix, buildTags)
if path == "syscall" { // syscall needs to use a typical GOARCH like amd64 to pick up definitions for _Socklen, BpfInsn, IFNAMSIZ, Timeval, BpfStat, SYS_FCNTL, Flock_t, etc.
Expand All @@ -81,14 +85,21 @@ func importWithSrcDir(path string, srcDir string, mode build.ImportMode, install
buildContext.InstallSuffix += "_" + installSuffix
}
}

// if current path is for gopherjs import and a vendored path for gopherjs has previously been encountered, update
// the path to be the vendored one
if strings.HasPrefix(path, "github.com/gopherjs/gopherjs") && vendoredGopherJS != "" {
path = vendoredGopherJS + strings.TrimPrefix(path, "github.com/gopherjs/gopherjs")
}

pkg, err := buildContext.Import(path, srcDir, mode)
if err != nil {
return nil, err
}

// TODO: Resolve issue #415 and remove this temporary workaround.
// if gopherjs package path is vendored, store it so that we can update future paths
if strings.HasSuffix(pkg.ImportPath, "/vendor/github.com/gopherjs/gopherjs/js") {
return nil, fmt.Errorf("vendoring github.com/gopherjs/gopherjs/js package is not supported, see https://github.com/gopherjs/gopherjs/issues/415")
vendoredGopherJS = strings.TrimSuffix(pkg.ImportPath, "/js")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we be sure that the order of execution will be right? That it will set vendoredGopherJS before its value needs to be used above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that, even though this appears to be true by convention right now, it is brittle and we shouldn't depend on it.

Conceptually, when a build is executed, we want all the gopherjs imports to resolve to some import path (either the canonical one or the vendored one). I think the most common case is that the github.com/gopherjs/gopherjs/js import path that is resolved from the location of the input file(s) is the correct one. Given this, I think I would advocate for something like the following:

  • When build is first invoked, resolve the github.com/gopherjs/gopherjs/js import from the location of the first argument (current working directory if none is specified)
    • There is an edge case here of what to do if multiple input files in different directories are specified -- I think it's probably reasonable to default to import path relative to the first argument for now. If it's really necessary, we could also introduce a flag that allows the path to the vendored gopherjs to be explicitly specified
  • Pass that in as a parameter to this and other functions that need to do import resolution

This makes the entire approach more stateless and explicit based on parameters (and if the vendored path is empty, then we simply don't do any re-writes as the default behavior)

}

switch path {
Expand Down Expand Up @@ -249,6 +260,36 @@ func parseAndAugment(pkg *build.Package, isTest bool, fileSet *token.FileSet) ([
panic(err)
}
r.Close()

if importPath == "runtime" && name == "runtime.go" && vendoredGopherJS != "" {
// special case handling for #415: runtime.go contains a string constant named "gopherJSPath" that
// contains the import path "github.com/gopherjs/gopherjs/js". If we are using a vendored version of
// gopherjs, we must update the contents of the string constant to be the vendored path.
for _, decl := range file.Decls {
switch d := decl.(type) {
case *ast.GenDecl:
switch d.Tok {
case token.CONST:
for _, spec := range d.Specs {
for i, name := range spec.(*ast.ValueSpec).Names {
if name.Name != "gopherJSPath" {
continue
}
if i < len(spec.(*ast.ValueSpec).Values) {
constValue, ok := spec.(*ast.ValueSpec).Values[i].(*ast.BasicLit)
if !ok {
continue
}
// update value of constant to be "js" package within vendored gopherjs
constValue.Value = fmt.Sprintf(`"%s/js"`, vendoredGopherJS)
}
}
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the code would be simpler if we factored this out into an unexported helper?

When it finds and updates the constant value, it can return right away, not having to loop over the rest needlessly. That can be done here to via break Outer, but a return is simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree that it would be better to decomp this function (just wanted to verify that the over-all approach was valid before investing extra time in refactoring)


for _, decl := range file.Decls {
switch d := decl.(type) {
case *ast.FuncDecl:
Expand Down
Loading