Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(147)

Issue 5794065: code review 5794065: undo CL 5754088 / cae9a7c0db06 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 1 month ago by rsc
Modified:
13 years, 1 month ago
Reviewers:
CC:
bradfitz, golang-dev
Visibility:
Public.

Description

undo CL 5754088 / cae9a7c0db06 broke builders ««« original CL description cmd/go: respect $GOBIN always Before, we only consulted $GOBIN for source code found in $GOROOT, but that's confusing to explain and less useful. The new behavior lets users set GOBIN=$HOME/bin and have all go-compiled binaries installed there. Fixes issue 3269. R=golang-dev, bradfitz CC=golang-dev http://codereview.appspot.com/5754088 »»»

Patch Set 1 #

Patch Set 2 : diff -r cae9a7c0db06 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -30 lines) Patch
M doc/install-source.html View 1 1 chunk +2 lines, -3 lines 0 comments Download
M src/cmd/go/build.go View 1 4 chunks +17 lines, -18 lines 0 comments Download
M src/cmd/go/doc.go View 1 1 chunk +1 line, -3 lines 0 comments Download
M src/cmd/go/help.go View 1 1 chunk +1 line, -3 lines 0 comments Download
M src/cmd/go/pkg.go View 1 2 chunks +1 line, -3 lines 0 comments Download

Messages

