[FELIX-6493] Extend is-up-to-date logic of Manifest goal

[HannesWell]

This PR addresses issue FELIX-6493 and extends the is-up-to-date logic of the Manifest goal to

• consider as not up-to-date if the MANIFEST.MF file to generate does not exist
• consider pom.xml modifications in up-to-date logic
• write incremental-info after manifest to not self-cause out-of-date

[HannesWell]

Besides the changes affecting the logic a moderate number of possible clean up changes.
To you want to have a dedicated issue for a clean up or can I create a subsequent PR without associated issue or should these changes go into this PR?

[HannesWell]

• consider pom.xml modifications in up-to-date logic

Actually the pom's of all parent projects have to be considered as well.
I'm about to update this PR accordingly.

[laeubi]

• consider pom.xml modifications in up-to-date logic

Actually the pom's of all parent projects have to be considered as well. I'm about to update this PR accordingly.

I'm not sure if this is not a bit out of scope. I would assume that if one updates the parent he also has to update the child to reflect the new version?

What I actually wonder is: Is there really a measurable benefit for all those stale checking? I could only think of a resource that was modified and thus do not change the generated manifest but does it really impact performance to regenerate the manifest in those scenarios?

[HannesWell]

• consider pom.xml modifications in up-to-date logic

Actually the pom's of all parent projects have to be considered as well. I'm about to update this PR accordingly.

I'm not sure if this is not a bit out of scope. I would assume that if one updates the parent he also has to update the child to reflect the new version?

For incremental builds in IDE's like Eclipse this is actually useful. If one modifies the parent pom (in a way that also affects the child's generated Manifest), Eclipse triggers an incremental build of parent and child project (I assume the ResourcesPlugin checks the project inter-dependencies). If the manifest plugin does not check the parent pom it falsely assume the manifest is up to date (because the child's pom did not change) and skips the execution. In order to re-generated the manifest a user would have to trigger a full build or modify the child pom manually. But especially during development between releases it is not necessary to change to version of the parent which would require adjustments of the child,

What I actually wonder is: Is there really a measurable benefit for all those stale checking? I could only think of a resource that was modified and thus do not change the generated manifest but does it really impact performance to regenerate the manifest in those scenarios?

From my recent observations the BND-Analyzer takes some time for larger projects with many classes to scan. So I think it is not worthless. At the moment biz.aQute.bndlib version 5.1.1 is used while 6.1.0 is available maybe the new major version also brings performance improvements.

But what I'm pretty confident about is that checking for new or modified Java-source files in anyJavaSourceFileTouchedSinceLastBuild() is not necessary because isUpToDate() checks the ${project.build.outputDirectory} for later modifications and the compiled class files are written there. So additional or modified java classes will cause later modifications in that directory.

[laeubi]

From my recent observations the BND-Analyzer takes some time for larger projects with many classes to scan. So I think it is not worthless.

Sure but how often is an incremental build triggered that does not is detected as a change? e.g if I see correctly it is only checked if a java file is affected but not if the change actually is a relevant change (e.g. if I just add a comment into a file this will still trigger generation of the manifest!).

So additional or modified java classes while cause later modifications in that directory

I'm not sure if this is always true as far as I know under linux one could disable that parents are updated.

[HannesWell]

From my recent observations the BND-Analyzer takes some time for larger projects with many classes to scan. So I think it is not worthless.

Sure but how often is an incremental build triggered that does not is detected as a change? e.g if I see correctly it is only checked if a java file is affected but not if the change actually is a relevant change (e.g. if I just add a comment into a file this will still trigger generation of the manifest!).

In general a change is not detected, if a non java resource is modified that is not copied into target/classes. It then depends on ones use cases/workspace. For some cases (like e.g. in our case in M2E where the project actually only contains a pom that specifies what is packed into a bundle) there are probably only very few cases where the manifest generation is skipped. But others might have other cases.

So additional or modified java classes while cause later modifications in that directory

I'm not sure if this is always true as far as I know under linux one could disable that parents are updated.

Of course one could disable autobuild in Eclipse (I don't know how other IDE handle it), but then no incremental builds happen anyways. But except this case, I don't know how to disable the write of compiled class files into target/classes directory?

[laeubi]

But except this case, I don't know how to disable the write of compiled class files into target/classes directory?

I mean that writing into a directory also updates the directory timestamp!

[HannesWell]

But except this case, I don't know how to disable the write of compiled class files into target/classes directory?

I mean that writing into a directory also updates the directory timestamp!

Ah, sorry I didn't understand that.
That's not a problem because the newer() methods walks the entire file-tree and checks each (transitively) contained file if a checked path is a directory.
For windows I don't even know if it is possible to update the timestamp of the containing directory if a contained element changes. At least by default this does not happen.

[laeubi]

But except this case, I don't know how to disable the write of compiled class files into target/classes directory?

I mean that writing into a directory also updates the directory timestamp!

Ah, sorry I didn't understand that. That's not a problem because the newer() methods walks the entire file-tree and checks each (transitively) contained file if a checked path is a directory. For windows I don't even know if it is possible to update the timestamp of the containing directory if a contained element changes. At least by default this does not happen.

Okay it seems I misread your comment then, I though you have proposed to only check the timestmap of the folder instead of its contents.

[laeubi]

@tjwatson @gnodet can you review/merge this PR?

[HannesWell]

But except this case, I don't know how to disable the write of compiled class files into target/classes directory?

I mean that writing into a directory also updates the directory timestamp!

Ah, sorry I didn't understand that. That's not a problem because the newer() methods walks the entire file-tree and checks each (transitively) contained file if a checked path is a directory. For windows I don't even know if it is possible to update the timestamp of the containing directory if a contained element changes. At least by default this does not happen.

Okay it seems I misread your comment then, I though you have proposed to only check the timestmap of the folder instead of its contents.

No no. Sorry for being unclear. My proposal is to skip the buildContext.newScanner(directory) in anyJavaSourceFileTouchedSinceLastBuild() (and maybe rename the method accordingly), which scans for new java files since the last build (if I understood the doc correctly).
But this should go into a separate clean-up change as suggested in my first comment.

[HannesWell]

@jbonofre, @cziegeler, hboutemy or @gnodet could you be so kind and review this change or ping somebody who can?
We would really appreciate if this change would be part of the next release and won't miss the next one too. Please let me know if I can do anything to speed up the review process.

[jbonofre]

I'm reviewing and testing this proposed change.

[jbonofre]

LGTM. NB: it would have been great to add a test to verify the metadata is cleanly generated.

[HannesWell]

I'm reviewing and testing this proposed change.

LGTM. NB: it would have been great to add a test to verify the metadata is cleanly generated.

Thank you.
Tests would indeed be good. But I wanted to discuss the technical details first to not (potentially) write tests unnecessarily. Unfortunately I have no time to look into that at the moment.