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

Add support for vendoring gopherjs #678

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@nmiyake
Contributor

nmiyake commented Aug 17, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@nmiyake

nmiyake Aug 17, 2017

Contributor

See #415 (comment) for context.

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.

Contributor

nmiyake commented Aug 17, 2017

See #415 (comment) for context.

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.

Add support for vendoring gopherjs
* 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
@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?

}
}
}
}

This comment has been minimized.

@dmitshur

dmitshur Aug 19, 2017

Member

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.

@dmitshur

dmitshur Aug 19, 2017

Member

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.

This comment has been minimized.

@nmiyake

nmiyake Aug 19, 2017

Contributor

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)

@nmiyake

nmiyake Aug 19, 2017

Contributor

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)

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

This comment has been minimized.

@dmitshur

dmitshur Aug 19, 2017

Member

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?

@dmitshur

dmitshur Aug 19, 2017

Member

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?

This comment has been minimized.

@nmiyake

nmiyake Aug 19, 2017

Contributor

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

nmiyake Aug 19, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nmiyake

nmiyake Aug 19, 2017

Contributor

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.

Contributor

nmiyake commented Aug 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tbruyelle

tbruyelle Nov 10, 2017

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

tbruyelle commented Nov 10, 2017

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 10, 2017

Member

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.

Member

dmitshur commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Apr 8, 2018

Member

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!

Member

dmitshur commented Apr 8, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment