-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
r.mapcalc: Add explicit dependency on mapcalc.tab.h #3415
Conversation
aed19e5
to
60cffc6
Compare
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 |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too late :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(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.
Thanks for testing and for your contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(OBJDIR)/*.o $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c
worked fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(OBJDIR)/*.o $(OBJDIR)/mapcalc.yy.o: mapcalc.tab.h mapcalc.tab.c
worked 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Add explicit dependency on
mapcalc.tab.h
and 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.mapcalc
andr3.mapcalc
would be rebuilt a 2nd time in a make -j1 run
with a slightly changed order of
mapcalc.yy.o
andmapcalc.tab.o
This patch was done while working on reproducible builds for openSUSE.