[MARTIFACT-59] tolerate injected timestamp value

[rmannibucau]

No description provided.

[hboutemy]

issue moved to https://issues.apache.org/jira/browse/MARTIFACT-59

[hboutemy]

IIUC, it's about not displaying an ERROR message when build run as mvn -Dproject.build.outputTimestamp=xxx?
(and improving error message with full XML proposal including a value)

[rmannibucau]

Goal is really to enable the value to be a property from the project but not hardcoded.
Typically it is injected from a plugin in initialize phase (git commit id plugin for ex).

High level here the issue with current option:

• hardcoded in the pom breaks several use cases until you make it moving everytime you change something in the project (or part of it - often the frontend but also the backend in some cases). Assuming you accept it - which I think is already not good, you have conflict issues very often with concurrent PR,
• System property reduces reproducibility since you need to ensure to pass the same value between builds which is manual so will fail (Murphy's law)
• Ultimately there are multiple plugins able to inject this date from a computed criteria - most of the time using SCM - so we should be able to use it by design.

This PR just tries to make last option not emitting a warning.

[hboutemy]

I mostly disagree on why fixed value is or not a good idea (and how to do it), but let's take that apart

but I agree that some people may prefer to inject last git commit time: please share a project doing that, and what issue m-artifact-p is causing in that case (what you call "not tolerate", which I don't know what are the symptoms): only after, we'll see what to improve. For now, I can't get where this PR is going

[rmannibucau]

@hboutemy you can check the test, the log.error will be emitted if you use a property instead of a hardcoded value which adds maintenance burden (either force a manual step or to work this outside maven ecosystem which are both not satisfying). So idea is to enable project to set up this property without any downside (log errors or plain error in check mojo).

[hboutemy]

I don't see any pom.xml to run any mvn command against: I don't know what the initial symptom is (with which artifact goal, during what type of work)
and given AFAIK the current behaviour "tolerates" because it does not fail, i don't see what more tolerant behaviour is useful

given there is MARTIFACT-54 bug, I'll release with that fix and we'll see later MARTIFACT-59 (please rework your PR: it's not MNG)

[rmannibucau]

I updated the description, an external reference is yupiik/tools-maven-plugin#12 but long story short it is <project.build.outputTimestamp>${git.commit.time}</project.build.outputTimestamp> like usages.

I thought about your position to say it is wrong to not hardcode it, if this PR is really not an option, I think I could "not care" about it if the value is automatically rewritten by a plugin.
I'm not yet sure of the best plugin for that - artifact looks wrong, git-commit-id too, spotless or pom rewriters don't know about last change date so not yet fixed on where.

Would it be better for you?

[hboutemy]

long story short it is <project.build.outputTimestamp>${git.commit.time}</project.build.outputTimestamp> like usages.

thanks: it's the first step into describing: then what is the symptom? what happens when you do that, that you want to change?

I thought about your position to say it is wrong to not hardcode it, if this PR is really not an option, I think I could "not care" about it if the value is automatically rewritten by a plugin.
I'm not yet sure of the best plugin for that - artifact looks wrong, git-commit-id too, spotless or pom rewriters don't know about last change date so not yet fixed on where.

what I say is that using git commit is not the most efficient, because it changes on each commit even if the jar file content does not really change: then it forces jar update only for timestamp.

Having a value hard coded + maven-release-plugin and versions-maven-plugin update/rewrite when version is updated is the most efficient strategy found so far: I know it's not a usual strategy, but it never creates a different jar when it is not necessary because its content has really changed.

But I don't fight hard against people wanting to use git commit: from a theoretical perspective, I understand it looks more natural. If you want to go that route, no problem. If you experience a problem maven-artifact-plugin, I just need to see what concrete problem is caused by what goal of maven-artifact-plugin for you when doing that strategy, to understand how to update the code to precisely detect that case and not have unintended side effects.

FYI, moditect uses the git commit strategy and there is no issue for them: https://github.com/moditect/moditect => https://github.com/jvm-repo-rebuild/reproducible-central/blob/master/content/org/moditect/moditect/README.md

[rmannibucau]

Having a value hard coded + maven-release-plugin and versions-maven-plugin update/rewrite when version is updated is the most efficient strategy found so far: I know it's not a usual strategy, but it never creates a different jar when it is not necessary because its content has really changed.

I almost more than agree with that except the first parth.
For me it means we must not set the value in the pom but set the strategy - you "only modify when needed" works until you use last modified date of the jars so guess in <build> we should get an option to define outputTimestamp when model is built - with multiple lookup flavors but it sounds like something which must be in maven to ensure any plugin can rely on this read only value otherwise anything is wrong.

Now (v4) we have a root pom we can enforce to define it in the root pom for the whole project too.

Side note: I don't care much if we stay in the status quo for v3, it kind of works.

FYI, moditect uses the git commit strategy and there is no issue for them

Cause they don't use any of artifact plugin nor use it in the root pom otherwise it is there for them too ;).

So overall more I think to it more I agree with you we do it wrong and should make it immutable - we should make it immutable ;) - and in maven core - there is no way it works reliably in any plugin by design and it is shared anyway by all plugins and maven properties - thinking to injected metadata in artifacts with filtering.
So it sounds like the ability to read .git folder - don't think we need jgit/scm-provider or alike at all.
Not sure any other SCM is worth it today.

So my proposal would be:

1. add a property in Build model
2. enable it to be a hardcoded value or provider "id" (git for ex)
3. ensure this value is initialized early and fully immutable
4. let plugin consume it in plugins

wdyt?

[hboutemy]

FYI, moditect uses the git commit strategy and there is no issue for them

Cause they don't use any of artifact plugin nor use it in the root pom otherwise it is there for them too ;).

wrong, they use everything, and it works: I still don't get what needs to be changed in maven-artifact-plugin

So my proposal would be:
...

this proposal is not in the hands of the maven-artifact-plugin, but Maven 5 (new element in <build>) and decision to make Maven core depend on Git or not: a discussion for Maven core in the mailing list

[rmannibucau]

wrong, they use everything, and it works: I still don't get what needs to be changed in maven-artifact-plugin

wrong (ok was cause it made me smile to read it)

please just run buildinfo goal and you'll get in the parent - as explained in my comment:

[WARNING] Reproducible Build not activated by project.build.outputTimestamp property: see https://maven.apache.org/guides/mini/guide-reproducible-builds.html

Which is not a "it works as expected" behavior for me. My setup with git-commit-id works similarly but a warning in the build is just a sign it is buggy by design and there it is technically justifiable too.

Now, as mentionned, run mvn artifact:check-buildplan and you get - still in the parent but you get it:

[ERROR] Reproducible Build not activated by project.build.outputTimestamp property: see https://maven.apache.org/guides/mini/guide-reproducible-builds.html

and a nice

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-artifact-plugin:3.5.0:check-buildplan (default-cli) on project moditect-parent: non-reproducible plugin or configuration found with fix available -> [Help 1]

which breaks the build.

So no, it does not work for them.

[hboutemy]

we're starting a little progression because now we have a concrete example to run against, instead of just speaking

on https://github.com/moditect/moditect.git, mvn artifact:3.5.1:check-buildplan and mvn artifact:3.5.1:buildinfo work flawlessly
As written before, 3.5.0 had a stupid bug MARTIFACT-54: i should have done a release a long time ago, because this bug has caused many headaches to many people

[rmannibucau]

@hboutemy not sure what you tried to explain but maven-artifact-plugin 3.5.1 behaves 100% the same so no step forward. Your commit does not change the behavior when build the whole project, even the change after which moves to the session does not change that so it still does not work for the referenced projects.
Even tested the 3.5.2-SNAPSHOT and behavior is the same - actually worse cause the error is printed on all modules whereas before it was only on the parent.

[hboutemy]

I could play and just say that your experience is wrong and mine right, but this would not help...
but I'm very tempted

[rmannibucau]

I could play and just say that your experience is wrong and mine right, but this would not help...

I would be happy if true but what did you do different from me? Also code is more on my side than yours for now - see this PR for details - so I don't understand what you're trying to say for now.

[hboutemy]

I would be happy if true but what did you do different from me?

I don't know what you did, you talked but did not share any output. I ran check-buildplan against latest release of moditect for 3.5.0 and 3.5.1: see https://gist.github.com/hboutemy/cca5a94c6f7a9bc65023a31c31d3308f

Also code is more on my side than yours for now - see this PR for details - so I don't understand what you're trying to say for now.

this code does some magic about "injecting": I don't know what injecting means, from unit test, I don't get what CLI command it is related to: mvn -Dproject.nuild.outputTimestamp=set? How is it related to using Git commit id configured in pom.xml = what we are discussing?
Yes, there is code with vague explanations: from the very beginning, I'm asking for concrete tests showing what is tried to be solved, given the single "tolerate injected timestamp value" description that is given does not explain at all. I'm trying to guess the problem, because the code and the test are strange (and marked MNG at the beginning)

[rmannibucau]

see https://gist.github.com/hboutemy/cca5a94c6f7a9bc65023a31c31d3308f

You have the same error than me.

I don't get what CLI command it is related to: mvn -Dproject.nuild.outputTimestamp=set

I'm trying to guess the problem

mvn xxxx no -D. The key point is that in the pom you have an interpolated value. See #30 (comment) which explains it.

This kind of injected (=interpolated) value just fails exactly the same than moditech in your example on both commands (first exit with status=0 but logs an error which is wrong and second just fails).

[hboutemy]

You have the same error than me.

there are 2 runs, look at the first one: no problem with 3.5.1, same problem with 3.5.0

The key point is that in the pom you have an interpolated value

the unit test does not have any pom.xml: it's hard to guess what the code tries to represent from an end-user point of view

[rmannibucau]

there are 2 runs, look at the first one: no problem with 3.5.1, same problem with 3.5.0

Hmm, not sure what which artifact you used built but got back the plugin from asf repo and still getting the same error: https://gist.github.com/rmannibucau/816c07cd6277e602584d0bb2b4497c5a#file-output-txt-L29

If it helps:

$ shasum ~/.m2/repository/org/apache/maven/plugins/maven-artifact-plugin/3.5.1/maven-artifact-plugin-3.5.1.jar
c24f8609c90af05fd1fd56c4ad238dacd7fbd791 /home/rmannibucau/.m2/repository/org/apache/maven/plugins/maven-artifact-plugin/3.5.1/maven-artifact-plugin-3.5.1.jar

the unit test does not have any pom.xml: it's hard to guess what the code tries to represent from an end-user point of view

assuming it is ambiguous:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>com.github.rmannibucau.test</groupId>
  <artifactId>foo</artifactId>
  <version>1.0.0-SNAPSHOT</version>

  <properties>
    <!-- this is what this PR was fixing by validating at eval time the presence of the property and not at model loading time -->
    <project.build.outputTimestamp>${git.commit.time}</project.build.outputTimestamp>
  </properties>

   <!-- git-commit-id-plugin in initialize phase since there is nothing much before that phase for a maven build from plugins -->
</project>

[hboutemy]

yours: [INFO] Building ModiTect Parent 1.3.0-SNAPSHOT [1/8]
mine: [INFO] Building ModiTect Parent 1.2.1.Final [1/8]

[rmannibucau]

@hboutemy ok, so 1.2.1 is using a hardcoded value - so out of scope of this PR - and 1.3.0-SNAPSHOT does use a placeholder (this PR case).