[LOGTOOL-160] Fix skiping of rendering a Generated annotation

[marko-bekhta]

Hey @jamezp !

It's me again 😄. This PR is similar to #81.

This came up from a discussion at reproducible-central: jvm-repo-rebuild/reproducible-central#178 (comment)

The build of Hibernate Search is reproducible in terms of produced jars, but as for sources - because of the generated classes the timestamp is causing a bit of a problem.

I've noticed that there was this org.jboss.logging.tools.addGeneratedAnnotation option and thought we could use it, but from what it seems, the option wasn't really making a difference 😕 😔. So while I've added a suggested patch to fix that I've also included a way for a user to override how the value of @Generated(date =?) is rendered....

Let me know if it makes sense, and if so, I'll add something to the docs for the new options ... Thanks!

[jamezp]

Hello @marko-bekhta. I definitely need to make a decision on these, I apologize :) Unfortunately, I'm just about to leave for PTO, but only for two days. I'm back Friday so I'll try to have a look then. We could probably use a new release of the tooling anyway :)

[jamezp]

I think I'm a little confused about the need for this. There is already a org.jboss.logging.tools.addGeneratedAnnotation configuration property which will simply not add the annotation.

I also don't understand why generated code would even be inspected :)

[marko-bekhta]

Hey 😃,

Thanks for taking a closer look at this one!

In most cases, we publish the jar, javadocs, and the sources jar. The sources jar contains the generated logger with the annotation. So, if we consider that all 3 archives are the build artifacts, it would mean that the sources jar would be different on each run.

Now as for the org.jboss.logging.tools.addGeneratedAnnotation, I thought about using it, but we'd need at least the first commit from this patch to make the processor actually consider the passed value 🙈... now, as for the new options... my thinking here was that since the logger impls are generated, it would be nice to keep the annotation on them.

Now, going to our specific case with Search, we are using Maven for it, and there's this project.build.outputTimestamp [1] property that is picked up by most of the Maven plugins and updated on release (also automatically updated when version:set is used). Hence my thinking here was that we could pass the value of this property to the processor through a parameter (Type.FIXED) and let the exact same date be used as if the loggers were generated on that date.

So long story short 😃 I agree that org.jboss.logging.tools.addGeneratedAnnotation could be enough if it works, and those new properties (org.jboss.logging.tools.generatedDateValueProvider) would be nice to have, but if you don't find them much useful, we can survive without them 😄.

Hope this brings some clarity to what we are trying to do 🙈 😃

[1] https://maven.apache.org/guides/mini/guide-reproducible-builds.html#how-do-i-configure-my-maven-build

[jamezp]

TBH I'd argue that generated source files should not be part of the sources :) If you were to build from source those would be overridden anyway.

For the org.jboss.logging.tools.addGeneratedAnnotation it does look like I accidentally disabled that some how so that's definitely a bug :)

FWIW I don't think generate code should be deterministic. It is generated and typically not added to version control and if building from source, it would be replaced anyway.

[marko-bekhta]

Well... I cannot think of any other arguments for it beyond "It would be nice to be able to generate the same sources when executing the build multiple times without any code changes" 😃

I'll update the PR to remove the new properties and keep the patch around just in case something changes and we reconsider 😄.

[yrodiere]

Hey @jamezp, trying to see if we can get a solution to this in the next release, as I understand it will happen soon :)

FWIW I don't think generate code should be deterministic. It is generated and typically not added to version control and if building from source, it would be replaced anyway.

I'm a bit confused by this statement.

For context, we're trying to set up a reproducible build of a library using the JBoss Logging annotation processor. So what we need is, if we build the same commit twice on two different environments at two different times, we get the same JAR, bit for bit. This is interesting for various reasons, the least important perhaps being caching, and the most important being productization and security related.

If generated code is not deterministic, then reproducible builds cannot be achieved unless we add generated code to version control. Which I'm personally not inclined to do, and it seems you are not either.

It does seem like a valid problem, doesn't it?

Note we don't need generated code to be the same for all versions of JBoss Logging. Just for a particular commit of our library, which also means a particular version (micro) of JBoss Logging.

For the org.jboss.logging.tools.addGeneratedAnnotation it does look like I accidentally disabled that some how so that's definitely a bug :)

FWIW I would personally be fine with just restoring this feature if that's enough to make the code generation deterministic.

[yrodiere]

For the org.jboss.logging.tools.addGeneratedAnnotation it does look like I accidentally disabled that some how so that's definitely a bug :)

FWIW I would personally be fine with just restoring this feature if that's enough to make the code generation deterministic.

@marko-bekhta it seems your PR now only solves this specific problem, so maybe we should update the description?

@jamezp Does the changeset seem good to merge? It's much smaller and to the point now.

[jamezp]

Hey @jamezp, trying to see if we can get a solution to this in the next release, as I understand it will happen soon :)

I will try and get a release out today or tomorrow for sure :)

FWIW I don't think generate code should be deterministic. It is generated and typically not added to version control and if building from source, it would be replaced anyway.

I'm a bit confused by this statement.

For context, we're trying to set up a reproducible build of a library using the JBoss Logging annotation processor. So what we need is, if we build the same commit twice on two different environments at two different times, we get the same JAR, bit for bit. This is interesting for various reasons, the least important perhaps being caching, and the most important being productization and security related.

If generated code is not deterministic, then reproducible builds cannot be achieved unless we add generated code to version control. Which I'm personally not inclined to do, and it seems you are not either.

It does seem like a valid problem, doesn't it?

Note we don't need generated code to be the same for all versions of JBoss Logging. Just for a particular commit of our library, which also means a particular version (micro) of JBoss Logging.

Perhaps I was a bit too broad with this comment :) What I meant was in this specific instance. The byte code shouldn't change between builds. The annotation shouldn't be included as it's got a scope of source, so it's only on the source file.

Note, maybe I'm missing something :) But, I don't really see how it's not reproducible in this way because the annotation processor would need to be used for building.

For the org.jboss.logging.tools.addGeneratedAnnotation it does look like I accidentally disabled that some how so that's definitely a bug :)

FWIW I would personally be fine with just restoring this feature if that's enough to make the code generation deterministic.

[yrodiere]

Perhaps I was a bit too broad with this comment :) What I meant was in this specific instance. The byte code shouldn't change between builds. The annotation shouldn't be included as it's got a scope of source, so it's only on the source file.

Right, if the annotation isn't included in bytecode, then yes the build is already reproducible for the main JAR.

I missed the context of your statement, which was source JARs specifically. I'll concede I do not know if we are required to include generated code in these JARs, and that it's not completely clear if having the build of those JARs be reproducible is required for productizations. But frankly, it seems simpler to make it reproducible than to find out if we must.

And since you merged this PR, soon we'll be able to do it, so we're done here: thank you!