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

bnd-maven-plugin: Problems with #1333 #1346

Closed
erwint opened this issue Mar 10, 2016 · 10 comments
Closed

bnd-maven-plugin: Problems with #1333 #1346

erwint opened this issue Mar 10, 2016 · 10 comments
Assignees
Milestone

Comments

@erwint
Copy link
Contributor

erwint commented Mar 10, 2016

Quoting from #1333 there are several problems with this change:

This change makes it better, but a considerable amount of time is spent on creating a aQute.bnd.osgi.Jar (https://github.com/bndtools/bnd/blob/master/biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java#L1306)

If you defer setting the buildpath (which implicitly creates the Jar instances) and instead just call builder.updateModified with the Jar's lastModified (a Jar should never be older than any of it's content) or in case of a directory with the one from the newest file in that directory - it reduces buildtimes in my setup by about 15%.

and

a resource path may be the same as the source path, in this case there is no need to add it to the source directories.

and

As you do not check for incremental builds, now the Manifest is not always generated. Instead, the default manifest generated by the archive plugin is found there.

@bjhargrave
Copy link
Member

Quoting from #1333 there are several problems with this change:

This change makes it better, but a considerable amount of time is spent on creating a aQute.bnd.osgi.Jar (https://github.com/bndtools/bnd/blob/master/biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java#L1306)

If you defer setting the buildpath (which implicitly creates the Jar instances) and instead just call builder.updateModified with the Jar's lastModified (a Jar should never be older than any of it's content) or in case of a directory with the one from the newest file in that directory - it reduces buildtimes in my setup by about 15%.

This is how bnd works, I am rather loath to special case thing here.

and

a resource path may be the same as the source path, in this case there is no need to add it to the source directories.

I am not sure what you mean here.src/main/resources is not the same as src/main/java. I suppose someone could configure maven in a non-standard way, but in those rare cases, there is little harm in the folder being added twice.

and

As you do not check for incremental builds, now the Manifest is not always generated. Instead, the default manifest generated by the archive plugin is found there.

I don't understand this. The manifest can only be generated when you build the jar, and the main point of your recent requests was to avoid building the jar. It you always want the manifest built, then we also need to always build the jar.

@erwint
Copy link
Contributor Author

erwint commented Mar 10, 2016

  1. In a commandline build it does not matter, if all Jars on the builpath are processed, as it happens just once. In Eclipse the Build is triggered much more often. I'm again quoting https://wiki.eclipse.org/M2E_compatible_maven_plugins:

