[PATCH] build: only generate version.h when needed and remove it in clean target

[PATCH] build: only generate version.h when needed and remove it in clean target

[Uwe Kleine-König]

This way version.h isn't generated when running $(make clean) but only
when lib.c is about to be compiled.

This simplifies packaging for Debian because the package building programs
abort when there are additional files after $(make clean).
---
 Makefile | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index a4653aa1b747..13b0f97f146e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,12 +1,6 @@
 VERSION=0.5.1
 
-# Generating file version.h if current version has changed
 SPARSE_VERSION:=$(shell git describe 2>/dev/null || echo '$(VERSION)')
-VERSION_H := $(shell cat version.h 2>/dev/null)
-ifneq ($(lastword $(VERSION_H)),"$(SPARSE_VERSION)")
-$(info $(shell echo '     GEN      'version.h))
-$(shell echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h)
-endif
 
 OS = linux
 
@@ -215,6 +209,18 @@ pre-process.sc: CHECKER_FLAGS += -Wno-vla
 %.o: %.c $(LIB_H)
 	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
 
+version.h: FORCE
+	$(QUIET_GEN)echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h.tmp; \
+	if cmp -s version.h version.h.tmp; then \
+		rm version.h.tmp; \
+	else \
+		mv version.h.tmp version.h; \
+	fi
+
+.PHONY: FORCE
+
+lib.o: version.h
+
 %.sc: %.c sparse
 	$(QUIET_CHECK) $(CHECKER) $(CHECKER_FLAGS) -c $(ALL_CFLAGS) $<
 
@@ -223,7 +229,7 @@ selfcheck: $(ALL_OBJS:.o=.sc)
 
 
 clean: clean-check
-	rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc
+	rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc version.h
 
 dist:
 	@if test "$(SPARSE_VERSION)" != "v$(VERSION)" ; then \
-- 
2.14.1

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Christopher Li]

On Thu, Sep 14, 2017 at 2:45 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> -# Generating file version.h if current version has changed
>  SPARSE_VERSION:=$(shell git describe 2>/dev/null || echo '$(VERSION)')
> -VERSION_H := $(shell cat version.h 2>/dev/null)
> -ifneq ($(lastword $(VERSION_H)),"$(SPARSE_VERSION)")
> -$(info $(shell echo '     GEN      'version.h))
> -$(shell echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h)
> -endif

You don't need to move them to into a rule section.

You can use "ifneq ($(MAKECMDGOALS),clean)"
to wrap it. GNU make document even show that as examples
of using $(MAKECMDGOALS).


> +version.h: FORCE
> +       $(QUIET_GEN)echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h.tmp; \
> +       if cmp -s version.h version.h.tmp; then \
> +               rm version.h.tmp; \
> +       else \
> +               mv version.h.tmp version.h; \
> +       fi
> +
> +.PHONY: FORCE
> +
> +lib.o: version.h
> +

The above section is not needed if you use the ifneq test on $(MAKECMDGOALS).
I also test it and found the problem that, the version.h was force
to obsolete. Two consequent make will always show "GEN version.h"
line.

>  clean: clean-check
> -       rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc
> +       rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc version.h

This line is good.

If you don't mind I am going to apply this (the 3 liner version) after
my debug build patches.
It has conflict with my debug build patch and it is much easier to fix conflict
on this 3 liner patch than the other way around.

Chris

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Christopher Li]

On Thu, Sep 14, 2017 at 2:45 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> This way version.h isn't generated when running $(make clean) but only
> when lib.c is about to be compiled.
>
> This simplifies packaging for Debian because the package building programs
> abort when there are additional files after $(make clean).

BTW, this one needs a SOB as well. I want to apply the modify 3 liner
version using the $(MAKECMDGOALS).

Thanks

Chris

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1800 bytes --]

On 09/15/2017 06:52 PM, Christopher Li wrote:
> On Thu, Sep 14, 2017 at 2:45 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>>
>> -# Generating file version.h if current version has changed
>>  SPARSE_VERSION:=$(shell git describe 2>/dev/null || echo '$(VERSION)')
>> -VERSION_H := $(shell cat version.h 2>/dev/null)
>> -ifneq ($(lastword $(VERSION_H)),"$(SPARSE_VERSION)")
>> -$(info $(shell echo '     GEN      'version.h))
>> -$(shell echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h)
>> -endif
> 
> You don't need to move them to into a rule section.
> 
> You can use "ifneq ($(MAKECMDGOALS),clean)"

So it still triggers when doing

	make clean all

Also you don't want to generate it for $(make check).

IMHO that's hardly manageable to get done consistently this way and the
easiest is a separate rule for version.h that is triggered by make
dependencies as I suggested

> to wrap it. GNU make document even show that as examples
> of using $(MAKECMDGOALS).

IMHO that is no prove that the idea is sane.

>> +version.h: FORCE
>> +       $(QUIET_GEN)echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h.tmp; \
>> +       if cmp -s version.h version.h.tmp; then \
>> +               rm version.h.tmp; \
>> +       else \
>> +               mv version.h.tmp version.h; \
>> +       fi
>> +
>> +.PHONY: FORCE
>> +
>> +lib.o: version.h
>> +
> 
> The above section is not needed if you use the ifneq test on $(MAKECMDGOALS).> I also test it and found the problem that, the version.h was force
> to obsolete. Two consequent make will always show "GEN version.h"
> line.

Then maybe split it into

	CHECK version.h
	GEN   version.h

? The GEN would be skipped if version.h doesn't need an update.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Christopher Li]

On Tue, Sep 19, 2017 at 10:50 AM, Uwe Kleine-König
<uwe@kleine-koenig.org> wrote:
>> You can use "ifneq ($(MAKECMDGOALS),clean)"
>
> So it still triggers when doing
>
>         make clean all

That is the correct behavior. Because "all" require version.h.
It is a silly thing to put clean and all together, but the makefile
actually do the right thing.

>
> Also you don't want to generate it for $(make check).

check is depend on all which depend on version.h.
So that seems acceptable to me.

Plus in the make check case, it will only look at the version.h,
it will not regenerate it if data is the same.

> IMHO that's hardly manageable to get done consistently this way and the
> easiest is a separate rule for version.h that is triggered by make
> dependencies as I suggested

The problem is that, you end up updating the version.h from make's
point of view. Then make detect the version.h's time stamp haven't
change and take a short cut. That is the part I consider not clean.

In other words, if you only look at the make rules and ignore the
time stamp short cut. "version.h" will be force to update every
time. It should also recompile lib.o without the short cut.

>> to wrap it. GNU make document even show that as examples
>> of using $(MAKECMDGOALS).
>
> IMHO that is no prove that the idea is sane.

All the example you give seems give sane result.


>> The above section is not needed if you use the ifneq test on $(MAKECMDGOALS).> I also test it and found the problem that, the version.h was force
>> to obsolete. Two consequent make will always show "GEN version.h"
>> line.
>
> Then maybe split it into
>
>         CHECK version.h
>         GEN   version.h
>
> ? The GEN would be skipped if version.h doesn't need an update.

Then there is another thing I consider slightly unclean.
The check rules will optionally update another target "version.h" without
specificity in its rules.

We can use order only rules for check target. The whole thing seems
gets more complicated than it needs to.

Chris

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4873 bytes --]

On Tue, Sep 19, 2017 at 11:42:30AM -0400, Christopher Li wrote:
> On Tue, Sep 19, 2017 at 10:50 AM, Uwe Kleine-König
> <uwe@kleine-koenig.org> wrote:
> >> You can use "ifneq ($(MAKECMDGOALS),clean)"
> >
> > So it still triggers when doing
> >
> >         make clean all
> 
> That is the correct behavior. Because "all" require version.h.

After just adding version.h to clean (which might be a sensible separate
change?) but without your proposed change I get this:

	uwe@taurus:~/sparse$ make clean all
	find validation/ \( -name "*.c.output.expected" \
			 -o -name "*.c.output.got" \
			 -o -name "*.c.output.diff" \
			 -o -name "*.c.error.expected" \
			 -o -name "*.c.error.got" \
			 -o -name "*.c.error.diff" \
			 \) -exec rm {} \;
	rm -f *.[oa] .*.d *.so test-lexing test-parsing obfuscate compile graph sparse test-linearize example test-unssa test-dissect ctags c2xml test-inspect sparse-llvm libsparse.so pre-process.h sparse.pc version.h
	     CC       test-lexing.o
	     CC       target.o
	     CC       parse.o
	     CC       tokenize.o
	     CC       pre-process.o
	     CC       symbol.o
	     CC       lib.o
	lib.c:46:10: fatal error: version.h: No such file or directory
	 #include "version.h"
		  ^~~~~~~~~~~
	compilation terminated.
	Makefile:212: recipe for target 'lib.o' failed
	make: *** [lib.o] Error 1

I'd say this is not correct behaviour. The problem is that as is done now,
version.h is created while make parses the Makefile and doesn't really
know about the dependencies (yet). As a consequence make doesn't notice
that version.h is missing while compiling lib.c.

