-
Notifications
You must be signed in to change notification settings - Fork 937
configury: make build reproducible #3779
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
configury: make build reproducible #3779
Conversation
|
I believe you also need to do the |
|
i google'd btw, in |
|
There is no standard for it, but we really should add it to be consistent. PMIx doesn't have an equivalent for "ompi_info" yet - probably something we need to add now that we have all these plugins appearing. |
|
@bmwiedemann FYI. |
jsquyres
left a comment
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.
@ggouaillardet Minor quibble: can you add some text in the commit message citing what @bmwiedemann described in #3759? E.g., the reproducible builds web site, the examples for using SOURCE_DATE_EPOCH web site, ... etc.?
That will just help Future Gilles / Future Jeff / etc. remember what was done, and why.
Thanks!
use SOURCE_DATE_EPOCH environment variable if defined in order to make build reproducible, by forcing timestamps. See https://wiki.debian.org/ReproducibleBuilds/TimestampsProposal#Bash_.2F_POSIX_shell Thanks Bernhard M. Wiedemann for bringing this to our attention Fixes open-mpi#3759 Signed-off-by: Gilles Gouaillardet <[email protected]>
6e446e0 to
4feb89e
Compare
|
@jsquyres i made the requested changes. |
|
Cool, thanks. I think we need to wait on #3759, and then update this PR to match whatever the decision is there. |
| dnl be edited by hand!! | ||
| dnl | ||
| dnl Generated by $username at " . localtime(time) . " | ||
| dnl Generated by $username at " . localtime($ENV{SOURCE_DATE_EPOCH} || time) . " |
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.
I don't think that this does what you expect. My reading of https://wiki.debian.org/ReproducibleBuilds/TimestampsProposal#Bash_.2F_POSIX_shell is that it expects SOURCE_DATE_EPOCH to be of the form 'YYYY-MM-DD'. If you pass a string of the form 'YYYY-MM-DD' to localtime(), you get... weirdness:
$ perl -e 'print localtime("2010-10-10") . "\n"';
Wed Dec 31 16:33:30 1969
$ perl -e 'print localtime("2018-10-10") . "\n"';
Wed Dec 31 16:33:38 1969
Did you mean:
... . ($ENV{SOURCE_DATE_EPOCH} || localtime(time)) . ...
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.
no, it is a plain int. In the linked example, you can see ${SOURCE_DATE_EPOCH:-$(date +%s)}
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.
Ah, ok. So this comment on the review is probably moot. But my other comments are still relevant.
| # Copyright (c) 2017 Research Organization for Information Science | ||
| # and Technology (RIST). All rights reserved. | ||
|
|
||
| date --date="@${SOURCE_DATE_EPOCH:-$(date +%s)}" |
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.
It might be good to put a comment here in this file explaining why it exists. Otherwise, Future People will wonder why there's a 1-line script to run the date command.
It might actually be good to put a 1-line comment in most/all places we do $SOURCE_DATE_EPOCH, referring to reproducible builds, just so that no on accidentally removes the $SOURCE_DATE_EPOCH because they don't understand what it's for.
|
@ggouaillardet Ping. I'm reminded of this PR (which has a few things that need to be fixed) because of #5653. |
|
@ggouaillardet Re-ping -- just merged #5653, so this PR now has some conflicts. |
|
Can one of the admins verify this patch? |
|
@bmwiedemann updated / refreshed this PR in #8136. Closing this PR in favor of the newer / fresher #8136. |
use SOURCE_DATE_EPOCH environment variable if defined
in order to make build reproducible
Fixes #3759
Signed-off-by: Gilles Gouaillardet [email protected]