[libvirt] Re: [PATCH] virsh edit (v3)

[Jim Meyering]

"Richard W.M. Jones" <rjones at redhat.com> wrote:
> A new patch is attached which should address everything that you
> mentioned.

That was quick ;-)

> Index: src/Makefile.am
> ===================================================================
> RCS file: /data/cvs/libvirt/src/Makefile.am,v
> retrieving revision 1.86
> diff -u -r1.86 Makefile.am
> --- src/Makefile.am	11 Jul 2008 16:23:36 -0000	1.86
> +++ src/Makefile.am	1 Aug 2008 10:33:22 -0000
> @@ -138,6 +138,31 @@
>  virsh_DEPENDENCIES = $(DEPS)
>  virsh_LDADD = $(LDADDS) $(VIRSH_LIBS)
>  virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS)
> +BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c
> +EXTRA_DIST += virsh-net-edit.c virsh-pool-edit.c
> +
> +virsh-net-edit.c: virsh.c Makefile.am
> +	echo '/* Automatically generated from the Makefile and virsh.c */' > $@
> +	echo 'static int' >> $@
> +	awk '/^cmdEdit/, /^}/' $< | \
> +	sed -e 's/domain/network/g' \
> +	    -e 's/Domain/Network/g' \
> +	    -e 's/cmdEdit/cmdNetworkEdit/g' \
> +	    -e 's/dom/network/g' \
> +	>> $@

Don't redirect directly to $@.
Otherwise, if something goes wrong before the output is completely written,
you're left with a corrupt input that has an up-to-date time stamp.
That can be a pain to diagnose.

Also, if you make each generated file read-only, then even those
who don't spot the Auto-Generated-from comment at the top will have
a hard time accidentally changing it.

Also, you can include dependent names in the comment with "$^":

I indented the "sed" command so it's clearer that it's
on a backslash-continued line:

virsh-net-edit.c: virsh.c Makefile.am
	rm -f $@-t $@
	echo '/* Automatically generated from: $^ */' > $@-t
	echo 'static int' >> $@-t
	awk '/^cmdEdit/, /^}/' $<		\
	  | sed -e 's/domain/network/g'		\
	     -e 's/Domain/Network/g'		\
	     -e 's/cmdEdit/cmdNetworkEdit/g'	\
	     -e 's/dom/network/g'		\
	  >> $@-t
	chmod a-w $@-t
	mv $@-t $@

This is longer than the original rule, but it's for a good cause:
the point is to better protect readers/builders/developers.
We incur the cost of writing Makefile.am once, but the audience
will be saved some small wasted effort over and over ;-)

Yes, there are many other rules that can benefit from the same
treatment.  have to start somewhere.