[lvm-devel] [PATCH]: Added installcheck target in Makefiles

Mon May 24 15:11:07 UTC 2010

Hi,

a couple of issues, see comments inline.

> --- Makefile.in	13 Apr 2010 13:28:52 -0000	1.52
> +++ Makefile.in	21 May 2010 13:34:58 -0000
> @@ -75,9 +75,14 @@
>  endif
>  DISTCLEAN_TARGETS += cscope.out
>  
> +.PHONY: check check_cluster check_local installcheck installcheck_cluster installcheck_local
> +
>  check check_cluster check_local: all
>  	$(MAKE) -C test $(@)
>  

[snip]

> --- test/Makefile.in	12 May 2010 11:59:46 -0000	1.42
> +++ test/Makefile.in	21 May 2010 13:34:59 -0000
> @@ -46,47 +46,63 @@
>  	$(MAKE) -C api vgtest
>  endif
>  
> -all check: init.sh
> -	@echo Testing with locking_type 1
> -	./bin/harness $(RUN)
> -	@echo Testing with locking_type 3
> -	LVM_TEST_LOCKING=3 ./bin/harness $(RUN)
> +.PHONY: all \
> +  check check_cluster check_local \
> +  installcheck installcheck_cluster installcheck_local \
> +  clean distclean
The .PHONY additions should be a separate patch, arguably. Not directly
related installcheck?

> -check_cluster: init.sh
> +all check: check_local check_cluster
> +
> +check_cluster: init.sh .bin-dir-stamp
>  	@echo Testing with locking_type 3
> -	LVM_TEST_LOCKING=3 ./bin/harness $(RUN)
> +	INSTALLCHECK= LVM_TEST_LOCKING=3 ./bin/harness $(RUN)
>  
> -check_local: init.sh
> +check_local: init.sh .bin-dir-stamp
>  	@echo Testing with locking_type 1
> -	LVM_TEST_LOCKING=1 ./bin/harness $(RUN)
> +	INSTALLCHECK= LVM_TEST_LOCKING=1 ./bin/harness $(RUN)
> +
> +installcheck: installcheck_local installcheck_cluster
> +
> +installcheck_cluster: init.sh
> +	@echo Testing installation with locking_type 3
> +	INSTALLCHECK=1 LVM_TEST_LOCKING=3 ./bin/harness $(RUN)
> +
> +installcheck_local: init.sh
> +	@echo Testing installation with locking_type 1
> +	INSTALLCHECK=1 LVM_TEST_LOCKING=1 ./bin/harness $(RUN)
Since this is leaked into global environment, the name should start with
LVM_TEST_ like with other envvars that control the testsuite (probably
LVM_TEST_INSTALLED?). Another option is to use a different mechanism.

> +bin:
> +	mkdir bin
>  
> -bin/not: $(srcdir)/not.c .bin-dir-stamp
> -	$(CC) -o bin/not $<
> +bin/not: $(srcdir)/not.c Makefile bin
> +	$(CC) -o $@ $<
>  	ln -sf not bin/should
>  
> -bin/harness: $(srcdir)/harness.c .bin-dir-stamp
> -	$(CC) -o bin/harness $<
> +bin/harness: $(srcdir)/harness.c Makefile bin
> +	$(CC) -o $@ $<
>  
> -bin/check: $(srcdir)/check.sh .bin-dir-stamp
> -	cp $< bin/check
> -	chmod +x bin/check
> +bin/check: $(srcdir)/check.sh Makefile bin
> +	cp $< $@
> +	chmod +x $@
Why are those changes in this patch? Same as with PHONY above.

> -init.sh: $(srcdir)/Makefile.in .bin-dir-stamp bin/not bin/check bin/harness $(SCRIPTS)
> +init.sh: Makefile bin/not bin/check bin/harness $(SCRIPTS)
>  	rm -f $@-t $@
>  	echo 'top_srcdir=$(top_srcdir)' >> $@-t
>  	echo 'abs_top_builddir=$(abs_top_builddir)' >> $@-t
>  	echo 'abs_top_srcdir=$(abs_top_builddir)' >> $@-t
> +	echo 'abs_srcdir=$(abs_srcdir)' >> $@-t
> +	echo 'abs_builddir=$(abs_builddir)' >> $@-t
>  	echo 'PATH=$$abs_top_builddir/test/bin:$$PATH' >> $@-t
> +	echo 'if test -z "$$INSTALLCHECK"; then' >> $@-t
> +	echo '  PATH=$$abs_top_builddir/test/sbin:$$PATH' >> $@-t
Why sbin? Looks unrelated and superfluous.

