Enabled usage of go modules #144
Conversation
|
Does this work for anyone? I tried |
|
I have issued a PR to @kolaente's @nickoneill @kolaente's branch now works, though be sure to read my notes on the PR (in particular regarding the docker image). If you aren't comfortable with docker, let me know and I'll publish an image that you can use to Docker Hub. |
Completes the addition of go module support, including vendored builds
|
I've merged the pr, please re-review this one. @gordonklaus |
|
Or perhaps even @karalabe? |
|
@kolaente probably worth removing the |
kolaente
changed the title
| args = append(args, []string{"-v", fmt.Sprintf("%s:%s:ro", locals[i], mounts[i])}...) | ||
| if usesModules { | ||
| args = append(args, []string{"-e", "GO111MODULE=on"}...) | ||
| args = append(args, []string{"-v", os.Getenv("GOPATH") + ":/go"}...) |
There was a problem hiding this comment.
If GOPATH is empty, then we should use the default GOPATH ($HOME/go). It looks like go/build.Default.GOPATH will do the trick.
Also, should probably join using filepath.ListSeparator for portability.
There was a problem hiding this comment.
The base Dockerfile currently maps GOPATH to /go, so all we're doing here is ensuring that we mount the users GOPATH in order to avoid go build hitting the network by getting all the packages (unless it needs to).
Do need to ensure GOPATH is a valid path first, though. That happens in the previous two cases, but not in this new one. I just missed that.
The "/go" is actually a docker-specific thing, and uses the same cli syntax on Windows.
|
@karalabe Would you mind reviewing this? |
|
@zgramana I'd love to test this. I'm not familiar with how to generate docker images, but I'd give it a shot with some instructions. |
|
@nickoneill 😎 If you change into the |
| if strings.HasPrefix(config.Repository, string(filepath.Separator)) || strings.HasPrefix(config.Repository, ".") { | ||
| // Resolve the repository import path from the file path | ||
| config.Repository = resolveImportPath(config.Repository) | ||
|
|
||
| // Determine if this is a module-based repository | ||
| var modFile = config.Repository + "/go.mod" |
There was a problem hiding this comment.
I recently discovered that go env provides GOMOD which makes this easier.
- When a repository contains a modules-based project, and
GO111MODULESis eitheronorauto,go env GOMODwill contain the path to thego.modfile. - When
GO111MODULESis eitherofforauto, theGOMODvar will be the empty string. - Interestingly, if
GO111MODULESis set toonbut the current repository is not a module-based project, then it will (at least sometimes) contain a path to ago.modfile which does not actually exist.
There was a problem hiding this comment.
* Interestingly, if `GO111MODULES` is set to `on` but the current repository is not a module-based project, then it will (at least sometimes) contain a path to a `go.mod` file which does not actually exist.
If thats the case I wouldn't want to rely on that env to find the mod file... Maybe this changes with Go 1.12 or 1.13, but as of now...
There was a problem hiding this comment.
This same debate is being had by the golang team itself. It appears that go env GOMOD is intended to be The One Right Answer, but as the discussion on this issue illustrates, even the go build tool diverges. That same thread also confirms that the algorithm for determining module status is volatile and likely to change in the future.
Perhaps even more interesting is this comment:
I put GOMOD in 'go env' mainly so that it would be listed in bug reports. The expected way to get at this is
go list -m -f {{.GoMod}}but I have not added that yet. :-)
The interesting thing is that, as of go 1.11.4 (at least), it actually does work (at least for my quick check on the command-line). So, worth looking at again in the future, especially given that GOPATH is expected to be deprecated at some point in the semi-distant future.
I'd like to see this PR get merged in the meanwhile--given how quickly people are adopting modules--and open issues + PR's to refactor and enhance later on.
There was a problem hiding this comment.
I'd like to see this PR get merged in the meanwhile--given how quickly people are adopting modules--and open issues + PR's to refactor and enhance later on.
Yes, I'm with you on that.
Also, go modules will become the new standard in 1.13, which will first deprecate and later drop GOPATH entirely, so we should get this merged.
At Gitea, we also need this, and because this repo looks not maintained anymore and other things we may start a fork of xgo (and maintain it).
|
Any updates? |
|
@qianlifeng Doesn't look like it @karalabe Any chances this will be merged? |
|
For all people who are waiting for it, have a look at the fork https://github.com/techknowlogick/xgo. It is currently being maintained and the problem has already been solved there. |
|
Looks like xgo is more or less dead and we got to use the fork? |
Resolves #138
This pr adds the automatic lookup for a
go.modfile and if found, setsGO111MODULE=onon the build container to enable go modules instead of gopath.I also want to pass
-mod=vendorto the build script if avendorfolder is found and modules are enabled. How do I do that? I couldn't find a good way using existing flags and environment variables, should I introduce a new env variable for the build script for that?