-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
cmd/go: go build does not reject importing commands #4210
Comments
Here's a slightly smaller reproduction, in which nothing from x gets used at all yet building fails: $ echo $GOPATH /tmp/foo $ pwd /tmp/foo/src/y $ cat y.go package main import ( _ "x" ) func main() { } $ cat ../x/x.go package main func main() { } $ go build $ go install $ go build # y ./y.go:4: can't find import: "x" $ rm $GOPATH/bin/x $ go build $ For real world context: I hit this while trying to use goose (https://bitbucket.org/liamstask/goose) on Heroku. To run goose in the Heroku environment, it needs to get built remotely, and the easiest way to trigger this is as the side-effect of an unused import. Unfortunately, it also causes local builds to fail. |
For others in a similar situation to me: I found a workaround for the problem using build constraints. Heroku's build environment is always clean, so the import is needed; my local environment already has the binary, so the import can be skipped. Build constraints to the rescue; cf. heroku/heroku-buildpack-go#15. |
Issue #5198 has been merged into this issue. |
Proposed fix at https://golang.org/cl/155160043 |
CL https://golang.org/cl/155160043 mentions this issue. |
Looking at this again, I don't understand why we even allow this. It seems to me that if code says 'package main', it should be a command (always) and not importable. I won't touch this for Go 1.4, but for Go 1.5 I think we should probably just disallow imports of package main. If I remember correctly, that was never intended to work. I'd be happy to hear counterarguments. Labels changed: added release-go1.5, removed release-none. |
I'm currently using this to build busybox-style binaries. Each command's main package exports a reference to its main function, and a wrapper command imports them all and selectively calls one, switching on os.Args[0]. This way, the commands are buildable both individually and wrapped in a busybox-style combined binary. I could also achieve this by splitting each command up into two packages -- one non-main package containing the implementation, and one main package containing a shim that calls into the first -- and will need to change to this approach if Go 1.5 removes the ability to import main packages. But with my current approach, I need fewer overall packages and less indirection. |
CL https://golang.org/cl/10925 mentions this issue. |
I'm curious -- what has changed since https://golang.org/cl/4126053? It seems like the intent of that change was to allow what https://golang.org/cl/10925 is now disallowing. |
CL 4126053 is a change to the spec, which describes the language. The language permits importing a package named main. That can be used, for example, when writing unit tests for functions in commands that use package main. This issue here is about the go tool, not the language. The question is whether the go tool should permit packages to import a package that defines a command. The general consensus is that it should not. |
Got it. Thanks. |
FWIW, this "fixing" commit just made writing hybrid single-call/multi-call binaries (i.e. Busybox, Toybox, git, etc) take a bit more work and pointless boilerplate, whereas prior to this it was extremely simple and clean (in fact, considerably simpler than any other language I've seen, including C, which has traditionally been considered "the language" for this kind of thing). FWIW, this is also a breaking change for some of my stuff. |
@extemporalgenome what kind of stuff broke? Can you provide more details? |
@adg I've put together a minimal representative example (and explanation in the README) at https://github.com/extemporalgenome/multicall-example |
I see, thanks for the thorough example. I guess the main question is whether the brevity benefit in the minority case is worth the conceptual overhead of encoding what it means for a 'package main' to be importable. The Go tool, at least, is not equipped to deal with this properly. I'm feeling ambivalent about it. :-/ |
Indeed. The main points: 1) There is at least one additional legitimate need for which importing main is strictly the simplest/shortest approach. 2) Preventing import of main isn't a direct solution to #4210 -- it merely makes the cause of #4210 impossible. A direct fix seems plausible (make the go tool aware of the difference between binaries and archives), thus I'd argue that the toolchain transparently could become well equipped to deal with these matters properly. (Oddly enough, #4210 has never affected me -- only the commit closing #4210 will affect me). If the toolchain change remains in place or is explicitly re-ordained, it needs to be addressed in the specification. The spec currently doesn't have any "Implementation restriction:" notes that allow the toolchain to prohibit imports of main, thus with this change, the toolchain at best supports only a subset of the [current] language. That said, the previously mentioned CL 4126053 removed a yet-earlier language-level prohibition of such imports. Since the official toolchain is, clearly, the canonical Go implementation, toolchain changes of this nature are defacto language changes, thus this would be a defacto reversal of the prior consensus reached in the codereview 4126053. I would personally be fully satisfied if all the LGTM's from 4126053 also put LGTM's on CL 10925, though I can't speak for @kevinwallace, who also has the same use-case as I do, and would also need to reorganize code if Ian's commit sticks through to the 1.5 release. Additionally/alternatively, I will be at GopherCon (including the optional days), and will happily demonstrate my counter-case; if informed consensus occurs in either direction, I will of course graciously yield. |
FWIW, my code has already been reorganized to work around this, and while I would be happy to see an outcome that allows importing of main packages, I'm not particularly attached to that outcome at this point. |
main packages are not importable; see the discussion at golang/go#4210 Signed-off-by: Burcu Dogan <[email protected]>
The text was updated successfully, but these errors were encountered: