current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

As part of my data collection, I have to run multiple kinds of coverage processing on multiple Java projects. Below is my main Makefile intented only for gmake. Portability is not a requirement for me (for this project), but DRY and following make best practices are. Please comment on my code, and if there is any way to make this better.

.PHONY: checkout all clobber clean $(checkout.all)
.SECONDARY: $(addprefix projects/,$(names))

root:=$(CURDIR)
names=$(shell cat etc/projects.txt)
checkout.all=$(addprefix checkout-,$(names))
clean.all=$(addprefix clean-,$(names))

testgen.types=original randoop tpalus

all:
  @echo use 'make <type>-<project>'
  @echo projects = $(names)
  @echo types = $(testgen.types)

checkout: $(checkout.all)
  @echo $@ done.

checkout-%: projects/%/pom.xml
  @echo $@ done.

projects build: ;  mkdir -p $@

projects/%/pom.xml: | projects build
  cd projects && $(root)/bin/checkout $*
  touch $@

clean: $(clean.all)
  @echo $@ done.

clean-%: | projects/%/pom.xml
  cd projects/$* && git clean -xfd && git reset --hard
  @echo $@ done.

clobber-%:
  cd projects && rm -rf $*

#-----------------------------------------------------------------
root:=$(CURDIR)

define testgen =
$1 : $(addprefix $(1)-,$(names))
  @echo $$(@) done.

$1-% : projects/%/.$(1).done
  @echo $$(@) done.

%/.$1.done : | %/pom.xml
  echo $$(MAKE) -C $$(*) root=$$(root) $(1)
endef

$(foreach var,$(testgen.types),$(eval $(call testgen,$(var))))

Some clarification about the intended purpose of this project. I am doing an analysis on a large number of projects from github. My method of action is to clone each project, run multiple tests on them. Before running any project, I do a git clean to make sure that the artifacts of previous tests are not present in the project directories. The types of tests and projects names vary frequently, and hence I have kept them outside the main Makefile.

share|improve this question

1 Answer 1

up vote 4 down vote accepted 
.PHONY: checkout all clobber clean $(checkout.all)

There's no clobber target, so I think that will be ignored.

The root variable is unnecessary (CURDIR should be well known by anyone using make), and is even defined twice.

names=$(shell cat etc/projects.txt)

In my experience it's more common to write out the list in the makefile. If this is so long as to clutter up the file, you could create a separate makefile for variables like this and include variables.mk or similar.

projects build: ;  mkdir -p $@

Why not split this into two lines? Also, the -p doesn't make any difference since it's only creating a single directory under the current one.

touch $@

This looks like a hack to get around checkout not updating the modified date properly, or to rebuild a project when make doesn't think it needs to be rebuilt. You might be able to just set the relevant target .PHONY or change it so that rebuilding is only done when necessary.

cd projects/$* && git clean -xfd && git reset --hard

This is rather brutal. I'd rather advise to use non-recursive make (based on Recursive Make Considered Harmful) and enumerate the files to remove to make sure you never remove anything other than generated files. You could have a separate target with cd projects/$* && git clean -xnd to check whether there's anything you still need to clean.

clobber-%:
  cd projects && rm -rf $*

This also looks dangerous. You're effectively saying that make clobber-whatever will delete whatever no matter what it is. I can't recall where I heard this advice, but I do believe it's good practice to enumerate the files which makefiles can and should work with, rather than adding general-purpose tactical nuclear missiles.

define testgen =
...
$(foreach var,$(testgen.types),$(eval $(call testgen,$(var))))

There's a lot of indirection going on here, so it's difficult to understand what will actually be run in the end. Are you sure this couldn't be done simpler?

This is a personal preference, but I tend to avoid any echo statements because the make output should make it clear what is actually happening, and comments like this often tend to get out of sync with the rest of the code. On the other hand, you can include special targets to get the value of all/any variables.

share|improve this answer
    
Thank for your comments, my clarifications are as follows –  rahul Mar 1 '13 at 23:23
    
I need the $root directory to save the location of outer project directory (it is passed to the recursive make). I have control only over the top level makefile. the inner projects under projects/... are independent projects expected to only expose a makefile with defined targets. –  rahul Mar 1 '13 at 23:24
    
mkdir -p will not fail if the directory exists unlike mkdir. Hence the usage. Also I dont want to projects build: ; mkdir -p $@ split this since that would be counter to DRY principle. I agree about touch. –  rahul Mar 1 '13 at 23:25
1  
Reasonable caveats all, and the main reason code review for strangers is difficult: You don't know which constraints the other is working under. –  l0b0 Mar 2 '13 at 9:48
    
I agree, and given the lack of info, your review was pretty good. thanks. –  rahul Mar 2 '13 at 10:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.