code review 5642045: cmd/dist: fix GOROOT_FINAL

cmd/dist: fix GOROOT_FINAL

[niemeyer, 2012-02-07 02:29:00]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-02-07 20:13:28]

I'm really not sure what to do here.  The dist command
gets run during the bootstrap but then it also might be
run after the final install.

It seems to me that maybe instead of reading $GOROOT_FINAL
from the environment, the compilation of the dist command
should be passing _that_ as the DEFAULT_GOROOT macro,
and that the dist command should be invoked during the build
with $GOROOT set, to override that default for the build.

Then you'd use default_goroot here instead of goroot_final.
The dist tool wouldn't even know about $GOROOT_FINAL;
only the build script would, while preparing the arguments
to cc during the build of dist itself.

Russ

[gustavo_niemeyer.net, 2012-02-07 23:06:47]

> I'm really not sure what to do here.  The dist command
> gets run during the bootstrap but then it also might be
> run after the final install.

My gut reaction when I first saw DEFAULT_ROOT was that it was actually
the same thing as GOROOT_FINAL. I'm more convinced now that this is
indeed the case.

> Then you'd use default_goroot here instead of goroot_final.
> The dist tool wouldn't even know about $GOROOT_FINAL;
> only the build script would, while preparing the arguments
> to cc during the build of dist itself.

I went into the opposite direction, as that allows us to get rid of
the idea of a default_root entirely, and live with a single concept.

PTAL to see what you think. I've successfully built the Ubuntu
packages with this change, and the same compiled binaries (including
dist) work before installing with $GOROOT set, and also after
installing without $GOROOT.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[gustavo_niemeyer.net, 2012-02-07 23:15:02]

Hold on. GOROOT_FINAL is now guaranteed to be set when building dist.
The logic may be made simpler.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2012-02-07 23:16:15]

Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-09 18:48:31]

Thanks for working on this.  I think it is close,
but I don't understand how it works unless you
also set $GOROOT.  I don't want to have to set
$GOROOT before my build: make.bash knows 
what directory it is in and should do that for me.

Please test a build without GOROOT set at all, and with
GOROOT_FINAL set to a different place than where the
build is happening.

I think it would suffice to use $GOROOT to override
$GOROOT_FINAL during the initial build, so it should
suffice to make make.bash say something like

export GOROOT="$(cd .. && pwd)"
GOROOT_FINAL="${GOROOT_FINAL:-$GOROOT}"
DEFGOROOT='-DGOROOT_FINAL="'"$GOROOT_FINAL"'"'

and similarly in make.bat.

http://codereview.appspot.com/5642045/diff/7006/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

http://codereview.appspot.com/5642045/diff/7006/src/cmd/dist/build.c#newcode76
src/cmd/dist/build.c:76: if(b.len > 0) {
No braces around 1-line body, sorry.

[niemeyer, 2012-02-09 22:13:38]

Hello rsc@golang.org, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-09 22:14:05]

Were you able to produce the failure scenario I imagined?

[rsc, 2012-02-09 22:15:28]

LGTM

http://codereview.appspot.com/5642045/diff/10002/src/make.bat
File src/make.bat (right):

http://codereview.appspot.com/5642045/diff/10002/src/make.bat#newcode21
src/make.bat:21: if x%GOROOT_FINAL%==x set GOROOT_FINAL="%GOROOT%"
I suspect you need to say if "x%GOROOT_FINAL%"=="x" here.  It can't hurt.

[gustavo_niemeyer.net, 2012-02-09 22:16:10]

> Were you able to produce the failure scenario I imagined?

I was indeed. It fails if $GOROOT_FINAL is set but not $GOROOT. It
didn't fail with both unset since $GOROOT_FINAL and $GOROOT were the
same.

Please note I'm flying blind with the make.bat changes. I think it's
right, but they're untested.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-02-09 22:19:34]

On Thu, Feb 9, 2012 at 17:15, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Please note I'm flying blind with the make.bat changes. I think it's
> right, but they're untested.

The builders are running make.bat now, so that's something.

Russ

[niemeyer, 2012-02-09 22:19:38]

http://codereview.appspot.com/5642045/diff/10002/src/make.bat
File src/make.bat (right):

http://codereview.appspot.com/5642045/diff/10002/src/make.bat#newcode21
src/make.bat:21: if x%GOROOT_FINAL%==x set GOROOT_FINAL="%GOROOT%"
On 2012/02/09 22:15:29, rsc wrote:
> I suspect you need to say if "x%GOROOT_FINAL%"=="x" here.  It can't hurt.

I think it should work since we had 

    if x%1==x--no-banner

at the end. It doesn't hurt indeed. I'll change both.

[rsc, 2012-02-09 22:24:40]

>> I suspect you need to say if "x%GOROOT_FINAL%"=="x" here.  It can't
>
> hurt.
>
> I think it should work since we had
>
>    if x%1==x--no-banner
>
> at the end. It doesn't hurt indeed. I'll change both.

This one I know won't have spaces in %1.

%GOROOT_FINAL% may well have spaces.
That's what I'm guarding against.  Quotes in both is fine.
As long as the quotes are on each argument separately,
even if the if statement treats them as literal characters,
it will still get the right answer.

[niemeyer, 2012-02-09 22:47:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=f3575ec144d5 ***

cmd/dist: fix GOROOT_FINAL

R=rsc, gustavo
CC=golang-dev
http://codereview.appspot.com/5642045