Mojos that runOnIncremental=true (the default), will be executed for any resource file, including all sources and generated files under target/. For performance and stability reasons it is absolutely essential to short-cut any time consuming work and all filesystem changes if there are no changes to the input sources processed by the mojo. For java code generating mojos, failure to act on relevant input changes only will almost certainly result in endless build -- mojo generates .java files, which triggers jdt to generate .class files, which triggers the mojo to generate .java files, and so on.

  1. src/main/resources is just the default. You can map it to anything, including using a src folder in Eclipse-style, where all Java files are used as the source and all other as resources. If it's no harm and no significant extra work (scanning twice?), then of course it does not matter.

  2. The problem here is the archiver plugin, or more exactly the m2e counterpart of it. It will always build a default manifest. While I manged to stop it from overwriting the manifest generated by bnd-maven-pluging (see Handle <useDefaultManifestFile>true</useDefaultManifestFile> correctly tesla/m2eclipse-mavenarchiver#7), with your latest changes I now encounter the case where I only find the default manifest. Somehow bnd-maven-plugin must have skipped the generation, and I think my initial assumption was wrong and it's because of https://github.com/bndtools/bnd/blob/master/maven/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/BndMavenPlugin.java#L184: you would need to force the generation if there is no Manifest.

@bjhargrave
Copy link
Member

  1. In a commandline build it does not matter, if all Jars on the builpath are processed, as it happens just once. In Eclipse the Build is triggered much more often.

The jars are input to the builder whose lastModified time it used to decide whether to build the bundle.

  1. src/main/resources is just the default. You can map it to anything, including using a src folder in Eclipse-style, where all Java files are used as the source and all other as resources. If it's no harm and no significant extra work (scanning twice?), then of course it does not matter.

Yes it can be changed but this would be rather the rare case as most of maven assume you use the default file layout.

  1. The problem here is the archiver plugin, or more exactly the m2e counterpart of it. It will always build a default manifest. While I manged to stop it from overwriting the manifest generated by bnd-maven-pluging (see Handle <useDefaultManifestFile>true</useDefaultManifestFile> correctly tesla/m2eclipse-mavenarchiver#7), with your latest changes I now encounter the case where I only find the default manifest. Somehow bnd-maven-plugin must have skipped the generation, and I think my initial assumption was wrong and it's because of https://github.com/bndtools/bnd/blob/master/maven/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/BndMavenPlugin.java#L184: you would need to force the generation if there is no Manifest.

The manifest is built as part of building the in-memory jar. You want to avoid building the in-memory jar in incremental builds which means it will not build the manifest.

If the archiver plugin always rewrites the manifest and then the bnd-maven-plugin were to then always overwrite that manifest, then the bundle output will always appear to have been changed defeating the time savings of the incremental build. Or am I missing something here? It seems the bug is in the archiver plugin if it always writes a manifest even where there are no incremental changes.

@erwint
Copy link
Contributor Author

erwint commented Mar 10, 2016

  1. In a commandline build it does not matter, if all Jars on the builpath are processed, as it happens just once. In Eclipse the Build is triggered much more often.
    The jars are input to the builder whose lastModified time it used to decide whether to build the bundle.

yes, and that would be ok. However, in the Constructor of the Jar the whole content is scanned (and if I'm not mistaken the lastModified time of all entries is used, too). As a project can have easily more than 50 dependencies, that load is significant - and just to find out there's nothing to do.

The manifest is built as part of building the in-memory jar. You want to avoid building the in-memory jar in incremental builds which means it will not build the manifest.

yes, in an incremental build when there's no change you do not want to build the Jar in memory. However, if it's not an incremental build or if there is no Manifest at the target location, then you'd need to build the Jar so that the Manifest can be written.

If the archiver plugin always rewrites the manifest and then the bnd-maven-plugin were to then always overwrite that manifest, then the bundle output will always appear to have been changed defeating the time savings of the incremental build. Or am I missing something here? It seems the bug is in the archiver plugin if it always writes a manifest even where there are no incremental changes.

It's even worse, as without my patch the archiver plugin was overwriting the manifest (it comes later in the lifecycle!). With my patch it will not overwrite the manifest if one is already present (and the config option to use an existing manifest is on).

It's just that with your latest changes I sometime see the "plain" Manifest instead of the bnd-generated one. Which leads me to believe that starting from a clean directory the bnd-maven-plugin somehow decided nothing is to do and then the m2e-archiver took over to generate the manifest. It worked before, also with my suggested changes. That's why I think there must be some oversight in the check when to generate the Jar and thus the Manifest.

I'll try to locate the problem exactly, as of course it does work correctly if I just clean the project...

@bjhargrave
Copy link
Member

I made PR #1347 to use BuildContext.isIncremental. If it returns false, the plugin will always build.

@bjhargrave
Copy link
Member

I added another commit to PR #1347 to defer setting the builder classpath until the decision is made to build the jar.

@erwint
Copy link
Contributor Author

erwint commented Mar 10, 2016

I tried out your branch and 3) seems fixed now

@bjhargrave
Copy link
Member

Please try the latest from master to see if your issues are attenuated.

@erwint
Copy link
Contributor Author

erwint commented Mar 15, 2016

I used it for a few days now and it looks good so far. Thanks!

@bjhargrave
Copy link
Member

Good, so I will close this issue. If you encounter additional problems, please open a new issue.

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

2 participants