Embed core GopherJS packages into build system; enable vendoring of GopherJS. #787

dmitshur · 2018-04-08T01:25:42Z

This change brings two major features:

It allows the gopherjs binary to be completely standalone and not require GopherJS source code to be in $GOPATH. Only $GOROOT is required.
It allows the source code of gopherjs to be vendored in a Go project, and the vendored copy of gopherjs will be able to compile Go code successfully.

The approach taken to make these things possible is as follows.

As of commit ab917e0, the natives (GopherJS-specific overrides for the Go standard library packages) are embedded in the GopherJS build system via a virtual filesystem. We re-generate the VFS whenever natives are modified.

Doing that made it possible to compile most parts of the Go standard library with GopherJS without needing the github.com/gopherjs/gopherjs repository in $GOPATH. However, without the github.com/gopherjs/gopherjs/js and github.com/gopherjs/gopherjs/nosync packages, compiling the Go standard library is not possible. Those two packages are core to the GopherJS compiler and are needed to build runtime, etc.

Therefore, since natives are already embedded, it makes a lot of sense to also embed the 2 small additional github.com/gopherjs/gopherjs/js and github.com/gopherjs/gopherjs/nosync packages analogously. Then, the VFS is used whenever the source of those two packages is accessed.

As a result, a compiled gopherjs binary can successfully compile Go code even if the GopherJS repository is not located in $GOPATH.

Additionally, the natives and gopherjspkg packages are very friendly towards being vendored. That means the entire GopherJS project can be vendored in another Go project, and then used to compile Go code.

Fixes #462.
Fixes #415.

hajimehoshi · 2018-04-08T05:28:21Z

I'd be happy if you could tell me the steps to test this.

hajimehoshi · 2018-04-08T06:07:41Z

compiler/gopherjspkg/doc.go

+//
+package gopherjspkg
+
+//go:generate vfsgendev -source="github.com/gopherjs/gopherjs/compiler/gopherjspkg".FS -tag=gopherjsdev


Where does vfgendev come from?

See https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#code-generation.

Please can we make this go:generate directive not rely on vfsgendev being in PATH?

That's out of scope for this PR. This gopherjspkg package is the same in structure as natives.

Good point. This would be better solved in any case by golang/go#22726

dmitshur · 2018-04-08T06:17:50Z

steps to test this

The way I tested was:

Create an empty directory in /tmp. Set GOPATH to it. go get -d GopherJS, then check out this PR, install it. Make a short test program that imports some packages, including github.com/gopherjs/gopherjs/js. Then remove the entire GopherJS repo from GOPATH. See if gopherjs build, gopherjs run, gopherjs serve work ok.
Create another empty directory in /tmp. Set GOPATH to it. Make a short test program like before. Make a vendor subdirectory and copy GopherJS code into it. Build the vendored gopherjs binary and test it like above.

You can also see #678 (comment) for more ideas.

hajimehoshi · 2018-04-08T07:33:07Z

NEWGOPATH=/tmp/gopherjs

rm -rf $NEWGOPATH
GOPATH=$NEWGOPATH go get github.com/gopherjs/gopherjs
(cd $NEWGOPATH/src/github.com/gopherjs/gopherjs; git checkout origin/embed-core-pkgs)
mkdir -p $NEWGOPATH/src/github.com/hajimehoshi/gopherjstest
echo 'package main                                                                                                                                                                                          
                                                                                                                                                                                                            
import "github.com/gopherjs/gopherjs/js"                                                                                                                                                                    
                                                                                                                                                                                                            
func main() {                                                                                                                                                                                               
    js.Global.Get("console").Call("log", "Hello")                                                                                                                                                           
}' > $NEWGOPATH/src/github.com/hajimehoshi/gopherjstest/main.go
mkdir -p $NEWGOPATH/src/github.com/hajimehoshi/gopherjstest/vendor/github.com/gopherjs

# vendering                                                                                                                                                                                                 
cp -r $NEWGOPATH/src/github.com/gopherjs/gopherjs $NEWGOPATH/src/github.com/hajimehoshi/gopherjstest/vendor/github.com/gopherjs
rm -rf $NEWGOPATH/src/github.com/gopherjs/gopherjs

GOPATH=$NEWGOPATH go build github.com/hajimehoshi/gopherjstest
GOPATH=$NEWGOPATH $NEWGOPATH/bin/gopherjs run $NEWGOPATH/src/github.com/hajimehoshi/gopherjstest/main.go

failed with the message

cannot find package "github.com/gopherjs/gopherjs/js" in any of:
        /usr/local/go/src/github.com/gopherjs/gopherjs/js (from $GOROOT)
        /tmp/gopherjs/src/github.com/gopherjs/gopherjs/js (from $GOPATH)

Am I missing something? (Without vendering, it succeeded to say 'Hello'.)

EDIT: Ah, gopherjs command itself relies on GOPATH/src/github.com/gohperjs/gopherjs, not vendered one.

EDIT2: Hmm, reinstalling gopherjs command doesn't change the situation 🤔

NEWGOPATH=/tmp/gopherjs

rm -rf $NEWGOPATH
GOPATH=$NEWGOPATH go get github.com/gopherjs/gopherjs
(cd $NEWGOPATH/src/github.com/gopherjs/gopherjs; git checkout origin/embed-core-pkgs)

# Create the main package                                                                                                                                                                                   
mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest
echo 'package main                                                                                                                                                                                          
                                                                                                                                                                                                            
import "github.com/gopherjs/gopherjs/js"                                                                                                                                                                    
                                                                                                                                                                                                            
func main() {                                                                                                                                                                                               
    js.Global.Get("console").Call("log", "Hello")                                                                                                                                                           
}' > $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go
mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs

# Vendering                                                                                                                                                                                                 
cp -r $NEWGOPATH/src/github.com $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
cp -r $NEWGOPATH/src/golang.org $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
rm -rf $NEWGOPATH/src/github.com/gopherjs/gopherjs
GOPATH=$NEWGOPATH go install foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs

GOPATH=$NEWGOPATH go build foobarbaz/hajimehoshi/gopherjstest
GOPATH=$NEWGOPATH $NEWGOPATH/bin/gopherjs run $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go

myitcv · 2018-04-08T07:38:53Z

The way I tested was:

Should we be getting tests like the ones you describe in a script (or similar) as part of the repo? Automates the process of verifying that the behaviour is as described, makes it easier for you to test that the vendored version works with any subsequent commits/changes submitted to this PR, but also makes it easier for anyone else making future (unrelated) changes to verify that their changes haven't broken the vendor behaviour.

dmitshur · 2018-04-08T17:18:11Z

@hajimehoshi I think you were forgetting to install the checked out PR branch in the first script. Instead of doing go get, use go get -d to avoid installing the master branch version:

-GOPATH=$NEWGOPATH go get github.com/gopherjs/gopherjs
+GOPATH=$NEWGOPATH go get -d github.com/gopherjs/gopherjs
 (cd $NEWGOPATH/src/github.com/gopherjs/gopherjs; git checkout origin/embed-core-pkgs)
+GOPATH=$NEWGOPATH go get github.com/shurcooL/httpfs/vfsutil
+GOPATH=$NEWGOPATH go install github.com/gopherjs/gopherjs

You can also do export GOPATH=$NEWGOPATH to avoid having to do GOPATH=$NEWGOPATH for every command.

The second one looks like it should've worked. Let me try it and see if I can reproduce.

Should we be getting tests like the ones you describe in a script (or similar) as part of the repo?

Yes, adding a test for this would be nice.

dmitshur · 2018-04-08T17:37:23Z

@hajimehoshi I followed your edited instructions and got a different outcome. Notably, gopherjs didn't build because you forgot to vendor one of its new dependencies. After I fetched that dependency, it built successfully, and the gopherjstest/main.go program was executed by GopherJS successfully.

I think you made a mistake somewhere, and ended up using the master branch of GopherJS instead of this PR. I suggest you try those instructions a second time.

See the log from my terminal for reference:

Last login: Sun Apr  8 13:11:18 on ttys000
~ $ cd /tmp/
tmp $ ls
com.apple.launchd.0q00aXw85q	powerlog
com.apple.launchd.v3Mhw42Gmb
tmp $ NEWGOPATH=/tmp/go2
tmp $ ls $NEWGOPATH
ls: /tmp/go2: No such file or directory
tmp $ rm -rf $NEWGOPATH
tmp $ GOPATH=$NEWGOPATH go get github.com/gopherjs/gopherjs
tmp $ tree $NEWGOPATH | head
/tmp/go2
├── bin
│   └── gopherjs
└── src
    ├── github.com
    │   ├── fsnotify
    │   │   └── fsnotify
    │   │       ├── AUTHORS
    │   │       ├── CHANGELOG.md
    │   │       ├── CONTRIBUTING.md
tmp $ (cd $NEWGOPATH/src/github.com/gopherjs/gopherjs; git checkout origin/embed-core-pkgs)
Note: checking out 'origin/embed-core-pkgs'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 9b293ec... README: Remove note saying vendoring GopherJS is unsupported.
tmp $ mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest
tmp $ echo 'package main                                                                                                                                                                                          
>                                                                                                                                                                                                             
> import "github.com/gopherjs/gopherjs/js"                                                                                                                                                                    
>                                                                                                                                                                                                             
> func main() {                                                                                                                                                                                               
>     js.Global.Get("console").Call("log", "Hello")                                                                                                                                                           
> }' > $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go
tmp $ tree /tmp/go2/src/foobarbaz/
/tmp/go2/src/foobarbaz/
└── hajimehoshi
    └── gopherjstest
        └── main.go

