Makefile for automatically processing image files

Question

I have a Makefile that is used to automatically process some image files. The basic idea behind this is

• copy everything from col-noborder and col-rewnew to col
• use ImageMagick convert to copy the files from col-addborder to col and add a pre-defined border
• use ImageMagick convert to copy the files from col to bw while applying a grayscale (greyscale?) filter

Due to the layout of the project, I have to apply this filter for five sets of identically structured subdirectories. Since the project contains quite a number of image files, I want to use parallel processing and update only the files whose sources have changed (hence the use of make in the first place).

The current implementation does what it is supposed to do, but it is quite ugly. It basically generates a secondary Makefile.raster based on the input files and then defers to the targets in that generated file. The generation of that file takes over 15 seconds alone, which leaves quite a bit of room for improvement.

I'm sure that there are various ways to improve this script. Can I get the same process (that is, including parallel processing and update-only) using a single, more dynamic Makefile? If not, can I somehow speed up the generation of the secondary Makefile? Is there anything else I should correct?

IMGPATH1 = content/part1/raster
IMGPATH2 = content/part2/raster
IMGPATH3 = content/part3/raster
IMGPATH4 = content/part4/raster
IMGPATH5 = content/part5/raster
IMGPATHS = $(IMGPATH1) $(IMGPATH2) $(IMGPATH3) $(IMGPATH4) $(IMGPATH5)
RASTERMAKEFILE = Makefile.raster
rastercoladdborderfiles := $(wildcard $(addsuffix /col-addborder/*, $(IMGPATHS)))
rastercolnoborderfiles := $(wildcard $(addsuffix /col-noborder/*, $(IMGPATHS)))
rastercolrenewfiles := $(wildcard $(addsuffix /col-renew/*, $(IMGPATHS)))
rastercolfiles := $(wildcard $(addsuffix /col/*, $(IMGPATHS)))
rasterbwfiles := $(wildcard $(addsuffix /col/*, $(IMGPATHS)))
rastermakefile: force 
 rm -f $(RASTERMAKEFILE)
 printf "all: allcolnoborder allcolrenew allcoladdborder allbw\n\n" >> $(RASTERMAKEFILE)
 # copy /col-noborder/ to /col/
 for infile in $(rastercolnoborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-noborder/col/'`; \
 printf "$$outfile: $$infile\n" >> $(RASTERMAKEFILE); \
 printf "\tcp $$infile $$outfile\n" >> $(RASTERMAKEFILE); \
 done; \
 printf "allcolnoborder: " >> $(RASTERMAKEFILE)
 for infile in $(rastercolnoborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-noborder/col/'`; \
 printf "$$outfile " >> $(RASTERMAKEFILE); \
 done
 printf "\n\n" >> $(RASTERMAKEFILE)
 # copy /col-renew/ to /col/ 
 for infile in $(rastercolrenewfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-renew/col/'`; \
 printf "$$outfile: $$infile\n" >> $(RASTERMAKEFILE); \
 printf "\tcp $$infile $$outfile\n" >> $(RASTERMAKEFILE); \
 done
 printf "allcolrenew: " >> $(RASTERMAKEFILE)
 for infile in $(rastercolrenewfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-renew/col/'`; \
 printf "$$outfile " >> $(RASTERMAKEFILE); \
 done
 printf "\n\n" >> $(RASTERMAKEFILE)
 # add border to images in /col-addborder/, copying them to /col/
 for infile in $(rastercoladdborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-addborder/col/'`; \
 printf "$$outfile: $$infile\n" >> $(RASTERMAKEFILE); \
 printf "\tconvert $$infile -bordercolor \"rgb(234,241,246)\" -border 10x10 $$outfile\n" >> $(RASTERMAKEFILE); \
 done; \
 printf "allcoladdborder: " >> $(RASTERMAKEFILE)
 for infile in $(rastercoladdborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-addborder/col/'`; \
 printf "$$outfile " >> $(RASTERMAKEFILE); \
 done
 printf "\n\n" >> $(RASTERMAKEFILE)
 # create the grayscale images from the color images
 for infile in $(rastercoladdborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-addborder/bw/'`; \
 colfile=`echo $$infile | sed 's/col-addborder/col/'`; \
 printf "$$outfile: $$colfile\n" >> $(RASTERMAKEFILE); \
 printf "\tconvert $$colfile -colorspace Gray -level -15%%,100%% $$outfile\n" >> $(RASTERMAKEFILE); \
 done
 for infile in $(rastercolnoborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-noborder/bw/'`; \
 colfile=`echo $$infile | sed 's/col-noborder/col/'`; \
 printf "$$outfile: $$colfile\n" >> $(RASTERMAKEFILE); \
 printf "\tconvert $$colfile -colorspace Gray -level -15%%,100%% $$outfile\n" >> $(RASTERMAKEFILE); \
 done
 for infile in $(rastercolrenewfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-renew/bw/'`; \
 colfile=`echo $$infile | sed 's/col-renew/col/'`; \
 printf "$$outfile: $$colfile\n" >> $(RASTERMAKEFILE); \
 printf "\tconvert $$colfile -colorspace Gray -level -15%%,100%% $$outfile\n" >> $(RASTERMAKEFILE); \
 done
 printf "allbw: " >> $(RASTERMAKEFILE)
 for infile in $(rastercoladdborderfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-addborder/bw/'`; \
 printf "$$outfile " >> $(RASTERMAKEFILE); \
 	done
 	for infile in $(rastercolnoborderfiles); \
 	do \
 		outfile=`echo $$infile | sed 's/col-noborder/bw/'`; \
 		printf "$$outfile " >> $(RASTERMAKEFILE); \
 done
 for infile in $(rastercolrenewfiles); \
 do \
 outfile=`echo $$infile | sed 's/col-renew/bw/'`; \
 printf "$$outfile " >> $(RASTERMAKEFILE); \
 done
 printf "\n\n" >> $(RASTERMAKEFILE)
rasterfiles: rastermakefile
 make -j 2 -f $(RASTERMAKEFILE)
rasterfiles-force: rastermakefile
 rm -f $(rastercolfiles)
 rm -f $(rasterbwfiles)
 make -j 2 -f $(RASTERMAKEFILE)
force: ;

Answer 1

Overview

As I understand it, you want use GNU Make to do...

• content/part?/raster/bw/* — the ultimate goal
  • content/part?/raster/col/* — a staging place before conversion to grayscale
    • content/part?/raster/col-addborder/* — source files that need a border to be added while copying to col
    • content/part?/raster/col-noborder/* — source files to be copied to col
    • content/part?/raster/col-renew/* — source files to be copied to col

Therefore, I believe that the makefile needs to accomplish...

1. Enumerate source files in content/part?/raster/{col-addborder,col-noborder,col-renew}/* using $(wildcard ...).
2. Based on the results of (1), calculate the list of files that should exist in content/part?/raster/col, and make them if necessary.
3. Based on the results of (2), calculate the list of files that should exist in content/part?/raster/bw, and make them if necessary.

As you have found, generating makefiles like that, which embed the names of the files that currently exist, is a bad idea. That's not how Make is intended to be used at all.

To get make to do everything for you, there are three features that can help:

• Static pattern rules
• The eval function
• The call function

Static pattern rules

These rules say: "For targets that look like some pattern, tweak the path like so to obtain the prerequisite(s). Here's the recipe."

For example, you "want" a rule that says:

$(rasterbwfiles): $(imgpathN)/bw/%: $(imgpathN)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'

Unfortunately, that doesn't quite work, since $(imgpathN) is also varying, and static pattern rules can only have one varying part, which is the % placeholder. So, we actually need five static pattern rules.

Strategy A: Write every rule five times in your makefile

You could write, for example:

$(rasterbwfiles1): $(IMGPATH1)/bw/%: $(IMGPATH1)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'
$(rasterbwfiles2): $(IMGPATH2)/bw/%: $(IMGPATH2)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'
$(rasterbwfiles3): $(IMGPATH3)/bw/%: $(IMGPATH3)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'
$(rasterbwfiles4): $(IMGPATH4)/bw/%: $(IMGPATH4)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'
$(rasterbwfiles5): $(IMGPATH5)/bw/%: $(IMGPATH5)/col/%
 convert '$<' -colorspace Gray -level -15%,100% '$@'

... but that's inelegant.

Strategy B: Dynamically evaluated makefile

Instead of writing parts of the makefile five times, let Make evaluate dynamic rules, using $(eval ...). To evaluate varying text, use $(call ...) on a variable that acts like a template.

IMGPATH1 = content/part1/raster
IMGPATH2 = content/part2/raster
IMGPATH3 = content/part3/raster
IMGPATH4 = content/part4/raster
IMGPATH5 = content/part5/raster
IMGPATHS = $(IMGPATH1) $(IMGPATH2) $(IMGPATH3) $(IMGPATH4) $(IMGPATH5)
CP = cp
CONVERT = convert
CP_RULE = $(CP) '$<' '$@'
BORDER_RULE = $(CONVERT) '$<' -bordercolor "rgb(234,241,246)" -border 10x10 '$@'
GRAY_RULE = $(CONVERT) '$<' -colorspace Gray -level -15%,100% '$@'
######################################################################
define defs_template
rastercoladdborderfiles_$(1) := $$(wildcard $(1)/col-addborder/*)
rastercolfiles_from_addborder_$(1) := $$(subst /col-addborder/,/col/,$$(rastercoladdborderfiles_$(1)))
rastercolnoborderfiles_$(1) := $$(wildcard $(1)/col-noborder/*)
rastercolfiles_from_noborder_$(1) := $$(subst /col-noborder/,/col/,$$(rastercolnoborderfiles_$(1)))
rastercolrenewfiles_$(1) := $$(wildcard $(1)/col-renew/*)
rastercolfiles_from_renew_$(1) := $$(subst /col-renew/,/col/,$$(rastercolrenewfiles_$(1)))
rastercolfiles_$(1) := $$(sort $$(rastercolfiles_from_addborder_$(1)) \
 $$(rastercolfiles_from_noborder_$(1)) \
 $$(rastercolfiles_from_renew_$(1)))
rasterbwfiles_$(1) := $$(subst /col/,/bw/,$$(rastercolfiles_$(1)))
endef
######################################################################
define rules_template
$$(rastercolfiles_from_addborder_$(1)): $(1)/col/%: $(1)/col-addborder/%
 	$$(BORDER_RULE)
$$(rastercolfiles_from_noborder_$(1)): $(1)/col/%: $(1)/col-noborder/%
 	$$(CP_RULE)
$$(rastercolfiles_from_renew_$(1)): $(1)/col/%: $(1)/col-renew/%
 	$$(CP_RULE)
$$(rasterbwfiles_$(1)): $(1)/bw/%: $(1)/col/%
 	$$(GRAY_RULE)
endef
######################################################################
$(foreach imgpath,$(IMGPATHS),$(eval $(call defs_template,$(imgpath))))
rastercolfiles := $(foreach imgpath,$(IMGPATHS),$(rastercolfiles_$(imgpath)))
rasterbwfiles := $(foreach imgpath,$(IMGPATHS),$(rasterbwfiles_$(imgpath)))
all: $(rasterbwfiles)
.PHONY: all
$(foreach imgpath,$(IMGPATHS),$(eval $(call rules_template,$(imgpath))))

Some remarks:

• The all target should be .PHONY, since you're not actually creating a file named all. You had several other phony targets as well, which I haven't bothered to reproduce above.
• (Re-)making the bw files will automatically (re-)make the col files.
• It's considered good style to specify your recipes using variables rather than hard-coding them.
• The all rule definition has to come before all other rule definitions. However, it has to come after your variable definitions, since some of them are simply expanded variables.

As elegant as Strategy B may be, I don't fully recommend it, because dynamic makefiles are hard to write and debug.

Strategy C: Five makefiles

Instead, I suggest treating your five $(IMGPATHS) as five "projects", each with its own makefile. It just happens that all five makefiles will be identical. You can symlink them all to a canonical copy of that makefile, or you can write five makefiles that consist solely of an include ../../../Common.mak directive.

The common makefile feels more natural than the templated version above.

CP = cp
CONVERT = convert
CP_RULE = $(CP) '$<' '$@'
BORDER_RULE = $(CONVERT) '$<' -bordercolor "rgb(234,241,246)" -border 10x10 '$@'
GRAY_RULE = $(CONVERT) '$<' -colorspace Gray -level -15%,100% '$@'
######################################################################
rastercoladdborderfiles := $(wildcard col-addborder/*)
rastercolfiles_from_addborder := $(rastercoladdborderfiles:col-addborder/%=col/%)
rastercolnoborderfiles := $(wildcard col-noborder/*)
rastercolfiles_from_noborder := $(rastercolnoborderfiles:col-noborder/%=col/%)
rastercolrenewfiles := $(wildcard col-renew/*)
rastercolfiles_from_renew := $(rastercolrenewfiles:col-renew/%=col/%)
rastercolfiles := $(sort $(rastercolfiles_from_addborder) \
 $(rastercolfiles_from_noborder) \
 $(rastercolfiles_from_renew))
rasterbwfiles := $(rastercolfiles:col/%=bw/%)
######################################################################
all: $(rasterbwfiles)
.PHONY: all
######################################################################
$(rastercolfiles_from_addborder): col/%: col-addborder/%
 $(BORDER_RULE)
$(rastercolfiles_from_noborder): col/%: col-noborder/%
 $(CP_RULE)
$(rastercolfiles_from_renew): col/%: col-renew/%
 $(CP_RULE)
$(rasterbwfiles): bw/%: col/%
 $(GRAY_RULE)

Then, you'd have a top-level makefile that calls Make recursively.

IMGPATH1 = content/part1/raster
IMGPATH2 = content/part2/raster
IMGPATH3 = content/part3/raster
IMGPATH4 = content/part4/raster
IMGPATH5 = content/part5/raster
IMGPATHS = $(IMGPATH1) $(IMGPATH2) $(IMGPATH3) $(IMGPATH4) $(IMGPATH5)
TARGETS = all
RECURSE_RULE = $(foreach imgpath,$(IMGPATHS),$(MAKE) -C $(imgpath) $@; )
$(TARGETS):
 $(RECURSE_RULE)
.PHONY: $(TARGETS)

In summary, I think that this is the best way to get around the limitation that static pattern rules can only have on varying part. It works, because the five projects have a parallel structure, and each project can be remade independently of the others.

• vwegert: Wow. Just ... wow. It'll take some time to digest all of this, but - what an incredibly well-written review. Thank you very much.
• vwegert: I had to change the substitution rules in the makefiles a bit to make it work (from $(rastercoladdborderfiles:col-addborder/%:col/%) to $(rastercoladdborderfiles:col-addborder/%=col/%)). Now I'm stuck with the top-level makefile. During the recursion, it apparently tries to assemble one huge command line make -C p1 all make -C p2 all make -C p3 all ... - is it possible that there's still some problem with the foreach statement?
• 200_success: Oops, try Rev 6. (Thanks for your edit, by the way.)