So here are needlessly two things mixed: parsing the Makefile and
generating version.h. It all gets easier if Makefile is parsed first
without caring about version.h as something special and then treat the
header just like every other target.

> It is a silly thing to put clean and all together, but the makefile
> actually do the right thing.

I'd say it's subjective if you consider that silly or not. I usually do
this if I don't trust the build system (e.g. because the build system
contains strange constructs involving MAKECMDGOALS :-)

> > Also you don't want to generate it for $(make check).
> 
> check is depend on all which depend on version.h.
> So that seems acceptable to me.

ok

> Plus in the make check case, it will only look at the version.h,
> it will not regenerate it if data is the same.

That's shared with my approach.

> > IMHO that's hardly manageable to get done consistently this way and the
> > easiest is a separate rule for version.h that is triggered by make
> > dependencies as I suggested
> 
> The problem is that, you end up updating the version.h from make's
> point of view. Then make detect the version.h's time stamp haven't
> change and take a short cut. That is the part I consider not clean.

I cannot follow here. lib.c depends on version.h and always when you
update version.h you want to recompile lib.c, don't you? Checking
timestamps is what make is good at (and there for), so I don't
understand your concern. And conditionally updating a target to keep the
old file if no update is needed is a usual approach.

> In other words, if you only look at the make rules and ignore the
> time stamp short cut. "version.h" will be force to update every
> time. It should also recompile lib.o without the short cut.

I'd not call it "time stamp short cut" but version.h being a target that
is only updated when necessary. Looks like plain make for me.

Another "problem" with the current approach is that version.h is
generated synchronously and so it is not parallelized as it should.
(IMHO doesn't matter much because build time is short enough, but IMHO
still a good hint that the construct is broken.)

> >> to wrap it. GNU make document even show that as examples
> >> of using $(MAKECMDGOALS).
> >
> > IMHO that is no prove that the idea is sane.
> 
> All the example you give seems give sane result.

Do you still think that after the failure reported above?
 
> >> The above section is not needed if you use the ifneq test on $(MAKECMDGOALS).> I also test it and found the problem that, the version.h was force
> >> to obsolete. Two consequent make will always show "GEN version.h"
> >> line.
> >
> > Then maybe split it into
> >
> >         CHECK version.h
> >         GEN   version.h
> >
> > ? The GEN would be skipped if version.h doesn't need an update.
> 
> Then there is another thing I consider slightly unclean.
> The check rules will optionally update another target "version.h" without
> specificity in its rules.

I don't understand that.

> We can use order only rules for check target. The whole thing seems
> gets more complicated than it needs to.

I don't follow.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Christopher Li]

On Tue, Sep 19, 2017 at 2:19 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>              CC       pre-process.o
>              CC       symbol.o
>              CC       lib.o
>         lib.c:46:10: fatal error: version.h: No such file or directory
>          #include "version.h"
>                   ^~~~~~~~~~~
>         compilation terminated.
>         Makefile:212: recipe for target 'lib.o' failed
>         make: *** [lib.o] Error 1
>
> I'd say this is not correct behaviour. The problem is that as is done now,
> version.h is created while make parses the Makefile and doesn't really
> know about the dependencies (yet). As a consequence make doesn't notice
> that version.h is missing while compiling lib.c.

No, that is not what is happening. What is happen is that, your "make clean all"
is incorrect command in the sense that, "clean" and "all" does not have
dependency and order between them. So it is just a race condition which get
to run first in the parallel build environment.

In your case, there is only one job. Clean runs first.

But if there is more than one job, even you specify the
lib.o depend on version.h. It is still not safe.
It can happen in the order of:

1) job1 "create version.h",
2) job2 "clean" remove version.h
3) job1 compile lib.o -> missing version.h

The root of the evil is that, your "make clean all" does not make
sense. It does not specify order between "clean" vs "all".
make can invoke it whichever order it see fit.
You get it working it is just because you are on the lucky path.

> So here are needlessly two things mixed: parsing the Makefile and
> generating version.h. It all gets easier if Makefile is parsed first
> without caring about version.h as something special and then treat the
> header just like every other target.

You should not mix clean with other target. Period.
Try to support that create a lot of complexity and corner cases.

>> Plus in the make check case, it will only look at the version.h,
>> it will not regenerate it if data is the same.
>
> That's shared with my approach.

it is slightly different in the sense that, the checking is all done
in GNU make function instead of invoking external shell. Granted, we
can use $(file ) function to remove the $(sell cat ) part.

It is slightly harder to do that inside the rule section.

Your patch will always update the version.h and force it to
run the command section. Which has a few shell commands.