2 directories, 1 file
tmp $ mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs
tmp $ ls go2/src/
foobarbaz	github.com	golang.org
tmp $ cp -r $NEWGOPATH/src/github.com $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
tmp $ cp -r $NEWGOPATH/src/golang.org $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
tmp $ cd go2
go2 $ ls
bin	src
go2 $ ls src/
foobarbaz/  github.com/ golang.org/ 
go2 $ ls src/foobarbaz/hajimehoshi/gopherjstest/vendor/
github.com	golang.org
go2 $ rm -rf $NEWGOPATH/src/github.com/gopherjs/gopherjs
go2 $ ls -la bin/gopherjs 
-rwxr-xr-x  1 Dmitri  staff  12955388 Apr  8 13:24 bin/gopherjs
go2 $ GOPATH=$NEWGOPATH go install -v foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs
src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/build/build.go:27:2: cannot find package "github.com/shurcooL/httpfs/vfsutil" in any of:
	/tmp/go2/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/shurcooL/httpfs/vfsutil (vendor tree)
	/usr/local/go/src/github.com/shurcooL/httpfs/vfsutil (from $GOROOT)
	/tmp/go2/src/github.com/shurcooL/httpfs/vfsutil (from $GOPATH)
go2 $ (mkdir /tmp/go2/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/shurcooL; cd /tmp/go2/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/shurcooL; git clone https://github.com/shurcooL/httpfs)
Cloning into 'httpfs'...
remote: Counting objects: 197, done.
remote: Total 197 (delta 0), reused 0 (delta 0), pack-reused 197
Receiving objects: 100% (197/197), 32.35 KiB | 1.08 MiB/s, done.
Resolving deltas: 100% (82/82), done.
go2 $ GOPATH=$NEWGOPATH go install -v foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/prelude
foobarbaz/hajimehoshi/gopherjstest/vendor/golang.org/x/sys/unix
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/neelance/sourcemap
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/kisielk/gotool/internal/load
foobarbaz/hajimehoshi/gopherjstest/vendor/golang.org/x/tools/go/buildutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/astutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/typesutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/spf13/pflag
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/vendor/github.com/neelance/astrewrite
foobarbaz/hajimehoshi/gopherjstest/vendor/golang.org/x/tools/go/gcimporter15
foobarbaz/hajimehoshi/gopherjstest/vendor/golang.org/x/tools/go/types/typeutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/filter
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/analysis
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/gopherjspkg
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler/natives
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/shurcooL/httpfs/vfsutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/kisielk/gotool
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/compiler
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/internal/sysutil
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/fsnotify/fsnotify
foobarbaz/hajimehoshi/gopherjstest/vendor/golang.org/x/crypto/ssh/terminal
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/spf13/cobra
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs/build
foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs/gopherjs
go2 $ GOPATH=$NEWGOPATH go build foobarbaz/hajimehoshi/gopherjstest
go2 $ GOPATH=$NEWGOPATH $NEWGOPATH/bin/gopherjs run $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go
Hello
go2 $

I'm going to bump the GopherJS version in this PR, so it'll be easier to tell it apart from previous versions (that did not support vendoring).

hajimehoshi · 2018-04-09T01:59:18Z

@shurcooL OK, I've succeeded to say 'hello' with your fix, thank you!

NEWGOPATH=/tmp/gopherjs

rm -rf $NEWGOPATH
GOPATH=$NEWGOPATH go get -d github.com/gopherjs/gopherjs
(cd $NEWGOPATH/src/github.com/gopherjs/gopherjs; git checkout origin/embed-core-pkgs)
GOPATH=$NEWGOPATH go get github.com/shurcooL/httpfs/vfsutil
GOPATH=$NEWGOPATH go install github.com/gopherjs/gopherjs

# Create the main package                                                                                                                                                                                   
mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest
echo 'package main                                                                                                                                                                                          
                                                                                                                                                                                                            
import "github.com/gopherjs/gopherjs/js"                                                                                                                                                                    
                                                                                                                                                                                                            
func main() {                                                                                                                                                                                               
    js.Global.Get("console").Call("log", "Hello")                                                                                                                                                           
}' > $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go
mkdir -p $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor/github.com/gopherjs

# Vendering                                                                                                                                                                                                 
cp -r $NEWGOPATH/src/github.com $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
cp -r $NEWGOPATH/src/golang.org $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/vendor
rm -rf $NEWGOPATH/src/github.com/gopherjs/gopherjs                                                                                                      

GOPATH=$NEWGOPATH go build foobarbaz/hajimehoshi/gopherjstest
GOPATH=$NEWGOPATH $NEWGOPATH/bin/gopherjs run $NEWGOPATH/src/foobarbaz/hajimehoshi/gopherjstest/main.go

hajimehoshi · 2018-04-09T02:21:45Z

build/build.go

+		IsDir: func(path string) bool {
+			if strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator)) {
+				path = filepath.ToSlash(path[len(gopherJSRoot):])
+				if fi, err := vfsutil.Stat(gopherjspkg.FS, path); err == nil {


This condition is repeated four times, so how about creating a function?

Which condition, exactly? I don't see a way of making this code simpler, can you show what you had in mind?

Note that these funcs return different signatures (bool, ([]os.FileInfo, error), (io.ReadCloser, error), (os.FileInfo, error)).

Sorry but I meant strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator) or path = filepath.ToSlash(path[len(gopherJSRoot):]). Aren't they repeated?

I see, thanks for clarifying.

I've considered it, but I think keeping it as is is more readable and simpler. The duplication is locally contained in the same place. Factoring it out into a helper moves the relevant code further away from where it's being used, and there's just too little code here to factor out efficiently.

hajimehoshi · 2018-04-09T02:22:54Z

build/build.go

@@ -91,17 +145,17 @@ func importWithSrcDir(path string, srcDir string, mode build.ImportMode, install
 	case "crypto/x509", "os/user":
 		// These stdlib packages have cgo and non-cgo versions (via build tags); we want the latter.
 		bctx.CgoEnabled = false
+	case "github.com/gopherjs/gopherjs/js", "github.com/gopherjs/gopherjs/nosync":


Isn't it ok to check in the above way like strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator) for consistency?

No, because this needs to be those 2 packages only. We don't want github.com/gopherjs/gopherjs/compiler or others to be included.

OK, thanks. I feel like this set is duplicated with FS definition in fs.go. Would it be possible to unify these sets into one?

The "core" GopherJS packages are enumerated and documented in the package comment of ./compiler/gopherjspkg package (see compiler/gopherjspkg/doc.go file).

Here, the ./build package implements that. It uses import paths.

In gopherjspkg.FS, the code picks up those packages via relative directory paths.

I don't think anything more needs to be done, this is very simple, and trying to make it more DRY isn't worth the effort (I'm usually all for it, but it needs to be worthwhile).

hajimehoshi · 2018-04-09T02:31:10Z

compiler/gopherjspkg/fs.go

+	func(path string, fi os.FileInfo) bool {
+		return path == "/" ||
+			path == "/js" || strings.HasPrefix(path, "/js/") ||
+			path == "/nosync" || strings.HasPrefix(path, "/nosync/")


Not only github.com/gopherjs/js, but also github.com/gopherjs/js/foobar would be a special path?

If we add a new package under js or nosync, yes, it would get included here. The chances of that are very low, so we can deal with it when/if it happens. We might want to include it as "core" or exclude it (by making the filter above more picky).

I made the filter more precise in 2ad4a5b, so it won't pick up any subdirectories now.

hajimehoshi · 2018-04-09T02:34:25Z

And as @myitcv suggests, we should have a test for this.

mpl · 2018-04-09T17:28:09Z

@shurcooL I haven't looked at the code at all, but with this PR it seems I was able to build Perkeep without our temp GOPATH hackery, so LGTM as far as I'm concerned. thanks!

dmitshur

we should have a test for this.

I'll try to make one. It needs to be simple and be able to run offline, so go get isn't allowed. It should be doable by copying the gopherjs repo itself into a temporary folder and making that the GOPATH.

I was able to build Perkeep without our temp GOPATH hackery

Good to hear, thanks for trying it!

dmitshur · 2018-04-10T04:19:36Z

compiler/gopherjspkg/fs.go

+	func(path string, fi os.FileInfo) bool {
+		return path == "/" ||
+			path == "/js" || strings.HasPrefix(path, "/js/") ||
+			path == "/nosync" || strings.HasPrefix(path, "/nosync/")


If we add a new package under js or nosync, yes, it would get included here. The chances of that are very low, so we can deal with it when/if it happens. We might want to include it as "core" or exclude it (by making the filter above more picky).

dmitshur · 2018-04-10T04:20:23Z

build/build.go

@@ -91,17 +145,17 @@ func importWithSrcDir(path string, srcDir string, mode build.ImportMode, install
 	case "crypto/x509", "os/user":
 		// These stdlib packages have cgo and non-cgo versions (via build tags); we want the latter.
 		bctx.CgoEnabled = false
+	case "github.com/gopherjs/gopherjs/js", "github.com/gopherjs/gopherjs/nosync":


No, because this needs to be those 2 packages only. We don't want github.com/gopherjs/gopherjs/compiler or others to be included.

dmitshur · 2018-04-10T04:21:52Z

build/build.go

+		IsDir: func(path string) bool {
+			if strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator)) {
+				path = filepath.ToSlash(path[len(gopherJSRoot):])
+				if fi, err := vfsutil.Stat(gopherjspkg.FS, path); err == nil {


Which condition, exactly? I don't see a way of making this code simpler, can you show what you had in mind?

Note that these funcs return different signatures (bool, ([]os.FileInfo, error), (io.ReadCloser, error), (os.FileInfo, error)).

hajimehoshi

Thanks, I'll wait for you adding tests :-)

hajimehoshi · 2018-04-10T13:51:51Z

build/build.go

+		IsDir: func(path string) bool {
+			if strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator)) {
+				path = filepath.ToSlash(path[len(gopherJSRoot):])
+				if fi, err := vfsutil.Stat(gopherjspkg.FS, path); err == nil {


Sorry but I meant strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator) or path = filepath.ToSlash(path[len(gopherJSRoot):]). Aren't they repeated?

hajimehoshi · 2018-04-10T13:54:51Z

build/build.go

@@ -91,17 +145,17 @@ func importWithSrcDir(path string, srcDir string, mode build.ImportMode, install
 	case "crypto/x509", "os/user":
 		// These stdlib packages have cgo and non-cgo versions (via build tags); we want the latter.
 		bctx.CgoEnabled = false
+	case "github.com/gopherjs/gopherjs/js", "github.com/gopherjs/gopherjs/nosync":


OK, thanks. I feel like this set is duplicated with FS definition in fs.go. Would it be possible to unify these sets into one?

lologarithm

Haven't read all the code but this branch is working for me when vendoring gopherjs/gopherjs.

The only change I had to make was to include a dummy go file with import _ "github.com/gopherjs/gopherjs" so that the dep tool will pull in everything instead of just the libraries directory.

dmitshur · 2018-04-16T00:20:23Z

I've added a test that GopherJS can be vendored in commit 49f2b5c. It's passing CI.

I've cherry-picked the same commit onto latest master version on master-with-vendor-test branch, and as expected, the same test is failing there (since current master version doesn't support being vendored):

--- FAIL: TestGopherJSCanBeVendored (0.35s)
	gorepo_test.go:38: exit status 1
FAIL
FAIL	github.com/gopherjs/gopherjs/tests	176.450s

That means the test is working as expected. PTAL.

I'll deal with the remaining minor comments next.

dmitshur · 2018-04-16T00:40:05Z

Improved failing test output in commit 8a9d21b:

$ go test -v -run=TestGopherJSCanBeVendored
=== RUN   TestGopherJSCanBeVendored
vendoring github.com/gopherjs/gopherjs/js package is not supported, see https://github.com/gopherjs/gopherjs/issues/415
--- FAIL: TestGopherJSCanBeVendored (5.19s)
	gorepo_test.go:40: exit status 1
FAIL
exit status 1
FAIL	github.com/gopherjs/gopherjs/tests	5.190s

dmitshur

Thanks for review comments and suggestions @hajimehoshi. I've considered and addressed them all. This is ready from my side now. PTAL.

dmitshur · 2018-04-16T01:54:05Z

compiler/gopherjspkg/fs.go

+	func(path string, fi os.FileInfo) bool {
+		return path == "/" ||
+			path == "/js" || strings.HasPrefix(path, "/js/") ||
+			path == "/nosync" || strings.HasPrefix(path, "/nosync/")


I made the filter more precise in 2ad4a5b, so it won't pick up any subdirectories now.

dmitshur · 2018-04-16T01:56:59Z

build/build.go

@@ -91,17 +145,17 @@ func importWithSrcDir(path string, srcDir string, mode build.ImportMode, install
 	case "crypto/x509", "os/user":
 		// These stdlib packages have cgo and non-cgo versions (via build tags); we want the latter.
 		bctx.CgoEnabled = false
+	case "github.com/gopherjs/gopherjs/js", "github.com/gopherjs/gopherjs/nosync":


The "core" GopherJS packages are enumerated and documented in the package comment of ./compiler/gopherjspkg package (see compiler/gopherjspkg/doc.go file).

Here, the ./build package implements that. It uses import paths.

In gopherjspkg.FS, the code picks up those packages via relative directory paths.

I don't think anything more needs to be done, this is very simple, and trying to make it more DRY isn't worth the effort (I'm usually all for it, but it needs to be worthwhile).

dmitshur · 2018-04-16T01:58:58Z

build/build.go

+		IsDir: func(path string) bool {
+			if strings.HasPrefix(path, gopherJSRoot+string(filepath.Separator)) {
+				path = filepath.ToSlash(path[len(gopherJSRoot):])
+				if fi, err := vfsutil.Stat(gopherjspkg.FS, path); err == nil {


I see, thanks for clarifying.

I've considered it, but I think keeping it as is is more readable and simpler. The duplication is locally contained in the same place. Factoring it out into a helper moves the relevant code further away from where it's being used, and there's just too little code here to factor out efficiently.

dmitshur · 2018-04-16T05:11:32Z

I filed issue #792 for some potential followup work.

hajimehoshi

Almost lgtm

hajimehoshi · 2018-04-16T05:25:29Z

tests/gorepo_test.go

+		t.Fatal(err)
+	}
+	if want := "hello using js pkg\n"; string(got) != want {
+		t.Errorf("got != want:\ngot:\n%s\nwant:\n%s", got, want)


I'd write the message mentioning the script like gopherjsvendored_test.js failed

The failure output is:

$ go test -v -run=TestGopherJSCanBeVendored === RUN TestGopherJSCanBeVendored --- FAIL: TestGopherJSCanBeVendored (17.78s) gorepo_test.go:43: got != want: got: unexpected output 123 want: hello using js pkg FAIL exit status 1 FAIL github.com/gopherjs/gopherjs/tests 17.787s

There's only one got != want check in the test. It seems okay as is, isn't it?

Hm, as I am not sure GopherJS's test policy, I don't have a strong opinion on it. Then, that's fine :-)

I can make it something like this, hope it's more clear:

unexpected stdout from gopherjsvendored_test.sh: got: unexpected output 123 want: hello using js pkg

hajimehoshi · 2018-04-16T05:26:04Z

tests/gorepo_test.go

+	}
+
+	cmd := exec.Command("bash", "gopherjsvendored_test.sh")
+	cmd.Stderr = os.Stdout


Why not catching the stderr and dumping it as an error message?

The test is considered failed if the exit code is non-zero, or if the stdout output is not exactly the expected "hello using js pkg\n" value.

Stderr can be anything, it shouldn't fail the test. It's just information.

For example, if you run the test without the sourcemap module, it'll print "gopherjs: Source maps disabled. Install source-map-support module for ..." message to stderr.

Makes sense.

hajimehoshi · 2018-04-16T05:28:55Z

tests/gopherjsvendored_test.sh

+cp -r $(go list -e -f '{{.Dir}}' github.com/spf13/pflag)        "$tmp/src/example.org/hello/vendor/github.com/spf13/pflag"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/crypto)           "$tmp/src/example.org/hello/vendor/golang.org/x/crypto"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/sys)              "$tmp/src/example.org/hello/vendor/golang.org/x/sys"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/tools)            "$tmp/src/example.org/hello/vendor/golang.org/x/tools"


If GopherJS's dependencies are changed, we should update these items, right? At least, should we leave a comment?

Yeah. Perhaps this script can be improved in the future to infer dependencies more automatically (e.g., with go list -f '{{join .Deps "\n"}}' github.com/gopherjs/gopherjs/... or so).

There's a comment on top saying # Vendor GopherJS and its dependencies into hello project., is it not clear?

Feels like we should make this change now; it's brittle having these hard-coded, particularly when using go list is something that we're already doing elsewhere and is easy to make work in this situation.

Agreed with @myitcv . This doesn't look so hard.

myitcv

I'll take a look through this more closely later. I do have a question about your comments in #792 though - which I will ask there.

myitcv · 2018-04-16T08:34:37Z

tests/gopherjsvendored_test.sh

+cp -r $(go list -e -f '{{.Dir}}' github.com/spf13/pflag)        "$tmp/src/example.org/hello/vendor/github.com/spf13/pflag"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/crypto)           "$tmp/src/example.org/hello/vendor/golang.org/x/crypto"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/sys)              "$tmp/src/example.org/hello/vendor/golang.org/x/sys"
+cp -r $(go list -e -f '{{.Dir}}' golang.org/x/tools)            "$tmp/src/example.org/hello/vendor/golang.org/x/tools"


Feels like we should make this change now; it's brittle having these hard-coded, particularly when using go list is something that we're already doing elsewhere and is easy to make work in this situation.

myitcv

Added a few more comments; please also note I added a response over in #792 (comment)

myitcv · 2018-04-16T09:15:36Z

compiler/gopherjspkg/doc.go

+//
+package gopherjspkg
+
+//go:generate vfsgendev -source="github.com/gopherjs/gopherjs/compiler/gopherjspkg".FS -tag=gopherjsdev


Please can we make this go:generate directive not rely on vfsgendev being in PATH?

myitcv · 2018-04-16T09:16:41Z

compiler/version_check.go

@@ -6,4 +6,4 @@ package compiler
 const ___GOPHERJS_REQUIRES_GO_VERSION_1_10___ = true

 // Version is the GopherJS compiler version string.
-const Version = "1.10-2"
+const Version = "1.10-3"


Should we not do a release separately from this PR?

I don't see a difference, but sure, I can take out this commit and apply it on master after this PR lands.

The difference being that if this PR needs to be reverted at a later date we don't want to revert the version change. Keeping the release as a separate PR ensures we have that ability.

myitcv · 2018-04-16T09:18:41Z

tests/gorepo_test.go

+		t.Skip("test meant to be run using normal Go compiler (needs os/exec)")
+	}
+
+	cmd := exec.Command("bash", "gopherjsvendored_test.sh")


sh should be sufficient here; and more flexible to those who do not use bash

Did you see the commit message of 49f2b5c?

However, I think I know what it'll take to make it work with sh. It might just be a matter of removing the SIG prefix from the signal names. I'll try it.

Is it not cleaner to just write this test as // +build ignore-d .go file in any case? That way we don't need to worry about shells, traps etc.

I often prefer Go over other tools, but a bash script seemed like a good fit for this task. It was based on the bash script in playground repo, which has been used with success for some years.

It's already done and it works well. I don't see a need to spend much more time on it.

Will leave it up to you. It might be, however, that addressing https://github.com/gopherjs/gopherjs/pull/787/files/2ad4a5b9a30e5f1f31c643c2a2ac27543a5ddb58#r181624969 would be easier/clearer in a .go file.

Call NewBuildContext at the top of Import, ImportDir, and inside NewSession. Previously, it was called more often than necessary (instead of being preserved for entire build session). Pass build context by value to importWithSrcDir so it can be safely modified there, without the changes persisisting from one package to the next. The build context will need to be available in more places in an upcoming commit. Storing it in Session helps with that.

This package embeds the core GopherJS packages (js, nosync). It's similar to how native overrides for stdlib are embedded in the compiler/natives package. Pick only files in /fs and /nosync, and no subdirectories (if any are added, they won't get included by default).

The Go stdlib overrides (i.e., compiler/natives package) are already embedded into the GopherJS build system. This makes it possible for the gopherjs binary to access them even if the gopherjs repository is not present in GOPATH (e.g., because it was deleted, or because it's vendored). Doing that for natives but not the js, nosync packages makes little sense, since the gopherjs repository still needs to exist in GOPATH. This change fixes that. By embedding the core GopherJS packages analogously to natives, the gopherjs binary becomes more standalone. Now, it only requires GOROOT/src to contain the matching version of Go source code. Non-core GopherJS packages are not embedded (e.g., the GopherJS compiler itself). That means it needs to exist in GOPATH to be buildable (otherwise, the user gets a "cannot find package" error). This is expected just like with any other normal Go package. Fixes #462. Helps #415.

…nosync. These core GopherJS packages are already embedded into the GopherJS build system via the gopherjspkg virtual filesystem. The gopherjspkg package can be safely vendored. Therefore, there's no need to try to use vendor directory to resolve those packages. This change makes it possible to use a vendored copy of GopherJS in a project, and use it to build any Go code. Fixes #415.

The _test.go file needs to be accessed via the build context's file system interface. Take into account the fact that the js and nosync packages are now effectively virtual, and don't have a corresponding physical directory. Use current directory when running tests with Node.js, otherwise it would fail with "directory not exist" error.

Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run with GopherJS. It's more clear and reliable than a file-wide build tag. Include stderr from gopherjsvendored_test.sh in output of TestGopherJSCanBeVendored. This is only to get get more information when it fails.

dmitshur · 2018-04-21T01:04:31Z

I've rebased this PR on top of latest master and addressed all outstanding comments. PTAL.

(Previous PR version was 2ad4a5b.)

hajimehoshi

lgtm

@myitcv, are you happy with this PR?

myitcv

Just a couple of minor comments/questions, but nothing blocking.

myitcv · 2018-04-21T12:05:41Z

tests/gopherjsvendored_test.sh

+
+# Make a hello project that will vendor GopherJS.
+mkdir -p "$tmp/src/example.org/hello"
+echo 'package main


Just a minor thought, but the heredoc syntax often reads a bit cleaner in these multi-line situations because the redirect is on the same line:

cat <<EOD > "$tmp/src/example.org/hello/main.go" package main import "github.com/gopherjs/gopherjs/js" func main() { js.Global.Get("console").Call("log", "hello using js pkg") } EOD

Thanks for the suggestion. I wasn't familiar with this syntax, but I'll consider it next time.

myitcv · 2018-04-21T12:06:19Z

tests/gorepo_test.go

 	"testing"
 )

 // Go repository basic compiler tests, and regression tests for fixed compiler bugs.
 func TestGoRepositoryCompilerTests(t *testing.T) {
+	if runtime.GOARCH == "js" {


What's the thinking behind dropping the // +build !js build tag here?

The motivation is written in the commit message of d0d69c0:

Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run
with GopherJS. It's more clear and reliable than a file-wide build tag.

t.Skip exists per-test (rather than for the entire file) and can offer an individual message for the skip reason. This was better in this context. Build tags were used previously because I didn't get the idea that t.Skip could be used instead until now.

myitcv · 2018-04-21T12:07:10Z

tests/lowlevel_test.go

@@ -15,6 +14,10 @@ import (
 //
 // See https://github.com/gopherjs/gopherjs/issues/279.
 func TestTimeInternalizationExternalization(t *testing.T) {
+	if runtime.GOARCH == "js" {


Same question on why the build tag isn't sufficient in this situation and why you prefer the t.Skip

dmitshur · 2018-04-21T13:55:27Z

Thanks for the reviews!

I'm going to merge this and bump up the GopherJS compiler version to 1.10-3, the first version to support vendoring GopherJS into other Go projects.

myitcv · 2018-04-21T13:57:02Z

@shurcooL - would it make sense to also review and merge #669 prior to the new version?

dmitshur · 2018-04-21T13:59:08Z

There isn't a need, that PR is orthogonal. We can bump up to 1.10-4 for it if needed.

myitcv · 2018-04-21T14:02:12Z

There isn't a need, that PR is orthogonal. We can bump up to 1.10-4 for it if needed.

Yes I know it's orthogonal, but I was proposing bundling together a number of fixes into one release. Or would you prefer to have one point release per fix/feature?

dmitshur · 2018-04-21T14:06:21Z

Let's discuss that outside this PR. I've already effectively allocated 1.10-3 for this fix by bumping to 1.10-3 in the earlier version of this PR, so that release has already been decided.

We can reconsider the approach for future versions.

…amd64). The github.com/gopherjs/gopherjs/js and github.com/gopherjs/gopherjs/nosync packages still need to be built and served as separate archives, because the playground uses a custom compiler.ImportContext for loading all packages from the server. The playground doesn't try to build them. Regenerate with: go generate github.com/gopherjs/gopherjs.github.io/playground Follows gopherjs/gopherjs#787.

dmitshur · 2018-04-21T20:39:45Z

As a followup step, I've checked that the GopherJS Playground works with the latest version (it does) and updated it in gopherjs/gopherjs.github.io@cbdec89.

This was referenced Apr 8, 2018

"undeclared name: efaceOf" build error when using different GOPATHs #462

Closed

Vendoring gopherjs code causes generated js to fail #415

Closed

Add support for vendoring gopherjs #678

Closed

dmitshur requested a review from hajimehoshi April 8, 2018 01:42

dmitshur mentioned this pull request Apr 8, 2018

Depend on Go 1.10, stop using tmp/ directory source code mirror for make.go? perkeep/perkeep#1004

Closed

hajimehoshi reviewed Apr 8, 2018

View reviewed changes

hajimehoshi suggested changes Apr 9, 2018

View reviewed changes

dmitshur commented Apr 10, 2018

View reviewed changes

hajimehoshi suggested changes Apr 10, 2018

View reviewed changes

dmitshur force-pushed the embed-core-pkgs branch from c934767 to 50a9ce5 Compare April 10, 2018 15:39

lologarithm approved these changes Apr 10, 2018

View reviewed changes

dmitshur force-pushed the embed-core-pkgs branch from 163f6c0 to 49f2b5c Compare April 15, 2018 23:56

dmitshur commented Apr 16, 2018

View reviewed changes

dmitshur mentioned this pull request Apr 16, 2018

Move tests in js package to tests package. #792

Closed

hajimehoshi reviewed Apr 16, 2018

View reviewed changes

myitcv suggested changes Apr 16, 2018

View reviewed changes

myitcv mentioned this pull request Apr 16, 2018

Sequencing PRs to be merged #793

Open

dmitshur mentioned this pull request Apr 20, 2018

Move js package tests into tests package. #800

Merged

dmitshur added 7 commits April 20, 2018 17:05

README: Remove note saying vendoring GopherJS is unsupported.

c8e5f7c

dmitshur force-pushed the embed-core-pkgs branch from 2ad4a5b to c8e5f7c Compare April 21, 2018 01:04

hajimehoshi approved these changes Apr 21, 2018

View reviewed changes

myitcv approved these changes Apr 21, 2018

View reviewed changes

dmitshur merged commit c8e5f7c into master Apr 21, 2018

dmitshur deleted the embed-core-pkgs branch April 21, 2018 14:08

This was referenced Apr 22, 2018

vfsgendev updates modification times #804

Open

playground: go generate fails when GOROOT is user-writeable. gopherjs/gopherjs.github.io#69

Closed

Embed core GopherJS packages into build system; enable vendoring of GopherJS. #787

Embed core GopherJS packages into build system; enable vendoring of GopherJS. #787

Uh oh!

Conversation

dmitshur commented Apr 8, 2018

Uh oh!

hajimehoshi commented Apr 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myitcv commented Apr 8, 2018

Uh oh!

dmitshur commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hajimehoshi commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajimehoshi commented Apr 9, 2018

Uh oh!

mpl commented Apr 9, 2018

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajimehoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lologarithm left a comment

Choose a reason for hiding this comment

dmitshur Apr 20, 2018 •

edited

Loading

dmitshur commented Apr 8, 2018 •

edited

Loading

hajimehoshi commented Apr 8, 2018 •

edited

Loading

dmitshur commented Apr 8, 2018 •

edited

Loading

dmitshur commented Apr 8, 2018 •

edited

Loading

dmitshur commented Apr 16, 2018 •

edited

Loading