code review 4023065: build: allow clean.bash to work on fresh checkout

build: allow clean.bash to work on fresh checkout

Must be invoked as ./clean.bash --gomake make
(or --gomake gmake, depending on the name of
GNU make).

[rsc, 2011-02-03 18:39:36]

Hello niemeyer (cc: golang-dev@googlegroups.com),

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

[niemeyer, 2011-02-04 07:29:27]

Thanks for pushing this, Russ.  Just one suggestion.

http://codereview.appspot.com/4023065/diff/2002/src/clean.bash
File src/clean.bash (right):

http://codereview.appspot.com/4023065/diff/2002/src/clean.bash#newcode49
src/clean.bash:49: $gomake clean
Using environment variables here looks pretty clean, and
much better than aliases.

I suggest using $MAKE here as well, though, rather than
asking the user for it. The script itself (env.bash)
already has the logic to use one or the other internally, 
and if this logic is broken cleaning will already break
anyway since the variable is used within the Makefile.

This will remove the need for the option parsing on
both clean.bash files.

[rsc, 2011-02-04 14:41:35]

On Fri, Feb 4, 2011 at 02:29,  <n13m3y3r@gmail.com> wrote:
> Thanks for pushing this, Russ.  Just one suggestion.
>
>
> http://codereview.appspot.com/4023065/diff/2002/src/clean.bash
> File src/clean.bash (right):
>
> http://codereview.appspot.com/4023065/diff/2002/src/clean.bash#newcode49
> src/clean.bash:49: $gomake clean
> Using environment variables here looks pretty clean, and
> much better than aliases.
>
> I suggest using $MAKE here as well, though, rather than
> asking the user for it. The script itself (env.bash)
> already has the logic to use one or the other internally,
> and if this logic is broken cleaning will already break
> anyway since the variable is used within the Makefile.
>
> This will remove the need for the option parsing on
> both clean.bash files.

It seems plausible that on a BSD system a user might
have MAKE=make, which is not what we want.
It is okay to refer to $(MAKE) inside Makefiles, because
GNU Make (which is what we are running at that point)
sets that variable to itself.  But if someone invokes a
clean.bash script by hand I do not believe we can depend
on that.

You're right that the top-level clean.bash can do the right
thing automatically, and I'll add that.  I think the clean.bash
that it invokes must keep the parameter.

Russ

[niemeyer, 2011-02-04 14:55:17]

> It seems plausible that on a BSD system a user might
> have MAKE=make, which is not what we want.

But how would that affect the build?

We have this in env.bash:

MAKE=make
if ! make --version 2>/dev/null | grep 'GNU Make' >/dev/null; then
        MAKE=gmake
fi


And we generate gomake like this:

        echo 'exec '$MAKE' "$@"'
) >"$GOBIN"/gomake

So I can't see how $MAKE would be wrong, or at least
how it could be wrong in a way that does not already
affect the build.

[rsc, 2011-02-04 17:28:32]

> We have this in env.bash:

After installing you should be able to

    cd $GOROOT/src/cmd
    ./clean.bash

I don't want that to fail if you keep $MAKE set funny.

Russ

[niemeyer, 2011-02-04 17:41:03]

> After installing you should be able to
> 
>     cd $GOROOT/src/cmd
>     ./clean.bash
> 
> I don't want that to fail if you keep $MAKE set funny.

Aha, understood now.

So I suggest using $MAKE in src/clean.bash, and keeping your
version of src/cmd/clean.bash as is.  src/clean.bash can then
do $MAKE --gomake=$MAKE, and that enables people to correctly
do cd src && ./clean.bash without worring about options.

Either way, that's a suggestion.  The change LGTM and is an
improvement over today's situation.  Thanks for pushing it.

[rsc, 2011-02-04 19:33:10]

*** Submitted as 779baf52b0c3 ***

build: allow clean.bash to work on fresh checkout

Must be invoked as ./clean.bash --gomake make
(or --gomake gmake, depending on the name of
GNU make).

R=niemeyer
CC=golang-dev
http://codereview.appspot.com/4023065