-
Notifications
You must be signed in to change notification settings - Fork 194
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
Write Properties files in a reproducible way #4583
Conversation
Test Results 606 files ±0 606 suites ±0 4h 21m 55s ⏱️ + 4m 38s For more details on these errors, see this check. Results for commit 74067e9. ± Comparison against base commit e052363. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
...quinox.security/src/org/eclipse/equinox/internal/security/storage/SecurePreferencesRoot.java
Outdated
Show resolved
Hide resolved
...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java
Outdated
Show resolved
Hide resolved
tycho-api/src/main/java/org/eclipse/tycho/ReproducibleUtils.java
Outdated
Show resolved
Hide resolved
Last time I followed a discussion with the maven folks, they said that newlines are always a problem for reproducible build and one should rebuild on the same system architecture, so maybe not a big deal but probably also don't harm much. |
That's true, but when maven-archiver fixes the pom.properties file (in https://github.com/apache/maven-archiver/blob/master/src/main/java/org/apache/maven/shared/archiver/PomPropertiesUtil.java) it also uses a system-independent newline character. Anyway, trying to reuse the existing newline characters would make the code more complex, so it's simpler to use a system-independent newline character here. It would be dangerous to try to blindly fix all the text files to use a system-independent newline character, but when we need to rewrite a file it makes sens to take the opportunity to do it. |
29f63ab
to
51a7c6c
Compare
Hi @laeubi, I took into account all your remarks. Thanks. |
tycho-api/src/main/java/org/eclipse/tycho/ReproducibleUtils.java
Outdated
Show resolved
Hide resolved
51a7c6c
to
a6e837a
Compare
tycho-api/src/main/java/org/eclipse/tycho/ReproducibleUtils.java
Outdated
Show resolved
Hide resolved
Perhaps the same conclusion as in eclipse-equinox/p2#589 (comment) applies here, i.e., with Java 21 the properties are sorted and you can set a fixed reproducible timestamp via |
Using a system property to set the date is quite messy, but it's an interesting workaround when tycho calls external tools like p2. It's a pity that this feature was only added in Java 18. |
@Zlika can you adjust the util method to use |
a6e837a
to
92e4fbc
Compare
Done. |
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.
Just a few adjustments and I think we are done!
tycho-api/src/main/java/org/eclipse/tycho/ReproducibleUtils.java
Outdated
Show resolved
Hide resolved
92e4fbc
to
74067e9
Compare
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 all looks fine and consistent to me now.
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Java does not write java.util.Properties files in a reproducible way: there is a timestamp, the order of the lines is not reproducible, and the new line character used is system-dependent.
This PR fixes this problem by removing the timestamp, ordering the lines in the alphabetical order and using a system-independent new line character.
This is a follow-up of #4546 and #4569.