Skip to content
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

Closed
rogpeppe opened this issue Oct 8, 2012 · 28 comments
Closed

cmd/go: go build does not reject importing commands #4210

rogpeppe opened this issue Oct 8, 2012 · 28 comments
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Oct 8, 2012

% echo $GOPATH
/tmp/foo
% pwd
/tmp/foo/src/y
% cat main.go
package main
import (
    "fmt"
    xmain "x"
)

func main() {
    fmt.Printf("y main called\n")
    xmain.Main()
}
% cat ../x/main.go
package main
import (
    "fmt"
)

func main() {
    Main()
}

func Main() {
    fmt.Printf("x main called\n")
}

% go build
% y
y main called
x main called
% go install
% go build
# y
./main.go:4: can't find import: "x"
% go install
% rm $GOPATH/bin/y
% go build
# y
./main.go:4: can't find import: "x"
% rm -r $GOPATH/pkg
rm: cannot remove `/tmp/foo/pkg': No such file or directory
% rm $GOPATH/bin/x
% go build
% 

It looks as if go build is treating the x binary as a valid prerequisite for the build
of y, which it is not.

Perhaps the build process should take into account the fact that main packages can
compile to both archives and executables.
@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 1:

Labels changed: added size-m.

@rsc
Copy link
Contributor

rsc commented Dec 30, 2012

Comment 2:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@josharian
Copy link
Contributor

Comment 3:

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.

@josharian
Copy link
Contributor

Comment 4:

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.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 5:

Labels changed: added go1.1maybe, removed go1.1.

@robpike
Copy link
Contributor

robpike commented May 18, 2013

Comment 6:

Labels changed: added go1.2maybe, removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 7:

Labels changed: added feature.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 8:

Issue #5198 has been merged into this issue.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 9:

Won't be fixed in time for 1.2.

Labels changed: removed go1.2maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 10:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 11:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 12:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 13:

Labels changed: added repo-main.

@kevinwallace
Copy link

Comment 14:

Proposed fix at https://golang.org/cl/155160043

@gopherbot
Copy link

Comment 15:

CL https://golang.org/cl/155160043 mentions this issue.

@rsc
Copy link
Contributor

rsc commented Oct 30, 2014

Comment 16:

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.

@ianlancetaylor
Copy link
Contributor

Comment 17:

People import the main package for tests.
As long as it works for tests I think it's probably OK to disallow importing it
otherwise.

@kevinwallace
Copy link

Comment 18:

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.

@gopherbot
Copy link

CL https://golang.org/cl/10925 mentions this issue.

@kevinwallace
Copy link

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.

@ianlancetaylor
Copy link
Contributor

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.

@kevinwallace
Copy link

Got it. Thanks.

@extemporalgenome
Copy link
Contributor

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.

@adg
Copy link
Contributor

adg commented Jun 22, 2015

@extemporalgenome what kind of stuff broke? Can you provide more details?

@extemporalgenome
Copy link
Contributor

@adg I've put together a minimal representative example (and explanation in the README) at https://github.com/extemporalgenome/multicall-example

@adg
Copy link
Contributor

adg commented Jun 24, 2015

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. :-/

@extemporalgenome
Copy link
Contributor

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.

@kevinwallace
Copy link

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.

rakyll added a commit to rakyll/containerd that referenced this issue Jan 24, 2016
main packages are not importable; see the discussion at golang/go#4210

Signed-off-by: Burcu Dogan <jbd@google.com>
@golang golang locked and limited conversation to collaborators Jun 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants