Merge lp:~wence/fluidity/make-fixes into lp:fluidity

[Lawrence Mitchell]

Reduce spurious recompilations in clean or mostly-clean trees

Avoid touching lib/libfluidity.a unless actually necessary. This comes in a few parts:

o Use bzr revision-info to set up Fluidity's version number (and only update confdefs.h if the version changed)
o Calculate hashes of the check_options and register_diagnostics files and only update them if the hash changes
o Avoid depending on .PHONY targets that just point directly at actual targets

I think I've addressed all of the comments (mostly mine) below, and copyright assignments are in, so I think this is good to go. Here's the original description:

Various small fixes to decrease build times in mostly built trees.

The main effect of the change is to avoid touching lib/libfluidity.a as much as possible. Timing example, in a just-built tree:

before:
$ time make all
real 0m40.936s
user 0m19.053s
sys 0m21.117s

after:
$ time make all
real 0m11.736s
user 0m7.471s
sys 0m4.223s

As a bonus, the version-number fluidity reports is now meaningful again.

[Patrick Farrell (pefarrell)]

Hi Lawrence,

Looks like great work. I've always found this business of the build being slow quite annoying.

I tried it out, and it changed the time from 9s to 7s on my workstation. Quick question: there is a noticeable delay at the start of the build now, as it calls bzr version-info; it appears like it isn't doing anything for a second or two. Is there any way to speed up the bzr version-info, or is that just how long it takes, and there's nothing we can do? With your change, but without the bzr version-info, it takes just 3s to compile on my workstation.

[Lawrence Mitchell (wence)]

It seems like the custom version-info format increases the time spent significantly:

fluidity-trunk $ time bzr version-info >/dev/null

real 0m0.293s
user 0m0.138s
sys 0m0.150s

fluidity-trunk $ time bzr version-info --custom --template="{revno} ({revision_id})" >/dev/null

real 0m0.865s
user 0m0.522s
sys 0m0.338s

If I try this:
$ cat > gen-version<<\EOF
#!/bin/bash

while read f; do
    if [[ $f == revision-id:\ * ]]; then
        revid=${f#revision-id: }
    elif [[ $f == revno:\ * ]]; then
        revno=${f#revno: }
    fi
done < <(bzr version-info) && echo "$revno (revid $revid)"
EOF

fluidity-trunk $ time ./gen-version >/dev/null

real 0m0.297s
user 0m0.123s
sys 0m0.170s

But is not portable over changes in the output format.

The clean solution is to use the format-spec, the fast solution appears to be to munge the textual output by hand.

The really fast solution is to do:

VERSION=$(shell cat .bzr/branch/last-revision)

but that is /really/ ugly, so I don't want to be the one doing it.

[Lawrence Mitchell (wence)]

I'm not quite happy with this entire series as is. Specifically, it breaks the sequence:

make
make clean-light
make

Since libwm is not cleaned by clean-light, the object files are left lying around. The default rule in libwm/Makefile with this patch series is:

default: $(LIB)

$(LIB): $(OBJS)
     $(AR) $(ARFLAGS) $(LIB) $(OBJS)
 cp *.h ../include/

But the make sequence is:

make debug
make libwm

debug touches $(LIB) and so the modtime for $(LIB) is newer than $(OBJS), so they're not linked in to the library. This causes an undefined symbol link error when we try to build bin/fluidity.

I see two ways of fixing this:

revert to the previous (.PHONY) rule:

default: $(OBJS)
        ...

So that the object files are always stuffed in $(LIB)

Or add libwm to the list of clean-light targets.

I prefer the latter, thoughts?

[Tim Bond (timothy-bond)]

Looks good, approved.

review: Approve

=== modified file 'Makefile.in'
--- Makefile.in	2011-06-02 11:01:17 +0000
+++ Makefile.in	2011-06-27 14:15:53 +0000
@@ -63,7 +63,7 @@
 
 EVAL = scripts/silenteval
 
-VERSION = $(shell svnversion .)
+VERSION = $(shell bzr revision-info)
 
 ifeq (@MBA2D@,yes)
 LIBMBA2D = lib/libmba2d.a
@@ -143,7 +143,7 @@
 	@cd @FLFEMDEM_PATH@;$(MAKE)
 endif
 
-bin/@FLUIDITY@: main.o fluidity_library
+bin/@FLUIDITY@: main.o lib/lib@FLUIDITY@.a
 	@echo "BUILD fluidity"
 	@echo "    MKDIR bin"
 	@mkdir -p bin
@@ -262,8 +262,7 @@
 fluidity_library: lib/lib@FLUIDITY@.a
 lib/lib@FLUIDITY@.a: $(OBJS) python_build sub_system 
 	@echo "DEFINE __FLUIDITY_VERSION__"
-	@echo "#undef __FLUIDITY_VERSION__" >> include/confdefs.h
-	@echo "#define __FLUIDITY_VERSION__ \"$(VERSION)\"" >> include/confdefs.h
+	@grep "$(VERSION)" include/confdefs.h >/dev/null 2>&1 || (echo "#undef __FLUIDITY_VERSION__" >> include/confdefs.h && echo "#define __FLUIDITY_VERSION__ \"$(VERSION)\"" >> include/confdefs.h)
 	@echo "BUILD libfluidity"
 	@echo "    MKDIR lib"
 	@mkdir -p lib
@@ -309,10 +308,10 @@
 	@cd main; $(MAKE)
 	@echo "    MAKE options_check"
 	@./scripts/make_check_options
-	@cd preprocessor; $(MAKE) check_options
+	@cd preprocessor; $(MAKE) check_options.o
 	@echo "    MAKE register_diagnostics"
 	@./scripts/make_register_diagnostics
-	@cd preprocessor; $(MAKE) register_diagnostics
+	@cd preprocessor; $(MAKE) register_diagnostics.o
 
 fldecomp: fluidity_library 
 	@echo "BUILD fldecomp"
@@ -334,6 +333,8 @@
 clean-light:
 	@echo "    CLEAN debug"
 	@cd debug; $(MAKE) clean
+	@echo "    CLEAN libwm"
+	@cd libwm; $(MAKE) clean
 	@echo "    CLEAN femtools"
 	@cd femtools; $(MAKE) clean
 	@echo "    CLEAN femtools/tests"

=== modified file 'libwm/Makefile.in'
--- libwm/Makefile.in	2011-05-27 13:44:17 +0000
+++ libwm/Makefile.in	2011-06-27 14:15:53 +0000
@@ -63,9 +63,10 @@
 	@echo "        CXX $<"
 	$(CXX) $(CXXFLAGS) -I. -I../include -c $<
 
-default: $(OBJS)
+$(LIB): $(OBJS)
 	$(AR) $(ARFLAGS) $(LIB) $(OBJS)
 	cp *.h ../include/
+default: $(LIB)
 
 clean:
 	 rm -f *.o *.mod

=== modified file 'preprocessor/Makefile.in'
--- preprocessor/Makefile.in	2011-05-19 14:29:56 +0000
+++ preprocessor/Makefile.in	2011-06-27 14:15:53 +0000
@@ -74,17 +74,17 @@
 	@$(AR) $(ARFLAGS) $(LIB) $(OBJS)
 default: $(LIB)
 
-.PHONY:check_options
-
 # This gets run last by the fluidity build process.
-check_options: check_options.o
+check_options.o: check_options.F90
+	@echo "        FC $<"
+	$(FC) $(FCFLAGS) -c $<
 	mkdir -p ../lib
 	$(AR) $(ARFLAGS) $(LIB) check_options.o &> check.log
 
-.PHONY:register_diagnostics
-
 # This gets run last by the fluidity build process.
-register_diagnostics: register_diagnostics.o
+register_diagnostics.o: register_diagnostics.F90
+	@echo "        FC $<"
+	$(FC) $(FCFLAGS) -c $<
 	mkdir -p ../lib
 	$(AR) $(ARFLAGS) $(LIB) register_diagnostics.o &> register_diagnostics.log
 

=== modified file 'scripts/make_check_options'
--- scripts/make_check_options	2010-11-16 23:08:06 +0000
+++ scripts/make_check_options	2011-06-27 14:15:53 +0000
@@ -1,6 +1,9 @@
 #!/usr/bin/env python
 import re
 import glob
+import hashlib
+import StringIO
+
 
 # File header
 header="""
@@ -11,9 +14,23 @@
 end subroutine check_options
 """
 
-outfile=file("preprocessor/check_options.F90", "w")
-
-outfile.write(header)
+outfile='preprocessor/check_options.F90'
+
+# get sha1 digest of existing generated file.  Can't use 'rw' here
+# because it updates the modtime of the file, which we're trying to
+# avoid doing.
+orig=hashlib.sha1()
+try:
+    f=open(outfile, 'r')
+    orig.update(f.read())
+except IOError:
+    pass
+else:
+    f.close()
+
+# Now read module files to generate potential new data
+output=StringIO.StringIO()
+output.write(header)
 
 # List of fortran source files.
 fortran_files=glob.glob("*/*.F")+glob.glob("*/*.F90")
@@ -23,7 +40,7 @@
 module_list=[]
 
 for filename in fortran_files:
-    
+
     fortran=file(filename,"r").read()
 
     modules=module_re.findall(fortran)
@@ -31,21 +48,37 @@
     for module in modules:
 
         if re.search(r"^\s*subroutine\s+"+module+"_check_options\s*$",\
-                     fortran,\
-                     re.IGNORECASE|re.MULTILINE):
+                         fortran,\
+                         re.IGNORECASE|re.MULTILINE):
             module_list.append(module)
 
 for module in module_list:
 
-        outfile.write("  use "+module+", only: "+module+"_check_options\n")
+    output.write("  use "+module+", only: "+module+"_check_options\n")
 
 # Ensure that the subroutine is legal in the trivial case.
-outfile.write("""
+output.write("""
    continue
    """)
 
 for module in module_list:
 
-    outfile.write("  call "+module+"_check_options\n")
-
-outfile.write(footer)
+    output.write("  call "+module+"_check_options\n")
+
+output.write(footer)
+
+new=hashlib.sha1()
+new.update(output.getvalue())
+
+# Only write file if sha1sums differ
+if new.digest() != orig.digest():
+    try:
+        f=open(outfile, 'w')
+        f.write(output.getvalue())
+    except IOError:
+        # Fixme, this should fail better
+        pass
+    else:
+        f.close()
+
+output.close()

=== modified file 'scripts/make_register_diagnostics'
--- scripts/make_register_diagnostics	2011-02-05 16:05:31 +0000
+++ scripts/make_register_diagnostics	2011-06-27 14:15:53 +0000
@@ -1,6 +1,9 @@
 #!/usr/bin/env python
 import re
 import glob
+import hashlib
+import StringIO
+
 
 # File header
 header="""
@@ -11,9 +14,23 @@
 end subroutine register_diagnostics
 """
 
-outfile=file("preprocessor/register_diagnostics.F90", "w")
-
-outfile.write(header)
+outfile = 'preprocessor/register_diagnostics.F90'
+
+# get sha1 digest of existing generated file.  Can't use 'rw' here
+# because it updates the modtime of the file, which we're trying to
+# avoid doing.
+orig=hashlib.sha1()
+try:
+    f=open(outfile, 'r')
+    orig.update(f.read())
+except IOError:
+    pass
+else:
+    f.close()
+
+# Now read module files to generate potential new data
+output=StringIO.StringIO()
+output.write(header)
 
 # List of fortran source files.
 fortran_files=glob.glob("*/*.F")+glob.glob("*/*.F90")
@@ -23,7 +40,7 @@
 module_list=[]
 
 for filename in fortran_files:
-    
+
     fortran=file(filename,"r").read()
 
     modules=module_re.findall(fortran)
@@ -31,21 +48,36 @@
     for module in modules:
 
         if re.search(r"^\s*subroutine\s+"+module+"_register_diagnostic\s*$",\
-                     fortran,\
-                     re.IGNORECASE|re.MULTILINE):
+                         fortran,\
+                         re.IGNORECASE|re.MULTILINE):
             module_list.append(module)
 
 for module in module_list:
 
-        outfile.write("  use "+module+", only: "+module+"_register_diagnostic\n")
+    output.write("  use "+module+", only: "+module+"_register_diagnostic\n")
 
 # Ensure that the subroutine is legal in the trivial case.
-outfile.write("""
+output.write("""
    continue
    """)
 
 for module in module_list:
 
-    outfile.write("  call "+module+"_register_diagnostic\n")
-
-outfile.write(footer)
+    output.write("  call "+module+"_register_diagnostic\n")
+
+output.write(footer)
+
+new=hashlib.sha1()
+new.update(output.getvalue())
+
+# Only write file if sha1sums differ
+if new.digest() != orig.digest():
+    try:
+        f=open(outfile, 'w')
+        f.write(output.getvalue())
+    except IOError:
+        # Fixme, this should fail better
+        pass
+    else:
+        f.close()
+output.close()