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

Manifest Last Modified #3118

Closed
swimmesberger opened this issue Apr 2, 2019 · 16 comments
Closed

Manifest Last Modified #3118

swimmesberger opened this issue Apr 2, 2019 · 16 comments
Assignees
Milestone

Comments

@swimmesberger
Copy link
Contributor

It seems that the last modified date of the generated jar of the bnd (gradle) jar task is not updated when only the MANIFEST.MF file is changed*. This is can lead to issues with gradle file caching. For our gradle plugin we rely on the gradle reporting if the generated jar is up to date or not and based on that reporting we do some additional work. When the bnd plugin is used this do not work because sometimes (when only the manifest is changed) gradle reports that the jar file is still up to date (even if it's clearly not because gradle uses last modified before the content is checked).

It know that I could define a manifest file via "-manifest" and then the last modified reporting would work but that is very unfortunate because I want to rely on the bnd manifest generation.
Maybe it's possible to compare the previous calculated manifest with the newly calculated manifest and only call the "updateModified" (with the current timestamp) when a change is detected.

As a workaround for now we added some additional properties to our custom gradle task which ensures that it's reexecuted but it would be much cleaner if we could simply rely on the generated jar.

Example of a flawed gradle build cycle with bnd:
Task :com.example.bundle:jar
Task ':com.example.bundle:jar' is not up-to-date because Value of input property 'someProp' has changed

Task :com.example.bundle:customTask UP-TO-DATE
Skipping task '::com.example.bundle:customTask' as it is up-to-date.

The customTask task defines an InputFile dependency on the generated jar file but even though the jar task of bnd is reexecuted and the file is newly written with an updated Manifest our customTask is not reexecuted because gradle does not detect the file change because the lastModified file property did not change.

@pkriens
Copy link
Member

pkriens commented Apr 3, 2019 via email

@swimmesberger
Copy link
Contributor Author

I use Gradle 5.3 and can say that when within the same gradle process (same build cycle) when a additional task uses the output of the bnd jar task as input, gradle considers the file UP-TO-DATE even tough if I examine the content of the jar - the file has changed.

I think the best solution is to find out why the manifest changes, if it had the same inputs it should generate the same manifest? If the contents depends on an outside source you could define this in a gradle dependency or include this source in your JAR?

We have some custom properties in the manifest (like the build which produced the artifact) which can change between each build. We already defines such external properties as dependencies for the bnd gradle jar task but we don't want to duplicate these dependencies for each task which depends on the output jar. So we simply want to rely on the jar task output that when the file is changed in someway gradle reports it as modified but this does not work with bnd.

This would also break plugins which rely on the default behavior of the jar task - when you add attributes to the manifest to the default gradle jar task gradle correctly writes the file modified property of the file bnd does not...
Unfortunately this behavior makes writing gradle plugins which rely on bnd much less fun.

@pkriens
Copy link
Member

pkriens commented Apr 3, 2019

I am going to wait for @bjhargrave since he worked on related issues and knows more about Gradle

@bjhargrave
Copy link
Member

This seems to me an issue with how you have expressed your dependencies in gradle. Your customTask should do inputs.files tasks.named('jar'). That is, the customTask's inputs are the jar task itself and not the files you think the jar task is making.

If that does not help, can you please provide a small github repo which demonstrates the problem? Then I can investigate. Thanks.

@swimmesberger
Copy link
Contributor Author

I have created a small self contained example which reproduces the problem (on Windows 10 x64):
https://github.com/swimmesberger/bnd-manifest-bug-test

@bjhargrave
Copy link
Member

So I played with your example and the issue seems to be with gradle.

[hargrave@mbp2017 bnd-manifest-bug-test (master)]$ export BND_TEST=3
[hargrave@mbp2017 bnd-manifest-bug-test (master)]$ ./gradlew bndBugTest --info --console=verbose && shasum TestProject/generated/TestProject.jar 

[elided]

> Task :TestProject:jar
Task ':TestProject:jar' is not up-to-date because:
  Value of input property 'externalProperty' has changed for task ':TestProject:jar'
Generated bundles: [/Users/hargrave/git/tmp/bnd-manifest-bug-test/TestProject/generated/TestProject.jar]
:TestProject:jar (Thread[Execution worker for ':' Thread 7,5,main]) completed. Took 0.025 secs.
:TestProject:bndBugTest (Thread[Execution worker for ':' Thread 7,5,main]) started.

> Task :TestProject:bndBugTest
Task ':TestProject:bndBugTest' is not up-to-date because:
  Input property '$1' file /Users/hargrave/git/tmp/bnd-manifest-bug-test/TestProject/generated/TestProject.jar has changed.
Executed!
:TestProject:bndBugTest (Thread[Execution worker for ':' Thread 7,5,main]) completed. Took 0.002 secs.

BUILD SUCCESSFUL in 0s
3 actionable tasks: 2 executed, 1 up-to-date
d7442736734256cef359edf3548f776565175cd4  TestProject/generated/TestProject.jar
[hargrave@mbp2017 bnd-manifest-bug-test (master)]$ export BND_TEST=4
[hargrave@mbp2017 bnd-manifest-bug-test (master)]$ ./gradlew bndBugTest --info --console=verbose && shasum TestProject/generated/TestProject.jar 

[elided]

> Task :TestProject:jar
Task ':TestProject:jar' is not up-to-date because:
  Value of input property 'externalProperty' has changed for task ':TestProject:jar'
Generated bundles: [/Users/hargrave/git/tmp/bnd-manifest-bug-test/TestProject/generated/TestProject.jar]
:TestProject:jar (Thread[Execution worker for ':',5,main]) completed. Took 0.024 secs.
:TestProject:bndBugTest (Thread[Execution worker for ':',5,main]) started.

> Task :TestProject:bndBugTest UP-TO-DATE
Skipping task ':TestProject:bndBugTest' as it is up-to-date.
:TestProject:bndBugTest (Thread[Execution worker for ':',5,main]) completed. Took 0.001 secs.

BUILD SUCCESSFUL in 0s
3 actionable tasks: 1 executed, 2 up-to-date
ae9fef145200937a9a179657a5d1a22f077c8db2  TestProject/generated/TestProject.jar
[hargrave@mbp2017 bnd-manifest-bug-test (master)]$ 

You can see that gradle ran the jar task both time and the the SHA of the jar generated by Bnd is different. Yet, in the second execution gradle does not declare the bndBugTest task to be out of date.

So to me, this looks like an issue with gradle and not with Bnd since it always makes the jar and the jar's SHA changes.

PS. I moved the build.gradle file from the root into TestProject since it only affects that project and cleaned up the script:

def jarTask = tasks.named('jar') {
  inputs.property('externalProperty', System.getenv('BND_TEST'))
}

tasks.register('bndBugTest') {
  def outputFile = new File(project.buildDir, 'outputFile.txt')
  inputs.files(jarTask)
  outputs.file(outputFile)
  doLast {
    project.delete(outputFile)
    outputFile.createNewFile()
    System.out.println("Executed!")
  }
}

@swimmesberger
Copy link
Contributor Author

swimmesberger commented Apr 8, 2019

Thanks for the detailed explanation. Is it possible that gradle sometimes tries to not calculate a new hash when the file last modified property is not altered?

Code like this makes me suspicious:
https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/java/org/gradle/api/internal/changedetection/state/CachingFileHasher.java#L85

Also if I do something like this:

jarTask.doLast(t -> {
      final Jar jarTask = (Jar) t;
      final File jarFile = jarTask.getArchiveFile().get().getAsFile();
      jarFile.setLastModified(System.currentTimeMillis());
});

It works as expected

@bjhargrave
Copy link
Member

I would submit that Gradle is still wrong here according to its own documentation.

https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:how_does_it_work

https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/InputFile.html

Fingerprinting talks about the hash of the file contents and nothing about the timestamp. A changed timestamp could be a way to declare the file changed and avoid the expense of hash computation, but an unchanged timestamp does not mean you can skip hash computation and declare the file unchanged. We see that in the example here, it works as expected for several iterations and then it stops.

@swimmesberger
Copy link
Contributor Author

swimmesberger commented Apr 8, 2019

I have created an issue in the gradle github project let's see what this leads us to 😃

@swimmesberger
Copy link
Contributor Author

swimmesberger commented May 22, 2019

It seems that gradle is indeed using the "file modification date and file size" before even comparing the hash (Gradle Documentation will be updated). Which brings us back to exactly this issue - we HAVE TO update the last modified date when the MANIFEST.MF file changes to conform to gradles up-to-date checking 🤔

@bjhargrave
Copy link
Member

bjhargrave commented May 22, 2019

we HAVE TO update the last modified date when the MANIFEST.MF file changes to conform to gradles up-to-date

MANIFEST.MF is generated. Is it not "changed". So under your suggestion, the MANIFEST.MF resource always has the current time as the last modified date and thus the jar always has the current time as the last modified date. This then obviates all the work being done by Jar to capture the last modified date of the resources which contribute to the jar.

@swimmesberger
Copy link
Contributor Author

swimmesberger commented May 28, 2019

Yeah I understand that it might be pretty tricky because the MANIFEST.MF is generated. It might be possible to detect changes between the last MANIFEST.MF and the newly calculated one and only change the lastModified if something has changed but for that to work you would have to know the previous state. I have no idea how easy/difficult that would be to implement in the current bnd codebase.

@bjhargrave
Copy link
Member

It might be possible to detect changes between the last MANIFEST.MF and the newly calculated one and only change the lastModified if something has changed but for that to work you would have to know what the previous state.

But we generally do not "know" the previous state. And it seems wrong to add it just for generated resources like the manifest.

@bjhargrave bjhargrave self-assigned this May 31, 2019
@bjhargrave bjhargrave added this to the 4.3 milestone May 31, 2019
@bjhargrave
Copy link
Member

I will update Bnd to allow built jars to get the current time. Call Jar.updateModified with TSTAMP value.

@rotty3000
Copy link
Contributor

@bjhargrave
Copy link
Member

when tackling this please consider this weird logic in the bnd-maven-plugin

Since bnd-maven-plugin does not write the final jar (maven-jar-plugin does that), I don't think anything special is needed to maven.

I will not be changing the any of the classes in bndlib regarding lastmodified. There is much code which relies on this. So I will just update the gradle plugin to update the jar file once written.

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

No branches or pull requests

4 participants