Treat GOPATH as a list of workspaces; use first workspace. #62
Conversation
|
I think it's a good idea to use |
|
Also note that, similarly to the code before my changes, I assume GOPATH contains at least one workspace. If that's not true, I will panic when trying to do |
Fixes #60. The behavior should be improved (fixes #60) if user has multiple GOPATH workspaces. The behavior should be unchanged if user has a single GOPATH workspace. First cut at correct GOPATH handling as a list, rather than assuming it contains no more than 1 entry. It's unfinished and needs more work to correctly handle all scenarios with multiple GOPATH workspaces, but it's an improvement over the original version. Use filepath.Join instead of concatenating strings for path manipulation.
|
Travis failing for Go 1.3 after I rebased, is that something I did wrong, or is one of the tests being flaky? Edit: Seems to be related to goroutines; could there be a race condition? |
|
I restarted your build and now it worked fine. Yes, there seems to be a race condition. |
|
Any thoughts on how to move this PR forward closer to merge? It fixes issue #60. |
|
I think your changes are good. It should be fine to always take the first GOPATH workspace as long as there is no other bug to solve and it doesn't break something that was working before. There is just a bit too much noise in this commit to merge it right now. Please don't change the order of the imports (I guess your editor does this) and remove the TODO comments. Amend your current commit, then I'll pull it. |
It's certainly an improvement over using completely incorrect paths, but there's still room for improvement. The logic for which GOPATH entry to use, as I understand it, is:
(http://golang.org/cmd/go/#hdr-Relative_import_paths is relevant.) This logic to make that happen resides somewhere in go/build.Context, and it would be best to make proper use of it rather than trying to re-do it on our own. This already happens to some degree in gopherjs, but not everywhere (hence this PR involves quite a few lines of code changes). However, that's difficult to do and test/verify, so I can't do that right now and will leave it for later or for someone else. But I just wanted to make note of this before I remove all the TODO comments.
Okay, I'll do that right now. Thanks. And yes, that would be goimports which uses a more modern (but not yet backported to |
Another case where it's not ideal is when searching for Go packages. If you only look in the first GOPATH workspace, but I import a Go package that happens to be in the second GOPATH workspace, gopherjs will not find it. Edit: Actually, you already do search multiple GOPATH workspaces via build.Context. The only remaining TODO here is when searching if a pkg has been built; that currently only checks the first GOPATH workspace while the pkg may be built inside another GOPATH workspace. Proper handling of multiple GOPATH workspaces is indeed not easy. |
Use gofmt instead of goimports to not change the order of the imports. Consolidate TODO comments into a short version.
|
I've added a commit to clean up this PR. Let me know if that looks good, and I can squash it into one commit if you wish. |
|
I have merged your two commits into one and pushed it to master, see 849fd25. You left some TODO comments. That's fine as long as they are not meant to stay forever. Will there be follow-up pull requests to address these issues? ;-) |
|
Thanks.
They will definitely not stay forever, the goal is to have them fixed. There are 3 TODOs. The first one is easy and I can do it, but the other 2 I'm not sure how to solve at this time. A part of the reason is because I don't really understand how |
First cut at correct GOPATH handling as a list, rather than assuming it contains no more than 1 entry. It's unfinished and needs more work to correctly handle all scenarios with multiple GOPATH workspaces, but it's an improvement over the original version.
filepath.Joininstead of concatenating strings for path manipulation.goimportsto gofmt the code, it separates 3rd party imports from standard library imports.While this PR can be merged as is (since it fixes a real issue), I think it's better to use it as a starting point of discussion about how completely proper GOPATH handling can be achieved. I've left a few TODOs in the code comments that need to be addressed.