-
Notifications
You must be signed in to change notification settings - Fork 3.5k
CI feature: Reproducible build #8701
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
CI feature: Reproducible build #8701
Conversation
I'm uploading it to see if it works with Travis. It did locally with this Bash script UPDATE: it worked in the first go! Looking forward to hearing your comments |
@eksperimental I like this a lot. I would just propose to run those changes at the end of the suite, this way we don't need to remove the trailing ebin_reproducible directories and we don't block the test results on this. However, in order to do that, we would need to run |
Would it make sense to move this into a script so you can run it locally? |
@fertapric I want to do that but currently there is no good placement for it as travis does not allow us to move the .travis.yml around. They do have an open issue and my plan is to move such scripts as well as the .yml files to a CI directory. We just need to wait a big longer. |
The travis.yml is already running shell commands like |
We wanted to avoid adding assorted scripts to the repository but I believe the answer is that we can simply add this to the makefile for now. :D |
@eksperimental another suggestion, that came up on #8702, is for us to have a specific entry in the test matrix that checks for reproducible long-term builds. In this matrix entry we would:
This way we keep those concerns isolated in the CI instead of running them on every OTP version. We can use the latest OTP version on this build. |
4a4b35f
to
7881cfa
Compare
Do we want to run tests in the future, or to compile as well? |
e4820bd
to
b256dc9
Compare
I ran It doesn't work. So I would skip it for now. |
3d4a069
to
ece2b72
Compare
@josevalim can you have a look at this and tell me what would be the cause of the error |
See elixir-lang#8689 for more information
bbac840
to
e1cf6eb
Compare
Alright, I 'm done with all the requests. Please have a look. The new command is |
e1cf6eb
to
db810db
Compare
Makefile
Outdated
$(Q) mv lib/iex/ebin/* lib/iex/ebin_reproducible/ | ||
$(Q) mv lib/logger/ebin/* lib/logger/ebin_reproducible/ | ||
$(Q) mv lib/mix/ebin/* lib/mix/ebin_reproducible/ | ||
SOURCE_DATE_EPOCH="$(call SOURCE_DATE_EPOCH)" $(MAKE) compile |
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.
do you want to do a call SOURCE_DATE_EPOCH
? If you export above, it is enough, no?
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 cannot make it work on Travis.
I had pushed a commit right before you mention this, exporting it.
if it works, i will see If I can avoid passing the variable explicitely
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.
Yeah, i saw the new commit, I think you have to remove the call here for the export to work. I just think though... :D not sure!
I am not sure why. If I had to guess, it changed the time for the VM but not for the filesystem operations. |
@eksperimental I pushed a commit where we can compute the epoch correctly but make is failing for some reason. |
ca2976c
to
d29239f
Compare
@josevalim I'm running ubuntu, but a later version than the one in CI, and this is working differently. exporting variables work locally, maybe it is the GNUMake version that Travis is running. The problem with your solution is that we need to determine SOURCE_DATE_EPOCH before moving the files from ebin to ebin_reproducible, and then use this value again when compiling the last time. As I'm writing this the new implementation seems to have worked. So, have a look please |
This is ready to be merged! |
Two minor comments and we should be good to go! |
b913411
to
8361c43
Compare
done! |
Amazing, thank you! ❤️ 💚 💙 💛 💜 |
/cc @bmwiedemann
Main issue: #8689
Related PR: #8694