Total messages: 3
rsc
Hello bradfitz (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
13 years, 1 month ago (2012-03-12 21:03:26 UTC) #1
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=9bfee3618a8c *** undo CL 5754088 / cae9a7c0db06 broke builders ««« original CL ...
13 years, 1 month ago (2012-03-12 21:03:32 UTC) #2
bradfitz
13 years, 1 month ago (2012-03-12 21:14:33 UTC) #3
LGTM

On Mon, Mar 12, 2012 at 2:03 PM, <rsc@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> undo CL 5754088 / cae9a7c0db06
>
> broke builders
>
> ««« original CL description
> cmd/go: respect $GOBIN always
>
> Before, we only consulted $GOBIN for source code
> found in $GOROOT, but that's confusing to explain
> and less useful.  The new behavior lets users set
> GOBIN=$HOME/bin and have all go-compiled binaries
> installed there.
>
> Fixes issue 3269.
>
> R=golang-dev, bradfitz
> CC=golang-dev
> http://codereview.appspot.com/**5754088<http://codereview.appspot.com/5754088>
> »»»
>
> Please review this at
http://codereview.appspot.com/**5794065/<http://codereview.appspot.com/5794065/>
>
> Affected files:
>  M doc/install-source.html
>  M src/cmd/go/build.go
>  M src/cmd/go/doc.go
>  M src/cmd/go/help.go
>  M src/cmd/go/pkg.go
>
>
> Index: doc/install-source.html
> ==============================**==============================**=======
> --- a/doc/install-source.html
> +++ b/doc/install-source.html
> @@ -393,12 +393,11 @@
>
>  <p><code>$GOBIN</code>
>  <p>
> -The location where Go binaries will be installed.
> +The location where binaries from the main repository will be installed.
> +XXX THIS MAY CHANGE TO BE AN OVERRIDE EVEN FOR GOPATH ENTRIES XXX
>  The default is <code>$GOROOT/bin</code>.
>  After installing, you will want to arrange to add this
>  directory to your <code>$PATH</code>, so you can use the tools.
> -If <code>$GOBIN</code> is set, the <a href="/cmd/go">go command</a>
> -installs all commands there.
>  </p>
>
>  <p><code>$GOARM</code> (arm, default=6)</p>
> Index: src/cmd/go/build.go
> ==============================**==============================**=======
> --- a/src/cmd/go/build.go
> +++ b/src/cmd/go/build.go
> @@ -199,8 +199,6 @@
>
>  For more about the build flags, see 'go help build'.
>  For more about specifying packages, see 'go help packages'.
> -For more about where packages and binaries are installed,
> -see 'go help gopath'.
>
>  See also: go build, go get, go clean.
>        `,
> @@ -304,13 +302,20 @@
>  )
>
>  var (
> -       gobin        = os.Getenv("GOBIN")
>        goroot       = filepath.Clean(runtime.GOROOT(**))
> +       gobin        = defaultGobin()
>        gorootSrcPkg = filepath.Join(goroot, "src/pkg")
>        gorootPkg    = filepath.Join(goroot, "pkg")
>        gorootSrc    = filepath.Join(goroot, "src")
>  )
>
> +func defaultGobin() string {
> +       if s := os.Getenv("GOBIN"); s != "" {
> +               return s
> +       }
> +       return filepath.Join(goroot, "bin")
> +}
> +
>  func (b *builder) init() {
>        var err error
>        b.print = fmt.Print
> @@ -382,24 +387,18 @@
>        pkg.load(&stk, bp, err)
>        pkg.localPrefix = dirToImportPath(dir)
>        pkg.ImportPath = "command-line-arguments"
> -       pkg.target = ""
>
> -       if pkg.Name == "main" {
> -               _, elem := filepath.Split(gofiles[0])
> -               exe := elem[:len(elem)-len(".go")] + exeSuffix
> -               if *buildO == "" {
> -                       *buildO = exe
> -               }
> -               if gobin != "" {
> -                       pkg.target = filepath.Join(gobin, exe)
> -               }
> -       } else {
> -               if *buildO == "" {
> +       if *buildO == "" {
> +               if pkg.Name == "main" {
> +                       _, elem := filepath.Split(gofiles[0])
> +                       *buildO = elem[:len(elem)-len(".go")] + exeSuffix
> +               } else {
>                        *buildO = pkg.Name + ".a"
>                }
>        }
> +       pkg.target = ""
> +       pkg.Target = ""
>        pkg.Stale = true
> -       pkg.Target = pkg.target
>
>        computeStale(pkg)
>        return pkg
> @@ -463,13 +462,13 @@
>                return a
>        }
>
> -       a.link = p.Name == "main"
> -       if p.local && (!a.link || p.target == "") {
> +       if p.local {
>                // Imported via local path.  No permanent target.
>                mode = modeBuild
>        }
>        a.objdir = filepath.Join(b.work, a.p.ImportPath, "_obj") +
> string(filepath.Separator)
>        a.objpkg = buildToolchain.pkgpath(b.work, a.p)
> +       a.link = p.Name == "main"
>
>        switch mode {
>        case modeInstall:
> Index: src/cmd/go/doc.go
> ==============================**==============================**=======
> --- a/src/cmd/go/doc.go
> +++ b/src/cmd/go/doc.go
> @@ -453,9 +453,7 @@
>  command with source in DIR/src/foo/quux is installed into
>  DIR/bin/quux, not DIR/bin/foo/quux.  The foo/ is stripped
>  so that you can add DIR/bin to your PATH to get at the
> -installed commands.  If the GOBIN environment variable is
> -set, commands are installed to the directory it names instead
> -of DIR/bin.
> +installed commands.
>
>  Here's an example directory layout:
>
> Index: src/cmd/go/help.go
> ==============================**==============================**=======
> --- a/src/cmd/go/help.go
> +++ b/src/cmd/go/help.go
> @@ -209,9 +209,7 @@
>  command with source in DIR/src/foo/quux is installed into
>  DIR/bin/quux, not DIR/bin/foo/quux.  The foo/ is stripped
>  so that you can add DIR/bin to your PATH to get at the
> -installed commands.  If the GOBIN environment variable is
> -set, commands are installed to the directory it names instead
> -of DIR/bin.
> +installed commands.
>
>  Here's an example directory layout:
>
> Index: src/cmd/go/pkg.go
> ==============================**==============================**=======
> --- a/src/cmd/go/pkg.go
> +++ b/src/cmd/go/pkg.go
> @@ -276,9 +276,6 @@
>  // load populates p using information from bp, err, which should
>  // be the result of calling build.Context.Import.
>  func (p *Package) load(stk *importStack, bp *build.Package, err error)
> *Package {
> -       if gobin != "" {
> -               bp.BinDir = gobin
> -       }
>        p.copyBuild(bp)
>
>        // The localPrefix is the path we interpret ./ imports relative to.
> @@ -541,6 +538,7 @@
>                bp, err := build.ImportDir(filepath.Join(**gorootSrc,
> arg), 0)
>                bp.ImportPath = arg
>                bp.Goroot = true
> +               bp.BinDir = gobin
>                bp.Root = goroot
>                bp.SrcRoot = gorootSrc
>                p := new(Package)
>
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b