>
> I cannot follow here. lib.c depends on version.h and always when you
> update version.h you want to recompile lib.c, don't you? Checking

Yes, of course.

> timestamps is what make is good at (and there for), so I don't
> understand your concern. And conditionally updating a target to keep the
> old file if no update is needed is a usual approach.

As I said, you need to use order only prerequisites to do it properly.
Otherwise it is kind of conflicting view version.h is updated or not.
From the rules point of view, the version.h is updated, the command
section gets executed. From the time stamp point of view, it is not.

>
> Another "problem" with the current approach is that version.h is
> generated synchronously and so it is not parallelized as it should.
> (IMHO doesn't matter much because build time is short enough, but IMHO
> still a good hint that the construct is broken.)

That is not a problem at all. I consider version.h at the bottom of
the dependency chain, so there is no paralleled when every body
depend on it.

The most common case, the version.h does not need to get
updated.

>
> Do you still think that after the failure reported above?

I think that failure is "user error". Should not try to mix "clean"
with other targets. Nothing good can come out of it.

>> Then there is another thing I consider slightly unclean.
>> The check rules will optionally update another target "version.h" without
>> specificity in its rules.
>
> I don't understand that.
>
>> We can use order only rules for check target. The whole thing seems
>> gets more complicated than it needs to.

The conflicting view is version.h gets updated or not.
I think the cleaner way to do it is some thing like:

check_version:
             <update version.h only if needed>

version.h: | check_version

Because I don't plan to support mixing clean vs
other targets. I think using $(MAKECMDGOALS)
is simpler and faster, no need to execute external
shells for the common case.

Chris

[PATCH] build: remove version.h in clean target

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

I think this is the inarguable part and that alone is still a
considerable improvement for me.

Best regards
Uwe

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index a4653aa1b747..d0341764158e 100644
--- a/Makefile
+++ b/Makefile
@@ -223,7 +223,7 @@ selfcheck: $(ALL_OBJS:.o=.sc)
 
 
 clean: clean-check
-	rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc
+	rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc version.h
 
 dist:
 	@if test "$(SPARSE_VERSION)" != "v$(VERSION)" ; then \
-- 
2.14.1

Re: [PATCH] build: only generate version.h when needed and remove it in clean target

[Luc Van Oostenryck]

On Thu, Sep 14, 2017 at 08:45:40PM +0200, Uwe Kleine-König wrote:
> This way version.h isn't generated when running $(make clean) but only
> when lib.c is about to be compiled.

Hi,

I would like to use in my development tree the following slightly updated version.
Would you mind to give your SoB?

Regards,
-- Luc Van Oostenryck

From bebb8ee6d8a793b50d77600ddcabec5f78cc9819 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <uwe@kleine-koenig.org>
Date: Thu, 14 Sep 2017 20:45:40 +0200
Subject: [PATCH] build: only generate version.h when needed and remove it in
 clean target

This way version.h isn't generated when running $(make clean) but only
when lib.c is about to be compiled.

This simplifies packaging for Debian because the package building programs
abort when there are additional files after $(make clean).

---
 Makefile | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index ee1334c42..1491accb0 100644
--- a/Makefile
+++ b/Makefile
@@ -1,12 +1,6 @@
 VERSION=0.5.1
 
-# Generating file version.h if current version has changed
 SPARSE_VERSION:=$(shell git describe 2>/dev/null || echo '$(VERSION)')
-VERSION_H := $(shell cat version.h 2>/dev/null)
-ifneq ($(lastword $(VERSION_H)),"$(SPARSE_VERSION)")
-$(info $(shell echo '     GEN      'version.h))
-$(shell echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h)
-endif
 
 OS = linux
 
@@ -204,6 +198,17 @@ CFLAGS += ${${*}_CFLAGS}
 selfcheck: $(OBJS:.o=.sc)
 
 
+lib.o: version.h
+version.h: FORCE
+	@echo '#define SPARSE_VERSION "$(SPARSE_VERSION)"' > version.h.tmp
+	@if cmp -s version.h version.h.tmp; then \
+		rm version.h.tmp; \
+	else \
+		echo    '     GEN      '$@; \
+		mv version.h.tmp version.h; \
+	fi
+
+
 clean: clean-check
 	rm -f *.[oa] .*.d *.so $(PROGRAMS) $(SLIB_FILE) pre-process.h sparse.pc version.h
 
@@ -225,3 +230,5 @@ clean-check:
 	                 -o -name "*.c.error.got" \
 	                 -o -name "*.c.error.diff" \
 	                 \) -exec rm {} \;
+
+.PHONY: FORCE
-- 
2.14.0