r.mapcalc: Add explicit dependency on mapcalc.tab.h#3415
Conversation
bmwiedemann
force-pushed
the
mapcaldeps
branch
from
aed19e5 to
60cffc6
Compare
neteler
changed the title
|
Thanks! I think I found one of these missing dependencies in issue #3406, but I didn't have the knowledge to finish debugging and writing a fix. I know using gnu make 4.4 with |
bmwiedemann
force-pushed
the
mapcaldeps
branch
from
60cffc6 to
511d122
Compare
|
For debugging I ran In the -j1 version you can see that such appear and trigger rebuilds of r.mapcalc and r3.mapcalc . But there might still be other issues left such as random missing .png files: |
I think you are describing another bug we caught earlier this year, but wasn't pinned down. I'm sure the solution will be through a makefile fix |
|
Just for personal curiosity, is adding an extra file the best way to define extra dependencies in targets? Is the file copied out to the installed directories or included in the bindist? Other than that, I would approve if I was confident enough with that level of Makefile, which isn't the case. Is anyone one else more knowledgeable for this? |
As we moved it to OBJ.$ARCH the |
|
Ok I see. Then who has the knowledge to approve and merge this PR? |
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
Given @bmwiedemann's activity elsewhere, I feel like I can confidently approve this contribution even with my low confidence in Makefiles.
The only heads up I see is that touch fails on systems with no access to touch which is apparently some HPC systems. However, we are using touch already, so no change there.
neteler
left a comment
neteler
left a comment
There was a problem hiding this comment.
Thanks for your work on reproducible builds of GRASS GIS!
|
I tagged it as "backport to 8.2" but we will not release 8.2.x any more - however, this PR might go into the upcoming 8.3.2 release (https://github.com/OSGeo/grass/milestone/24). |
|
What's the relation with using an extra file vs |
to tell make that both .c and .h are needed This allows for reproducible builds. Without this change, r.mapcalc and r3.mapcalc would be rebuilt a 2nd time in a make -j1 run with slightly varied order of mapcalc.yy.o and mapcalc.tab.o This might help improve OSGeo#3406 This patch was done while working on reproducible builds for openSUSE.
bmwiedemann
force-pushed
the
mapcaldeps
branch
from
511d122 to
bc7ec6e
Compare
|
I experimented some more and found a way to make it do the right thing without the extra .META file and with less overall changes. |
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
Thanks! This one is indeed much nicer!
|
@neteler Would you want to merge this new revision? |
That looks like #3038. I have not been able to reproduce either with |
Add explicit dependency on mapcalc.tab.c to tell make that both .c and .h are needed This allows for reproducible builds. Without this change, r.mapcalc and r3.mapcalc would be rebuilt a 2nd time in a make -j1 run with slightly varied order of mapcalc.yy.o and mapcalc.tab.o This might help improve #3406 This patch was done while working on reproducible builds for openSUSE.
Add explicit dependency on mapcalc.tab.c to tell make that both .c and .h are needed This allows for reproducible builds. Without this change, r.mapcalc and r3.mapcalc would be rebuilt a 2nd time in a make -j1 run with slightly varied order of mapcalc.yy.o and mapcalc.tab.o This might help improve #3406 This patch was done while working on reproducible builds for openSUSE.
|
Backported to
|
| $(OBJDIR)/*.o: mapcalc.tab.h mapcalc.tab.c | ||
| $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c | ||
|
|
There was a problem hiding this comment.
| $(OBJDIR)/*.o: mapcalc.tab.h mapcalc.tab.c | |
| $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c | |
| $(OBJDIR)/*.o $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c |
While it bothers me not to be able to reproduce this issue (and to full depth understand what's going on), I think this solution is good. If my suggested simplification works I'd personally prefer to have this in one line (as they are logically interconnected). Or even better, would the following solve the issue
$(OBJDIR)/*.o: mapcalc.tab.h mapcalc.tab.c
There was a problem hiding this comment.
@nilason I'm sorry, I just merged it (didn't understand that you wanted to keep it open and modify further). So, a new PR will be needed...
There was a problem hiding this comment.
@nilason I'm sorry, I just merged it (didn't understand that you wanted to keep it open and modify further). So, a new PR will be needed...
No worries! The one line solution I suggested should work (as it is basically the same). I can't test the latter suggestion (just adding mapcalc.tab.c as a pre-requisite to $(OBJDIR)/*.o targets) for solving the issue at hand, perhaps @bmwiedemann can chip in. The end-effect should be the same, but the code more concise.
There was a problem hiding this comment.
$(OBJDIR)/*.o: mapcalc.tab.h mapcalc.tab.c alone did not work - my guess is that the wildcard only matches existing files. Combining both in one line should work - I'm giving it a test-run now and should have results in an hour.
There was a problem hiding this comment.
$(OBJDIR)/*.o: mapcalc.tab.h mapcalc.tab.calone did not work - my guess is that the wildcard only matches existing files. Combining both in one line should work - I'm giving it a test-run now and should have results in an hour.
Thanks for testing and for your contribution!
There was a problem hiding this comment.
$(OBJDIR)/*.o $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c worked fine.
There was a problem hiding this comment.
$(OBJDIR)/*.o $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.cworked fine.
Thought so, thanks! I'll put up a PR, simplifying this.
Would you have been able to reproduce this issue you reported by only (re-)building in this module (directory)? E.g.:
cd raster/r.mapcalc
make clean
make -j1
I'm asking because I try figure out my inability to reproduce.
There was a problem hiding this comment.
My tooling does 2 whole package builds from scratch via our spec file. Not that easy to adapt to test a partial build. My guess is that there are some cross-dependencies from outside that dir that caused the issue.
There was a problem hiding this comment.
My tooling does 2 whole package builds from scratch via our spec file. Not that easy to adapt to test a partial build. My guess is that there are some cross-dependencies from outside that dir that caused the issue.
I did take a look at your tools, I'll see if I can make use of it on the Mac (definitely won't work as is).
Studying your log file, I realised the default GNU Make is quite old (3.81), but trying with 4.4.1 didn't make any difference.
Also I happened to notice:
[ 31s] configure: WARNING: unrecognized options: --enable-socket, --with-curses, --with-motif, --with-python, --with-wxwidgets
the listed options may all be removed from configure.
Thanks for your feedback!
Add explicit dependency on mapcalc.tab.c to tell make that both .c and .h are needed This allows for reproducible builds. Without this change, r.mapcalc and r3.mapcalc would be rebuilt a 2nd time in a make -j1 run with slightly varied order of mapcalc.yy.o and mapcalc.tab.o This might help improve OSGeo#3406 This patch was done while working on reproducible builds for openSUSE.
5 participants
Add explicit dependency on
mapcalc.tab.hand use a .META file to tell make that
both .c and .h are created by one call
to allow for reproducible builds.
Without this change,
r.mapcalcandr3.mapcalcwould be rebuilt a 2nd time in a make -j1 run
with a slightly changed order of
mapcalc.yy.oandmapcalc.tab.oThis patch was done while working on reproducible builds for openSUSE.