-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fix reproducibility issues #187
base: develop
Are you sure you want to change the base?
Conversation
@mzf-guest, should we close this, or could you address my prior point? |
b808670
to
4f848cd
Compare
@prj- I've just updated the problematic file to remove the tab to white-space modification. I've also rebased to recent Thanks! |
Sorry, I just realised that my comment back from August 2021 was pending and that I did not submit my review... |
strversionnumber.cpp: $(libff_a_SOURCES2) ../../Makefile | ||
m4 -DVersionFreeFemDate="`date`" -DGitVersion="`git describe --tags 2>/dev/null|| echo 'no git'`" strversionnumber.m4 > $@ | ||
m4 -DVersionFreeFemDate="$(BUILD_DATE)" -DGitVersion="`git describe --tags 2>/dev/null|| echo 'no git'`" strversionnumber.m4 > $@ |
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.
This does not work on even on Debian.
$ make strversionnumber.cpp
m4 -DVersionFreeFemDate="""" -DGitVersion="`git describe --tags 2>/dev/null|| echo 'no git'`" strversionnumber.m4 > strversionnumber.cpp
Any suggestion @mzf-guest?
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.
Indeed! This code does not work when SOURCE_DATE_EPOCH
is not defined by the build chain. Good catch! and sorry the miss.
The Makefile example from this page seems to work better, at least in a minimal test. But I can't make it work from the Makefile.am file. When running autoreconf -i
, the ifeq
is recognized as a AM command instead of copying it verbatim in the Makefile.
DATE_FMT = +%Y-%m-%d
ifdef SOURCE_DATE_EPOCH
BUILD_DATE ?= $(shell date -u -d "@$(SOURCE_DATE_EPOCH)" "$(DATE_FMT)" 2>/dev/null || date -u -r "$(SOURCE_DATE_EPOCH)" "$(DATE_FMT)" 2>/dev/null || date -u "$(DATE_FMT)")
else
BUILD_DATE ?= $(shell date "$(DATE_FMT)")
endif
Unfortunately, I'm not very skilled with autotools. Do you have any hints?
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.
Ahah, no, I faced the exact same issue. Maybe this should be put a static makefile, like Makefile.date
. And then we'd replace date
by make -f Makefile.date date
with
$ cat Makefile.date
DATE_FMT = +%Y-%m-%d
ifdef SOURCE_DATE_EPOCH
BUILD_DATE ?= $(shell date -u -d "@$(SOURCE_DATE_EPOCH)" "$(DATE_FMT)" 2>/dev/null || date -u -r "$(SOURCE_DATE_EPOCH)" "$(DATE_FMT)" 2>/dev/null || date -u "$(DATE_FMT)")
else
BUILD_DATE ?= $(shell date "$(DATE_FMT)")
endif
date:
echo $(BUILD_DATE)
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.
Any update about this @mzf-guest?
Hello,
here the patch from the Debian package that fixes reproducibility issues:
date
by SOURCE_DATE_EPOCHls
for stable inputs(This pull request invalidates the previous one, based on master instead of develop. Sorry for the noise.)
Best,
François