-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Updating build to be reproducible #774
base: master
Are you sure you want to change the base?
Conversation
pom.xml
Outdated
<plugin> | ||
<groupId>org.apache.felix</groupId> | ||
<artifactId>maven-bundle-plugin</artifactId> | ||
<version>3.3.0</version> | ||
<version>5.1.8</version> |
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.
IIRC, this will fail CI on the older JVMs
PR is marked as draft as we likely want to wait a bit on this one
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 know this is probably counter to reproducibility, but you can set different versions for different JVMs in the <profiles>
section at the bottom as a version property. Or have a different plugin configuration entirely. Just a thought.
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.
That could work 🤔
I've been wanting to try out a different idea too, building with a newer JDK, and using the toolchain option in the Surefire plugin to test with 7.
https://github.com/actions/setup-java/#maven-options
But, I have a feeling that's more work than it's worth, given that support for 7 will be dropped. 🤷
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 dunno how much work you'd want to put in for JDK 7. We won't need it much longer.
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 updated using the profile technique used, it's not worth spending more time with 7 😄
@bdemers do you want to try this again given all that stuff has been merged to |
We might want to add a CI check in here 🤔 Maybe pick a version, like the min supported reproducible version, (zulu-8)? and add another job? |
Multiple builds (without changes) will produce the same output (e.g. the hash of the jars will be identical)
What do you mean run it twice? I'm not following 😅 |
Sorry about that! Basically this: mvn clean install && mvn clean verify artifact:compare This would double the run time, which I don't think is worth doing. We could run the above against a single target (potentially even skipping tests, to speed things up), I'm not sure we should worry about this at all in CI right now. It's something we could check periodically or at release time. @hboutemy do you have any thoughts/recommendations for projects on how to validate reproducibility via CI? |
@bdemers why do you need to run the tests twice? Isn't the goal to ensure that the artifacts are identical? So you just need to run |
I recommend NOT trying to validate reproduciblity via CI: there is no reason reproduciblity will change from commit to commit checking reproducibility by hand is sufficient, and even necessary because 2 builds on the same environment may give the same result, but not on 2 different environments (for example if the current directory is put in the output) then real checking requires ore than just normal CI |
@@ -678,6 +710,7 @@ | |||
<properties> | |||
<maven.jar.version>3.2.2</maven.jar.version> | |||
<maven.compiler.version>3.8.1</maven.compiler.version> | |||
<felix.version>3.5.1</felix.version> |
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.
3.5.1 is known to have some RB issues: why not upgrade to 5.1.9?
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.
We still support JDK 7, so that's probably it.
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 😭, when testing with Java 7 we use the older version, when testing with Java 8+ we use the latest.
We should probably use toolchains for testing with Java 7, but... it's getting dropped soon, so this is a temporary hack.
Thanks @hboutemy, great points! |
Multiple builds (without changes) will produce the same output (e.g. the hash of the jars will be identical)
Note
This can be tested by running: