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

go/build: should package be aware of GOBIN? #4891

Open
GeertJohan opened this issue Feb 24, 2013 · 12 comments

Comments

@GeertJohan
Copy link
Contributor

commented Feb 24, 2013

When using build.Import("GOPATH/src/myCmd", "", 0) a *Package is
returned that has the field BinDir set to be GOPATH/bin. 
Package.BinDir is documented as "command install directory".
However: cmd/go states "If the GOBIN environment variable is set, commands are
installed to the directory it names instead of DIR/bin."
(http://golang.org/cmd/go/#hdr-GOPATH_environment_variable)

Is it up to pkg/go/build to check for the existence of GOBIN? Or should a user
implementation check for GOBIN?

Example of a fix:
GeertJohan/rerun@b3bf4d7

pkg/go/build source code concerning the *Package BinDir field:
http://code.google.com/p/go/source/browse/src/pkg/go/build/build.go#521
@adg

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2013

Comment 1:

Currently all handling of the GOBIN environment variable is in cmd/go.
Another (perhaps better) approach would be to add a GOBIN field to go/build's Context,
and have the handling done there. If the GOBIN field is empty it should be ignored,
preserving backward compatibility with 1.0. Although if defaultContext pulls the GOBIN
from the environment, this potentially breaks backward compat, although it's arguably a
bug fix.
In summary, I'm inclined to support adding a GOBIN field to go/build's Context, and
removing the GOBIN handling from cmd/go.
What say Russ?

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

Status changed to Accepted.

@GeertJohan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2013

Comment 2:

I have made some changes (attached diff) that yield the expected results for go/build..
Added the GOBIN field in go/build's Context. I also tried to removing the GOBIN handling
from cmd/go. However, the go tool still checks and applies GOBIN in some situations
outside the BinDir field from go/build's Package. So I'm not fully satisfied with this
patch, but it might be a good indication and/or be used for backward compatibility tests.

Attachments:

  1. gobin_in_build_package.diff (3057 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2013

Comment 3:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 4:

Labels changed: added go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 5:

Labels changed: added feature.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2013

Comment 6:

The approach seems reasonable. Should we do this?
@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2013

Comment 7:

Not for 1.2.

Labels changed: removed go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 8:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 9:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 10:

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 11:

Labels changed: added repo-main.

@GeertJohan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2014

It's been a while and I thought I'd pick this up again.

I've made the changes to pkg/go/build locally and I'm ready to commit those changes in a CL, but I'm wondering if the same CL should also remove/change the (now) obsolete handling of GOBIN in cmd/go?
cmd/go/pkg.go:269
cmd/go/pkg.go:837

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.