Allow constant modified time to support reproducibility #363
Conversation
Codecov Report
@@ Coverage Diff @@
## master #363 +/- ##
============================================
+ Coverage 68.13% 68.46% +0.33%
- Complexity 93 97 +4
============================================
Files 7 7
Lines 568 593 +25
Branches 78 82 +4
============================================
+ Hits 387 406 +19
- Misses 128 137 +9
+ Partials 53 50 -3
Continue to review full report at Codecov.
|
|
Thanks for the contribution, @michal-riha |
|
We could also support the SOURCE_DATE_EPOCH environment variable, that's the standard to set the build timestamp and that would be probably less intrusive. |
|
@ebourg you mean on top of the config or as another option? |
|
I was thinking about the variable as the only configuration mechanism. The developer sets the variable and all the build tools involved are supposed to handle it. That's how the reproducible builds work usually, the developer doesn't have to configure each tool specifically, it just works. |
|
@michal-riha what are your thoughts on that? would that also work for you? |
|
I don't mind supporting the env variable. However it doesn't seem very maven-like: everything about the project and how to build it should be in pom.xml. This will make the build result dependent on build environment. (From practical point of view, consider local builds - you'd probably need extra script to wrap the maven build and set the env variable before running it, which seems like a bit of pain). But the agrument about not having to configure the setting everywhere is also a good one. Maybe it is worth to support both? Anyway, I don't really follow any discussions about this topic, maybe all of this was already considered. If you think supporting only the env variable is the way to go, I can change it. |
|
If we do it the Maven way we should support the https://maven.apache.org/guides/mini/guide-reproducible-builds.html |
|
I agree, I'll change it to use the common property. |
|
Env (which should also work with ant) plus official maven property sounds like a good plan then. Thanks! |
| @@ -140,6 +140,11 @@ | |||
| <artifactId>xz</artifactId> | |||
| <version>1.9</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.apache.maven</groupId> | |||
| <artifactId>maven-archiver</artifactId> | |||
There was a problem hiding this comment.
This is only needed for the test case, right?
Any reason for not using <scope>test</scope> ?
There was a problem hiding this comment.
It's 'needed' to resolve the property - see OutputTimestampResolver#24, where MavenArchiver is created.
Date outputDate = new MavenArchiver().parseOutputTimestamp(paramValue);
But to be fair the functionality can be recreated, so if you don't want to add new dependency, it can be removed.
There was a problem hiding this comment.
Might be worth asking @hboutemy if the property could be exposed in the core Maven API.
| * @since 1.9 | ||
| */ | ||
| @Parameter(defaultValue = "${project.build.outputTimestamp}") | ||
| private String outputTimestamp; |
There was a problem hiding this comment.
If we parse project.build.outputTimestamp and SOURCE_DATE_EPOCH I don't think it's necessary to keep a mojo property, nobody is going to configure jdeb specifically if it's possible to do it at a higher level.
There was a problem hiding this comment.
Sorry I don't follow. I thought this is the way how to read the properties from pom.xml. Is there another way how to get the specified value?
There was a problem hiding this comment.
If you add the project to the mojo with:
@Component
private MavenProject project;
you can then call project.getProperties() to get the properties defined in the pom.xml file:
project.getProperties().get("project.build.outputTimestamp");
There was a problem hiding this comment.
see https://maven.apache.org/guides/mini/guide-reproducible-builds.html for user documentation
the property is defined globally, then available globally, and it is read by maven-archiver in archiver.configureReproducible( outputTimestamp );
see for example how maven-jar plugin has integrated the feature: https://github.com/apache/maven-jar-plugin/commit/64c5e6530b4712cd95501fffb2de6bb1a202cd89/
There was a problem hiding this comment.
Since this follows the example given above, I'd prefer to leave it as is.
|
notice: one way to discover how reproducible builds is expected to be used by Maven builds is to make jdeb build reproducible, applying https://maven.apache.org/guides/mini/guide-reproducible-builds.html |
|
@hboutemy Thank you, I was actually wondering if it was possible to have a method somewhere in the core API (in MavenProject maybe?) that provides the reproducible timestamp to be used by the plugins. For example a |
|
@ebourg reproducible build is a bout plugins, not core, which permits to have reproducible builds whatever Maven core version you use, even Maven 2 |
|
Maybe there is a common plugin dependency that could expose such a method if it's not in the core API then? It's a bit suboptimal and error prone to let every plugin reimplement the same logic to compute the output timestamp. |
|
it's available as |
|
Yes but it adds a dependency that wasn't otherwise necessary for this plugin. Maybe it doesn't matter after all since Maven always pulls this dependency. |
|
in fact, sharing code will require a dependency |
|
So - what do we do about the dependency? IIUC I see 3 options:
Unless the code is trivial 3. might give is us the best of 1) and 2). Just might need some testing. |
|
Let's just add the dependency, maven-archiver is already pulled by the maven-jar-plugin anyway. |
michal-riha
force-pushed
the
allow-specific-modified-time
branch
from
98af964
to
aa1839f
Compare
|
Hi, |
Sorry, I was semi-offline the past few days. If I find time I might try inlining the deb. Thanks for your contribution! |
Hi,
This change allows users to specify 'modified time' that will be used for all files/folders. This is required to achieve reproducible builds (https://reproducible-builds.org/) of debian packages using this plugin, since there isn't another way how to control the time.
It adds new parameter 'modifiedTimeMs' to the plugin configuration, which (if not left empty) should contain epoch time in milliseconds that will be used as 'modified time'.
Looking forward to hearing from you