>  	LDLPATH="\$$abs_top_builddir/libdm"; \
>  	LDLPATH="$$LDLPATH:\$$abs_top_builddir/tools"; \
>  	LDLPATH="$$LDLPATH:\$$abs_top_builddir/daemons/dmeventd"; \
>  	LDLPATH="$$LDLPATH:\$$abs_top_builddir/daemons/dmeventd/plugins/lvm2"; \
>  	LDLPATH="$$LDLPATH:\$$abs_top_builddir/daemons/dmeventd/plugins/mirror"; \
>  	LDLPATH="$$LDLPATH:\$$abs_top_builddir/daemons/dmeventd/plugins/snapshot"; \
> -	echo "export LD_LIBRARY_PATH=\"$$LDLPATH\"" >> $@-t
> -	echo 'top_srcdir=$(top_srcdir)' >> $@-t
> -	echo 'abs_srcdir=$(abs_srcdir)' >> $@-t
> -	echo 'abs_builddir=$(abs_builddir)' >> $@-t
> +	echo "  export LD_LIBRARY_PATH=\"$$LDLPATH\"" >> $@-t
> +	echo 'fi' >> $@-t
>  	echo 'export PATH' >> $@-t
>  	echo 'export DM_UDEV_SYNCHRONISATION=$(dm_udev_synchronisation)' >> $@-t
>  	chmod a-w $@-t
Other than the sbin bit, this is the actual relevant change -- do not
override PATH and LD_LIBRARY_PATH for the tests if INSTALLCHECK is set
in the environment (see above wrt naming).

> @@ -95,16 +111,18 @@
>  
>  Makefile: $(srcdir)/Makefile.in $(top_builddir)/config.status
>  	cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
> +	echo 'Makefile rebuilt! Run again....' >&2
> +	exit 1

This does not look very canonic. It is probably also quite superfluous.
>From GNU make manual, section 3.7:
| To this end, after reading in all makefiles, make will consider each
| as a goal target and attempt to update it. If a makefile has a rule
| which says how to update it (found either in that very makefile or in
| another one) or if an implicit rule applies to it (see Using Implicit
| Rules), it will be updated if necessary. After all makefiles have been
| checked, if any have actually been changed, make starts with a clean
| slate and reads all the makefiles over again. (It will also attempt to
| update each of them over again, but normally this will not change them
| again, since they are already up to date.)

>  .bin-dir-stamp: lvm-wrapper
> -	rm -rf bin
> -	mkdir bin
> +	rm -rf sbin
> +	mkdir sbin
>  	for i in lvm $$(cat ../tools/.commands); do \
> -	  ln -s ../lvm-wrapper bin/$$i; \
> +	  ln -s ../lvm-wrapper sbin/$$i; \
>  	done
> -	ln -s "$(abs_top_builddir)/tools/dmsetup" bin/dmsetup
> -	ln -s "$(abs_top_builddir)/daemons/clvmd/clvmd" bin/clvmd
> -	ln -s "$(abs_top_builddir)/daemons/dmeventd/dmeventd" bin/dmeventd
> +	ln -s "$(abs_top_builddir)/tools/dmsetup" sbin/dmsetup
> +	ln -s "$(abs_top_builddir)/daemons/clvmd/clvmd" sbin/clvmd
> +	ln -s "$(abs_top_builddir)/daemons/dmeventd/dmeventd" sbin/dmeventd
Just bin -> sbin again.

> @@ -118,7 +136,7 @@
>  	mv $@-t $@
>  
>  clean:
> -	rm -rf init.sh lvm-wrapper bin .bin-dir-stamp
> +	rm -rf init.sh lvm-wrapper bin sbin .bin-dir-stamp
>  	if test "$(srcdir)" != . ; then rm -f $(subst $(srcdir)/, ,$(SCRIPTS)) lvm2app.sh ; fi
>  
>  distclean: clean

> diff -a -u -r1.9 t-000-basic.sh
> --- test/t-000-basic.sh	20 Apr 2010 15:19:36 -0000	1.9
> +++ test/t-000-basic.sh	21 May 2010 13:34:59 -0000
> @@ -18,7 +18,11 @@
>  lvm pvmove --version|sed -n "1s/.*: *\([0-9][^ ]*\) .*/\1/p" > actual
>  
>  # ensure they are the same
> -diff -u actual expected
> +if test -z "$INSTALLCHECK"; then
> +  diff -u actual expected
> +else
> +  should diff -u actual expected
> +fi
Makes sense.

Please separate out the actual relevant changes and submit again. I
think that can go in. I don't see the reasoning behind "sbin", but I am
not opposed to the other refactors -- but please submit them separately.

Thanks,
   Petr.