Skip to content
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

WICKET-7090: add outputTimestamp property to enable reproducible builds #758

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

papegaaij
Copy link
Contributor

In addition to reprocible builds, this should also result in timestamps being set on files in jars.

In addition to reprocible builds, this should also result in timestamps
being set on files in jars.
@papegaaij papegaaij marked this pull request as draft January 4, 2024 10:22
@papegaaij
Copy link
Contributor Author

Ok, unfortunately, this does not seem to give the desired effect. The files in the jars still don't have timestamps.

@papegaaij papegaaij marked this pull request as ready for review January 4, 2024 10:58
@@ -24,7 +24,7 @@
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>wicket-auth-roles</artifactId>
<packaging>bundle</packaging>
<packaging>jar</packaging>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break the OSGi support ?!
CC @mattrpav

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, it shouldn't. A jar is an OSGi bundle, when it's manifest contains OSGi related information, but it's still a jar. The documentation for the maven-bundle-plugin even describes this setup as a valid solution: https://felix.apache.org/documentation/subprojects/apache-felix-maven-bundle-plugin-bnd.html#_adding_osgi_metadata_to_existing_projects_without_changing_the_packaging_type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, type should be ‘bundle’ to activate the bundle plugin when generating the jar.

ActiveMQ had changes to support reproducible builds and maintained the bundle plugin.

I recommend we should know exactly what is the miss or change needed to support reproducible builds, and not just change the project type to jar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattrpav the bundle plugin is activated by binding one of its goals to the maven execution phase (in this case manifest in process-classes). This will generate the MANIFEST.MF, which will then be used by the maven-jar-plugin. I can see the correct MANIFEST.MF in the produced jar.

The problem with the maven-bundle-plugin is that it generates jars without timestamps on its entries. This is valid (and considered reproducible), but very inconvenient. There is no way to change this when using packaging bundle. I think the produced jars are fine. They do contain the correct MANIFEST.MF. However, we do not use OSGi, so I have no way of testing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@papegaaij the bundle plugin can add any field to the MANIFEST.MF. What field are you looking for in the MANIFEST.MF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattrpav This change is to get correct timestamps on files (also called entries) in the jar files. The maven-bundle-plugin does not set these timestamps at all, causing them to revert to the jar-epoch (02-01-1980). The maven-jar-plugin honors project.build.outputTimestamp. If it also gives us reproducible builds, that's a bonus but not the reason for this change.

We do however not want to break OSGi support. However, none of the Wicket core developers uses OSGi, so this is very hard for us to test. With this change, the produced jars should still be valid OSGi bundles, but we have no good way to verify this. It be really great of you could check this.

As far as I know OSGi, the only thing that makes a normal jar an OSGi bundle is a set of entries in the MANIFEST.MF. These all seem unaffected by this change (I compared before and after), so I think the change is correct. But maybe you can point to something else that must be in place for an OSGi bundle?

@martin-g
Copy link
Member

martin-g commented Jan 4, 2024

In addition: one of the commits uses TKH-7090 instead of WICKET-7090

@papegaaij
Copy link
Contributor Author

In addition: one of the commits uses TKH-7090 instead of WICKET-7090

Doh, TKH is the prefix we use for Topicus KeyHub :) I've fixed the commit.

@@ -129,6 +129,8 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>

<project.build.outputTimestamp>2024-01-01T00:00:00Z</project.build.outputTimestamp>
Copy link
Contributor

@mattrpav mattrpav Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a static value seems difficult to manage. I suggest looking at using the git commit timestamp.

Benefits:

  1. Automatically changes
  2. Changes with every commit
  3. Ensures reproducible builds get the same timestamp when rebuilt from any commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maven-release-plugin updates this field during release .....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the maven-release-plugin should update this field automatically during a release. We do need to verify this at the next release though.

@papegaaij
Copy link
Contributor Author

@martin-g are you ok with merging this change?

Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Hopefully the change will be tested by an OSGi user during the vote for 10.0.0!
If there is a regression we can work on a fix in 10.0.x!

@papegaaij papegaaij merged commit 07d457b into master Jan 10, 2024
3 checks passed
@mattrpav
Copy link
Contributor

@martin-g @papegaaij I'll test the v10 release in OSGi and report any issues. Thanks.

@papegaaij
Copy link
Contributor Author

Thank you @mattrpav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants