Cactus ignors (since r4839 ie #768) changes to param.ccl when recompiling

[Roland Haas]

Christian Reisswig (and I and Christian Ott and Philipp Moesta, but we did not properly identify it as a bug) found that adding or removing a parameter does not trigger a recompile of source files where the parameter was used. To reproduce, take a compiled configuration and eg. comment out a parameter in (any) thorn's param.ccl. Recompile and notice that no source files are recompiled (but configs/<CONFIG>/bindings/Parameters/<Thorn>_Parameters.c is). This manifests itself in wrong parameter values in the thorns (but correct values as returned by CCTK_ParameterGet).

I checked and the file containing the updated parameter structure (configs/<CONFIGNAME>/bindings/include/ParameterCPrivate<THORNNAME>.h) '''does''' appear in the source file's auto-dependency file (configs/<CONFIGNAME>/build/<THORNNAME>/<SOURCEFILE>.c.d) but that files seems to be completely ignored (ie. touch'ing a file listed in there does not trigger a recompile).

I am making this a critical bug since it affects every single thorn.

I attach a trivial thorn to display the issue but any configuration you have sitting around will do.

Keyword:

[Roland Haas]

The problem seem to be that the ".d" file has the wrong file names for the target in it. Eg. it will contain "source.o" rather than "source.c.o".

[Roland Haas]

the attached patch fixes the issue for me. The underlying cause was that the dependency fixer tried to remove files that match $(DEP_EXCLUDE_FILES), however (at least with Steve's patch) this variable was empty which causes perl to construct a pattern "s:.*/::g" (more or less) which causes compile aborts which in turn seems to have prompted removal of the fixer from the makefile assuming that if there are no files to remove then the fixer is not required. This is almost true with the exception that the previous perl line changes file names.

[Erik Schnetter]

If DEP_EXCLUDE_FILES is empty, then we should consider removing the functionality, instead of only wrapping it in an if statement.

I suggest the following dependency fixer, which is (a) simpler, (b) easier to understand, and (c) better commented:

1. Replace the dependency target (i.e. the LHS of the make rule) since it has the wrong name $(PERL) -pi -e 's{^[^:]+:}{$(basename $@).o $(basename $@).d:}' $@

[Roland Haas]

Fine with me. In fact I would then suggest to remove any mention of this variable from lib/make/make.config.rules.in. The comment might also state that in fact we add an extra target, namely the .d file:

1. Replace and enlarge the dependency targets (i.e. the LHS of the make rule) since it has the wrong name and does no list the auto generated dependency file $(PERL) -pi -e 's{^[^:]+:}{$(basename $@).o $(basename $@).d:}' $@

Since this affects the flesh, there are two "Please applies" required, yes?

[Erik Schnetter]

I support the suggestion described in comment 5.

[Erik Schnetter]

The regexp suggested in comments 4 and 5 does not work as-is. The dependency files contain several dependencies, and only one of them (the first) should be modified. Currently, Cactus looks for ": " (colon-space) to ensure this. Another way may be to look for a file name ending in ".o" before a colon. The correct way would be to replace the file name, which we know in principle -- maybe the expression

^$(basename $(basename $@))\.o: would work as text that should be replaced?

Since this is currently a serious bug in Cactus, we don't need approval of two maintainers. Anything that stands a fighting chance of correcting this (without introducing an even larger disaster) should be applied.

[Roland Haas]

The regexp was the one that was in Cactus before the change that introduced the bug (at that point the whole PERL call was simply commented out.) Looking at the auto-generated dependency files (which are the only ones to whom the fixer is applied I hope), I only see single targets (ie. nothing matches `m/ .*:/`. On the other hand, making the pattern more specific is no a bad idea either. I would however suggest adding perl escapes to prevent it from interpreting parts of the file name as regexp metacharacters ie.

$(PERL) -pi -e 's{^\Q$(basename $(basename $@)).o:\E}{$(basename $@).o $(basename $@).d:} if(m/: /)' $@ Where I added `\Q` and `\E` and replaced double quotes with single quotes to prevent the shell eating up the backslashes (not sure if that is required).

Ok to apply?

[Roland Haas]

Re-reading commit comment6: was this about there not being an `if m/: /` anymore? I'd have of course added it again?

[Erik Schnetter]

The m{: } (matching colon-space) is there to ensure that only the first rule matches, which is the one that happens to have the .o file as target. Since we now explicitly match the .o file name, we don't need this if condition any more.

To avoid misunderstandings, this is the new definition that I propose:

1. Define how to do dependencies.
2. Correct the dependency target (i.e. the item on the LHS of a make
3. rule). The name of the object file is wrong, and the dependency file
4. (.d file) is missing. To simplify things, we don't try to match the
5. object file name -- we assume that any file ending in .o is the
6. object file. ifeq ($(strip $(PERL_BACKUP_NECESSARY)),) define DEPENDENCY_FIXER $(PERL) -pi -e 's{^\Q$(basename $(basename $@)).o:\E}{$(basename $@).o $(basename $@).d:}' $@ endef else define DEPENDENCY_FIXER $(PERL) -pi.bak -e 's{^\Q$(basename $(basename $@)).o:\E}{$(basename $@).o $(basename $@).d:}' $@ rm $@.bak endef endif

I also suggest to add a new line to "force-reconfigure" to ensure that these changes are immediately picked up:

15 Jul 2012: Correct error in determining make dependencies

[Erik Schnetter]

I have applied this patch.

Roland, could you test it to ensure I didn't miss something?

[Roland Haas]

The applied patch works for me in generating correct dependency files and in forcing me to reconfigure. Unfortunately reconfiguring does not force a -cleandeps so that the old faulty dependency files are left behind even after a -reconfig and the user has to manually trigger a -cleandeps.

To remedy I propose to add the -cleandeps target to -reconfig since reconfiguring might change dependency relationships. Attached please find a patch.

[Erik Schnetter]

Forcing reconfiguring occurs so rarely that regenerating dependencies makes sense. (In fact, it may even make sense to force a realclean.)

Please apply.

[Roland Haas]

Applied as rev 4843 of the flesh.

[Roland Haas]

the patch in comment 14 causes the sequence:

make sim-realclean make sim-reconfig to fail with an error

Deleting all dependency files in Cactus/configs/sim find: `Cactus/configs/sim/build': No such file or directory make[2]: * [cleandeps] Error 1 make[1]: * [sim-cleandeps] Error 2 Use make sim to build the configuration.

Since the implied -cleandeps (in -reconfig) appantly does not guard against a clean configuration.

[Erik Schnetter]

Instead of testing trying to find out whether find will succeed, I suggest to allow find to fail instead, and not abort when such a failure is encountered. This can be implemented in two ways:

1. Add a minus sign ("-") in front of the find command (that's a make feature) 2. Add "|| true" after the find command

This is more elegant, since one does not need to try to find out ahead of time what could make find fail. It is also faster, since the "-d" test is executed only once (by find) (but being faster is irrelevant here).

[Roland Haas]

I actually thought about this possibility as well and decided against it since it also hides all other errors that find might encounter (such as it for some reason not being able to delete files) and that we might want to report to the user. The "-d" test, tests for one particular circumstance where we know that nothing needs to be done. Should I still apply the "-" or "|| true" option?

[Erik Schnetter]

Please apply either of the three.

[Roland Haas]

applied the original patch as revision 4853 of the flesh.