Add support for vendoring gopherjs #678

nmiyake · 2017-08-17T17:22:13Z

Determines whether there is an import path to a vendored version
of gopherjs
If so, updates all import paths to gopherjs in generated code to
use the vendored version
Updates runtime/runtime.go to use a string constant for its static
reference to the gopherjs import path
Adds logic to update the string constant to be the vendored path
during generation

Fixes #415

nmiyake · 2017-08-17T17:26:21Z

Initial PR for feedback -- although it should work/be functional, the way in which the vendored path is determined and stored is pretty hacky and needs to be changed.

* Determines whether there is an import path to a vendored version of gopherjs * If so, updates all import paths to gopherjs in generated code to use the vendored version * Updates runtime/runtime.go to use a string constant for its static reference to the gopherjs import path * Adds logic to update the string constant to be the vendored path during generation Fixes gopherjs#415

dmitshur

Thanks a lot investigating that it'd take to make vendoring GopherJS work!

If this is all that it takes, then that's great news. I think this is quite reasonable. I want to think a bit more about how we can clean it up and potentially simplify it, but this is already very manageable and solves a valuable issue.

@neelance Could you take a look and see if you get any ideas for how we could simplify the solution, now that we have a baseline of a solution?

@nmiyake How did you test this change? Do you have suggestions for how we can test it?

dmitshur · 2017-08-19T22:32:14Z

build/build.go

+						}
+					}
+				}
+			}


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.

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)

dmitshur · 2017-08-19T22:37:05Z

build/build.go

 	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")


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?

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)

nmiyake · 2017-08-19T23:52:54Z

I currently have a project that uses gopherjs to generate Javascript output. I tested this change by vendoring all of gopherjs (as well as its dependencies since gopherjs doesn't appear to use a top-level vendor directory) and using go run to run the main in tool.go to perform the generation for my project. I verified that all of the import paths in the generated Javascript were to the vendored path, and also loaded the resulting Javascript to confirm that it worked.

From a unit testing perspective this should be pretty straightforward to test (especially if we make the recommended modification and add the target vendor rewrite path as a parameter) -- we can narrowly test that all the import paths are re-written as expected and trust that things work.

The one complication here for integration testing comes from the fact that gopherjs doesn't vendor its own dependencies (this makes setting up an environment a bit harder).

If we assume for a second that gopherjs did vendor its dependencies, I would write an integration test like the following:

Create a new temporary directory
Copy the contents of the project directory (gopherjs and everything in it) to tmpDir/src/github.com/gopherjs/gopherjs
Write out a simple test file/project to tmpDir/src/github.com/test/gopherjstest.go
Set GOPATH to be the temporary directory
Run go run tmpDir/src/github.com/gopherjs/gopherjs/tool.go build on the test file
Assert that run succeeded and that output files have certain characteristics (import paths etc.)

If vendoring isn't an approach this project is going to take, then the easiest way to integration test is probably Docker (this is what I did for a lot of my iteration). Would look like:

Use a Docker image based on a Go base image
Run go get -u github.com/goherjs/gopherjs (mainly a convenience for getting all of the dependencies and putting those into the GOPATH
Run rm -rf $GOPATH/src/github.com/gopherjs/gopherjs $GOPATH/pkg to clear out existing code
Create a test project in $GOPATH/src/github.com/test
Copy code from local gopherjs into vendor directory of the test project
Run go run vendor/.../gopherjs/tool.go build to build
Verify that output is generated and is correct

Either of the above approaches (or mimicking the above manually) should be sufficient for verification. As a sanity check I also did a search for the github.com/gopherjs import path in the codebase, and there didn't seem to be any other occurrences that I think would impact the functionality here.

tbruyelle · 2017-11-10T21:00:53Z

What's the status of this PR, is it freezed ?

dmitshur · 2017-11-10T23:47:28Z

The status of this PR is "open". To make progress, the PR needs to be reviewed and tested. The PR author has provided sufficient information to make that possible. There would either be changes requested, or it'd be approved and merged.

Unfortunately, I don't have an opportunity to review it soon.

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

Thank you for sending this PR, @nmiyake. It helped me better understand the problem and a potential solution (as well as ways of testing the end result).

After thinking more about this, I came up with a different approach to resolve issue #415. Pease see PR #787.

I think it's conceptually simpler (there are more code changes, but that's mostly because the build context had to be propagated into more places) and has the benefit of making the gopherjs binary more standalone. It can continue to function without anything in GOPATH; only needs the Go standard library code in GOROOT now.

I'll close this since it shouldn't be needed now (if we do, it can be reopened). Thank you again!

nmiyake mentioned this pull request Aug 17, 2017

Vendoring gopherjs code causes generated js to fail #415

Closed

nmiyake force-pushed the supportVendoringGopherjs branch from f02709f to 8a13825 Compare August 17, 2017 18:01

nmiyake force-pushed the supportVendoringGopherjs branch from 8a13825 to de4b94d Compare August 17, 2017 18:07

dmitshur reviewed Aug 19, 2017

View reviewed changes

marwan-at-work mentioned this pull request Sep 5, 2017

Update to reflect recent Vecty API change marwan-at-work/marwanio#1

Merged

dmitshur closed this Apr 8, 2018

dmitshur mentioned this pull request Apr 8, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for vendoring gopherjs #678

Add support for vendoring gopherjs #678

Uh oh!

nmiyake commented Aug 17, 2017

Uh oh!

nmiyake commented Aug 17, 2017

Uh oh!

dmitshur left a comment

Uh oh!

dmitshur Aug 19, 2017

Uh oh!

nmiyake Aug 19, 2017

Uh oh!

dmitshur Aug 19, 2017

Uh oh!

nmiyake Aug 19, 2017

Uh oh!

nmiyake commented Aug 19, 2017

Uh oh!

tbruyelle commented Nov 10, 2017

Uh oh!

dmitshur commented Nov 10, 2017 •

edited

Loading

Uh oh!

dmitshur commented Apr 8, 2018 •

edited

Loading

Uh oh!

Uh oh!

Add support for vendoring gopherjs #678

Add support for vendoring gopherjs #678

Uh oh!

Conversation

nmiyake commented Aug 17, 2017

Uh oh!

nmiyake commented Aug 17, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

nmiyake Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

nmiyake Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

nmiyake commented Aug 19, 2017

Uh oh!

tbruyelle commented Nov 10, 2017

Uh oh!

dmitshur commented Nov 10, 2017 • 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!

Uh oh!

dmitshur commented Nov 10, 2017 •

edited

Loading

dmitshur commented Apr 8, 2018 •

edited

Loading