code review 4221047: build: convert src/cmd to use make directly and simplif...

build: convert src/cmd to use make directly and simplify src/make.bash

[dave_cheney.net, 2011-02-24 08:50:02]

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

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

[bsiegert, 2011-02-24 09:30:05]

http://codereview.appspot.com/4221047/diff/4001/src/cmd/Makefile
File src/cmd/Makefile (right):

http://codereview.appspot.com/4221047/diff/4001/src/cmd/Makefile#newcode14
src/cmd/Makefile:14: 
Maybe set .PHONY for the different targets, as they are all phony?

[dave_cheney.net, 2011-02-24 09:43:37]

> http://codereview.appspot.com/4221047/diff/4001/src/cmd/Makefile#newcode14
> src/cmd/Makefile:14: 
> Maybe set .PHONY for the different targets, as they are all phony?

cmd/Makefile was based on pkg/Makefile which doesn't define .PHONY targets
either so they don't appear to be needed (that isn't to say they should be added
in a latter CL)

[rsc, 2011-02-24 13:55:14]

it does clean up src/make.bash.
please make it look like our other makefiles, though,
which don't use .NOTPARALLEL nor .PHONY
and which typically use one-per-line lists.

also you can avoid some intermediate target
names that are only used once anyway.

more like


include ../Make.inc

# Only build tools for current architecture, and only tools written in C.
# The tools written in Go are managed by ../pkg/Makefile.
DIRS=\
	$Oa\
	$Oc\
	$Og\
	$Ol\
	cc\
	cov\
	gc\
	godefs\
	gopack\
	gotest\
	nm\
	prof\

# Clean applies to all directories, even for other architectures or
written in Go.
CLEANDIRS=\
	$(DIRS)\
	5a\
	5c\
	5g\
	5l\
	6a\
	6c\
	6g\
	6l\
	8a\
	8c\
	8g\
	8l\
	cgo\
	ebnflint\
	godoc\
	gofmt\
	goinstall\
	goyacc\
	hgpatch\

mkenam:
	(cd $Ol && bash mkenam)

install: $(patsubst %,%.install,$(DIRS))
clean: $(patsubst %,%.clean,$(CLEANDIRS))

echo-dirs:	
	...

%.install:
	...

%.clean:
	...

[dave_cheney.net, 2011-02-25 00:08:18]

Hi Russ,

Thank you for your comments

On 2011/02/24 13:55:14, rsc wrote:
> it does clean up src/make.bash.
> please make it look like our other makefiles, though,
> which don't use .NOTPARALLEL nor .PHONY
> and which typically use one-per-line lists.

Done. Please see the next patch set, which adds targets that express the
dependancies between gc and cc, and $(O)g and gc, etc. gomake -j 8 -C cmd
install now builds in parallel.

> # Only build tools for current architecture, and only tools written in C.
> # The tools written in Go are managed by ../pkg/Makefile.

Done, but I opted to use names such as $(O)a rather than $Oa as the former were
more common in the other makefiles.

> mkenam:
> 	(cd $Ol && bash mkenam)

Done:

mkenam: 
	cd $(O)l && bash mkenam && $(MAKE) enam.o

[dave_cheney.net, 2011-02-25 00:09:25]

Thanks for the review Russ and bsiegert. Please take another look.

[dave_cheney.net, 2011-02-25 05:14:43]

I discovered that calling mkenam directly from cmd/Makefile was not necessary as
the $(O)a will call make -C ../$(O)l enam.o if that target is not up to date.

[bsiegert, 2011-02-25 07:06:22]

LGTM

--Benny.

> http://codereview.appspot.com/4221047/

[rsc, 2011-02-25 18:44:15]

http://codereview.appspot.com/4221047/diff/13001/src/cmd/Makefile
File src/cmd/Makefile (right):

http://codereview.appspot.com/4221047/diff/13001/src/cmd/Makefile#newcode59
src/cmd/Makefile:59: @echo
Do you need this recipe?  I expected you'd be able to write just

gc.install: cc.install
$(O)c.install: cc.install
$(g).install: gc.install

as plain dependencies, leaving the install to the %.install recip.e

[dave_cheney.net, 2011-02-25 21:23:14]

Even better! Thanks for the tip. PTAL

http://codereview.appspot.com/4221047/diff/13001/src/cmd/Makefile
File src/cmd/Makefile (right):

http://codereview.appspot.com/4221047/diff/13001/src/cmd/Makefile#newcode59
src/cmd/Makefile:59: @echo
On 2011/02/25 18:44:15, rsc wrote:
> Do you need this recipe?  I expected you'd be able to write just
> 
> gc.install: cc.install
> $(O)c.install: cc.install
> $(g).install: gc.install
> 
> as plain dependencies, leaving the install to the %.install recip.e

Done.

[rsc, 2011-02-28 15:10:16]

I think you can simplify src/clean.bash now too.

Russ

[rsc, 2011-02-28 21:04:12]

LGTM

[adg, 2011-02-28 22:20:25]

LGTM

Nice change!

On 24 February 2011 19:50,  <dave@cheney.net> wrote:
> Reviewers: adg,
>
> Message:
> Hello adg (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> build: convert src/cmd to use make directly and simplify src/make.bash
>
> Please review this at http://codereview.appspot.com/4221047/
>
> Affected files:
>  A src/cmd/Makefile
>  R src/cmd/clean.bash
>  R src/cmd/make.bash
>  M src/make.bash
>
>
>

[adg, 2011-02-28 22:20:37]

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

build: convert src/cmd to use make directly and simplify src/make.bash

R=adg, bsiegert, rsc
CC=golang-dev
http://codereview.appspot.com/4221047

Committer: Andrew Gerrand <adg@golang.org>