search

tesla / m2eclipse-tycho

25 stars 26 forks source link

Add support for maven-bundle-plugin:2.5.0 #19

Closed gamerson closed 10 years ago

gamerson commented 10 years ago

The Tycho project configurator fails to correctly update project config when the project uses the latest maven-bundle-plugin 2.5.0, because there are assumptions that the number of mojoexecutions will be 1 and with 2.5.0 there are 2.

See https://github.com/bndtools/bndtools/issues/873

So this proposed fix simply loosens that logic and allows 1 or more, and continues to configure the project. Also added a unit test along with lots of new repo binaries to get the tests to pass.

ifedorenko commented 10 years ago

I believe you will need to sign a contributor license agreement before we can accept this patch, but I need to double-check.

Also, the amount of extra artifacts required by the test seems excessive. I prefer the same version of maven and other dependencies used by all m2e/tycho tests.

ifedorenko commented 10 years ago

The CLA http://takari.io/support/takaricla.pdf

gamerson commented 10 years ago

Sure thing I'll get the CLA stuff done. Regarding the extra artifacts, I do agree it is really excessive. I do have a question related to how to use existing artifacts. So if I add the maven-bundle-plugin:2.5.0 artifact into the remote repo so we can test it, can I lock down the versions of all the other dependencies in the test pom.xml even if the maven-bundle-plugin explicitly says it depends on other artifacts like doxia-sink-api:1.0 and all that is already in "remoterepo" is doxia-sina-api:1.0-alpha7? This is just one example there are scores more. Thanks for the help with this.

On Sun, Jul 27, 2014 at 2:37 PM, ifedorenko notifications@github.com wrote:

The CLA http://takari.io/support/takaricla.pdf

— Reply to this email directly or view it on GitHub https://github.com/tesla/m2eclipse-tycho/pull/19#issuecomment-50257213.

Greg Amerson Liferay Developer Tools Liferay, Inc. www.liferay.com

ifedorenko commented 10 years ago

I had a closer look at this.

The problem is not specific to maven-bundle-plugin 2.5.0, the same problem can be reproduced with any maven-bundle-plugin version, all is necessary is to configure two or more executions of 'bundle' goal for the same project.

I do not believe the proposed fix is correct.

There are two cases to consider when a project has multiple bundle goal executions: 1) all executions generate bundle manifest at the same location and 2) there are multiple bundle manifests generated during the build.

The first case, i.e. the same manifest is regenerated multiple times, is most likely a project misconfiguration, at least I don't see why it should be needed to generate bundle manifest at the same location multiple times during the build.

The second case, i.e. multiple manifests are generated during the build, even if it makes sense from project build point of view, cannot be supported inside m2e-tycho because PDE only allows single bundle manifest per project.

So, unless I am missing a usecase, if a project has multiple bundle goal executions, m2e-tycho should simply mark all this executions with "multiple bundle plugin execution" error.

gamerson commented 10 years ago

Thanks Igor for taking the time to explain the thinking behind how Tycho m2e connector was designed. I see now in the org.sonatype.tycho.m2e that is has configured the package>bundle to be handled by the connector. It does make sense that the Tycho m2e connector is handling the bundle goal execution and that if a user overrides in the pom.xml it would be an invalid configuration.

ifedorenko commented 10 years ago

I think it will be useful improvement to detect and report via error markers when multiple bundle and/or manifest goals are configured for the same project. The current behaviour to throw illegal argument exception is not exactly user friendly and does not help to identify and fix the root cause of the problem.

gamerson commented 10 years ago

So what would you want to see happen if the user has multiple bundle executions? Creating the error markers would be straightforward but should that be done in the configure() method or in the calculateRawClasspath()? And if the error markers were added, what should happen in the generateBundleManifest() method, NOP?

On Tue, Jul 29, 2014 at 2:19 AM, ifedorenko notifications@github.com wrote:

I think it will be useful improvement to detect and report via error markers when multiple bundle and/or manifest goals are configured for the same project. The current behaviour to throw illegal argument exception is not exactly user friendly and does not help to identify and fix the root cause of the problem.

— Reply to this email directly or view it on GitHub https://github.com/tesla/m2eclipse-tycho/pull/19#issuecomment-50377027.

Greg Amerson Liferay Developer Tools Liferay, Inc. www.liferay.com

ifedorenko commented 10 years ago

I don't think it matters much if the error markers are created in configure() or configureRawClasspath(), so whichever is easier to implement.

generateBundleManifest() should do nothing if there are multiple bundle/manifest executions. AbstractMavenBundlePluginProjectConfigurator#getBuildParticipant should do nothing too, I think.

gamerson commented 10 years ago

Gotcha, here you go: https://github.com/tesla/m2eclipse-tycho/pull/20

On Tue, Jul 29, 2014 at 3:12 AM, ifedorenko notifications@github.com wrote:

I don't think it matters much if the error markers are created in configure() or configureRawClasspath(), so whichever is easier to implement.

generateBundleManifest() should do nothing if there are multiple bundle/manifest executions. AbstractMavenBundlePluginProjectConfigurator#getBuildParticipant should do nothing too, I think.

— Reply to this email directly or view it on GitHub https://github.com/tesla/m2eclipse-tycho/pull/19#issuecomment-50384491.

Greg Amerson Liferay Developer Tools Liferay, Inc. www.liferay.com