make --question fails

[mdavidsaver]

With a template created by makeBaseApp, attempting to run make in "question" mode gives an error under configure/ which prevents this feature from being useful. The error message is a bit mystifying.

[mdavidsaver (mdavidsaver)]

This is something to do with RELEASE checking. If I can successfully run

> make CHECK_RELEASE=NO CHECK_RELEASE_NO= -q

The "CHECK_RELEASE_NO=" is necessary, which avoids unconditional execution of an "@echo". So to support --question may require avoiding any unconditional execution (including genVersionHeader.pl).

Do we want to support --question?

As background. FRIB would like to use this in combination with Puppet to decide if an IOC process should be rebuilt/restarted.

[Andrew Johnson (anj)]

This change avoids having to override CHECK_RELEASE_NO:

=== modified file 'configure/RULES_BUILD'
--- configure/RULES_BUILD 2017-01-04 22:32:13 +0000
+++ configure/RULES_BUILD 2017-03-03 21:34:39 +0000
@@ -176,7 +176,7 @@
        -$(CONVERTRELEASE) checkRelease
 noCheckRelease:
 ifeq ($(EPICS_HOST_ARCH),$(T_A))
- @echo "Warning: RELEASE file consistency checks have been disabled"
+ $(info Warning: RELEASE file consistency checks have been disabled)
 endif

 #---------------------------------------------------------------

tux% make -sq CHECK_RELEASE=NO
RELEASE file consistency checks have been disabled
tux% make -sq
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_ARCHS:61: recipe for target 'install.linux-x86_64' failed
make: *** [install.linux-x86_64] Error 1

However it looks like 'make -q' gives false negatives; when I touched a file in my *App/src directory running 'make -q' only failed when I ran it inside the O.<host> directory. That's the recursive make thing I was talking about, I don't think -q works in our setup.

[mdavidsaver (mdavidsaver)]

> $(info

Ah, a new trick.

> when I touched a file in my *App/src directory

I made the same test with a .c file, which did trigger a re-build. This is with Debian 8, which has make 4.0.

[Andrew Johnson (anj)]

tux% make --version
GNU Make 4.1
Built for x86_64-unknown-linux-gnu
Copyright (C) 1988-2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Hmm, my bad, it does work, I probably wasn't careful enough before:

tux% make -sj
tux% make -sq CHECK_RELEASE=NO
Warning: RELEASE file consistency checks have been disabled
tux% touch testApp/src/testMain.cpp
tux% make -sq CHECK_RELEASE=NO
Warning: RELEASE file consistency checks have been disabled
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_ARCHS:61: recipe for target 'install.linux-x86_64' failed
make[2]: *** [install.linux-x86_64] Error 1
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'src.install' failed
make[1]: *** [src.install] Error 2
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'testApp.install' failed
make: *** [testApp.install] Error 2

tux% make -sj
tux% touch configure/RELEASE
tux% make -sq CHECK_RELEASE=NO
Warning: RELEASE file consistency checks have been disabled
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'iocppc32.install' failed
make[1]: *** [iocppc32.install] Error 1
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'iocBoot.install' failed
make: *** [iocBoot.install] Error 2

tux% make -sj
tux% make -sq CHECK_RELEASE=NO
Warning: RELEASE file consistency checks have been disabled
tux% touch testApp/Db/user.substitutions
tux% make -sq CHECK_RELEASE=NO
Warning: RELEASE file consistency checks have been disabled
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_ARCHS:61: recipe for target 'install.linux-x86_64' failed
make[2]: *** [install.linux-x86_64] Error 1
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'Db.install' failed
make[1]: *** [Db.install] Error 2
/home/phoebus/ANJ/epics/base/3-14-dev/configure/RULES_DIRS:83: recipe for target 'testApp.install' failed
make: *** [testApp.install] Error 2

I will commit the $(info ..) change to the 3.14 branch.

This doesn't answer your question about genVersionHeader.pl though, there is a fundamental incompatibility there.

[mdavidsaver (mdavidsaver)]

> ... there is a fundamental incompatibility there.

It doesn't pay to underestimate gnumake. There are dusty corner with all kinds of tricks. Prefixing with '+' will cause a line to be executed even in question or dry-run modes. This is apparently how recursive make works.

> Using the MAKE variable has the same effect as using a ‘+’ character at the beginning of the recipe line.

In this case, something would need to be done to act appropriately if $(MAKEFLAGS) contains characters 'q' or 'n', 'B', and perhaps 't'.

[Andrew Johnson (anj)]

Thanks, I didn't know about the '+' prefix, although I am aware that gnumake has many hidden corners. We don't actually need it here though.

It initially looked like the fix should be trivial: I changed the genVersionHeader.pl recipe to be skipped when $(MAKEFLAGS) contains 'q' by modifying the rule like this:

=== modified file 'configure/RULES_BUILD'
--- configure/RULES_BUILD 2017-02-01 17:57:04 +0000
+++ configure/RULES_BUILD 2017-03-06 21:43:43 +0000
@@ -363,7 +363,7 @@
 # Generate header with version number from VCS

 ifneq ($(GENVERSION),)
-$(COMMON_DIR)/$(GENVERSION): FORCE
+$(COMMON_DIR)/$(GENVERSION): $(if $(findstring q,$(MAKEFLAGS)),,FORCE)
        $(GENVERSIONHEADER) -t $(TOP) -N $(GENVERSIONMACRO) -V "$(GENVERSIONDEFAULT)" $@
 endif

However this doesn't work when you're building for more than one target architecture. Each extra target regenerates the header file, so the first target immediately goes out of date when the second target gets built due to the file time-stamps.

I can remove $(COMMON_DIR) from the path so separate header files get created in each O.<target> directory; any applications using this will have to adjust the dependency in their Makefile (see the template fix below). Note that this fix won't work if someone tries to install the generated file in $(TOP)/include though:

=== modified file 'configure/RULES_BUILD'
--- configure/RULES_BUILD 2017-02-01 17:57:04 +0000
+++ configure/RULES_BUILD 2017-03-06 22:28:49 +0000
@@ -363,7 +363,7 @@
 # Generate header with version number from VCS

 ifneq ($(GENVERSION),)
-$(COMMON_DIR)/$(GENVERSION): FORCE
+$(GENVERSION): $(if $(findstring q,$(MAKEFLAGS)),,FORCE)
        $(GENVERSIONHEADER) -t $(TOP) -N $(GENVERSIONMACRO) -V "$(GENVERSIONDEFAULT)" $@
 endif

=== modified file 'src/template/base/top/exampleApp/src/Makefile'
--- src/template/base/top/exampleApp/src/Makefile 2017-02-01 17:57:04 +0000
+++ src/template/base/top/exampleApp/src/Makefile 2017-03-06 22:35:48 +0000
@@ -85,4 +85,4 @@
 # ADD EXTRA GNUMAKE RULES BELOW HERE

 # Explicit dependency needed for generated header file
-dev_APPNAME_Version$(DEP): $(COMMON_DIR)/$(GENVERSION)
+dev_APPNAME_Version$(DEP): $(GENVERSION)

'make -q' still failed when configured to do MinGW cross-builds, but I eventually worked out how to fix that (more obscurity!) and I just committed that change to the 3.15 branch where the problem originated.

If you're OK with the above I will commit those changes after merging the other commits up from 3.15.

[Ralph Lange (ralph-lange)]

Do we need documentation covering the change needed in dependency declarations for generated includes?

[Andrew Johnson (anj)]

Michael wrote documentation for the GENVERSION stuff which is in the 3.16 appDevGuide, but neither it nor the RELEASE_NOTES.html section say anything about where the generated header file gets created or whether it can be installed. Should I add this to the original RELEASE_NOTES.html section (which was included in the 3.16.0.1 release), or add a new section about the file location and dependency rule changes?

[mdavidsaver (mdavidsaver)]

> I changed the genVersionHeader.pl recipe to be skipped when $(MAKEFLAGS) contains 'q' by modifying the rule like this:

Well, this does "fix" the problem that '-q' always shows changes. However, it misses the possibility that the header should be regenerated due to changes to files not tracked by make deps.

I hate to say it, but genVersionHeader.pl needs to run unconditionally, and be taught about question and dry-run mode. Dry-run mode is easy, immediately 'exit 0'. Question mode would (I think) 'exit 1' just before the seek/truncate (where the mod. time would be updated).

[Andrew Johnson (anj)]

@michael: I just merged up the changes from 3.15 into 3.16.

I'm not sure whether the $(COMMON_DIR) stuff I talked about will need to stay in or not, and since you have a better idea than I do as to what needs to happen in genVersionHeader.pl I assigned the remaining fixes for 3.16 to you.

TBH I don't foresee too many users for the version header generation, and these complications just make me like the feature even less.

[Andrew Johnson (anj)]

Did we fix this properly for the 3.16 branch? Not marking as "Fix Released" until I'm sure.

[Andrew Johnson (anj)]

Ping, can those who know about this answer the question about what still needs doing on this bug?

- Coredev meeting

[mdavidsaver (mdavidsaver)]

This still needs doing. I think I know what needs to be done (see #10), but I don't have support to do the work.

[Martin Konrad (info-martin-konrad)]

I have implemented the changes Michael suggested. See https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+ref/fix-make-question-mode. With these changes in place Andrew's $(COMMON_DIR) changes shouldn't be needed.

I'm having some problems testing this. I tried

mkdir testioc
cd testioc/
makeBaseApp.pl -t ioc foo
makeBaseApp.pl -t ioc -i -a linux-x86_64 -p foo foo
make --question
echo $? # should return !=0 (IOC needs to be compiled)
make
make --question
echo $? # should return 0 (nothing to do)

Unfortunately the 2nd "make --question" call still returns 1. Also note that genVersionHeader.pl doesn't seem to be called at all (not sure why). Am I missing something?

[Andrew Johnson (anj)]

You missed Michael's CHECK_RELEASE=NO argument (or you could edit testioc/configure/CONFIG_SITE and disable the release check there):

tux% make -sq CHECK_RELEASE=NO || echo Make needed
Make needed
tux% make -sj
tux% make -sq CHECK_RELEASE=NO || echo Make needed
Warning: RELEASE file consistency checks have been disabled
tux% make -sj clean
tux% make -sq CHECK_RELEASE=NO || echo Make needed
Make needed

You will need to test with the makeBaseApp.pl -t example to actually check your genVersionHeader script changes, it only gets called when fooApp/src/Makefile has GENVERSION macros in it.

[mdavidsaver (mdavidsaver)]

Will there be a merge request for ~info-martin-konrad/epics-base/+git/epics-base/+ref/fix-make-question-mode ?

I don't see that the dry run flag is being passed.

[Martin Konrad (info-martin-konrad)]

Thanks for your hints!

@anj: The plan was actually to make the Makefiles and genVersionHeader smart enough that users don't need to pass CHECK_RELEASE=NO.

@anj: You might be right about "-t ioc" vs. "-t example". That might get me going again :-)

I'll probably not find time to work on this next week but I hope I can finish this soon. @mdavidsaver: I'll open a merge proposal as soon as I have successfully tested it.

[Andrew Johnson (anj)]

If you prefix the checkRelease command with a + sign in its rule (like the warnRelease rule has a - sign prefix) it shouldn't get flagged as requiring the rebuild by make --question.

[Martin Konrad (info-martin-konrad)]

Merge proposal: https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+merge/354520

Please test.