Bug 564868 - Support Maven Artifacts in PDE-Target Platform

[laeubi]

This adds support for maven artifacts as PDE target platform items. I'm not sure how I made acomplete build and test this inside eclipse, but starting from inside eclipse works as expected (due to a bug in PDE one needs to mark the ui bundle as autostart).

The following features are included:

Add a plain bundle artifact from maven

Add a bundle artifact from maven including its dependencies

There are three choices

get an error when any item is not already a bundle

ignore artifacts with missing bundle-data

missing artifacts will then of course not be part of the target

add missing meta-data automatically

plain jar will be displayed with a jar symbol and bundles with a plugin symbol when "show location content" is selected.

[mickaelistria]

That's great progress! How can one control from the .target file which Maven repositories are accessible?
Similarly to Tycho, I don't think bundle should be named . without some prefix to hint about the source.

[laeubi]

I tried to use the m2e settings so whats configure there should be used here. As the target file itself does not (necessarily) have any maven context the support might be rather basic and only use global configured repositories.

My plan is to enhance the PDE support in such a way that more is possible later (notably inherit p2 repositories defined in pom files) and the it might be possible to inherit from the parent project some data.

The only thing I can think of right now would be to use the name of the target or the name of the project where the target is located in. Both suffer a bit IMO as the can contain any characters and do not necessary form a good BSN prefix.
I could still add a prefix like "pde.target" or something... I'll at least add the original maven coordinates as the Bundle Name.

[laeubi]

Any idea why FileCompletionTest fails?

[mickaelistria]

No need to worry about FileCompletionTests, they started failing with an upgrade to newer WTP.

[laeubi]

@mickaelistria as the contribution is larger than 1k LOC it seems a review from the IP team is required, can you schedule one for this PR?

[mickaelistria]

I tried to use the m2e settings so whats configure there should be used here. As the target file itself does not (necessarily) have any maven context the support might be rather basic and only use global configured repositories.

Wouldn't it be better if instead of 1 root entry per Maven artifact, it would be 1 root entry "Maven artifacts" with multiple possible artifacts and repository children elements?

I could still add a prefix like "pde.target" or something...

Maybe packed.by.pde as prefix or suffix?

[laeubi]

The problem with one root is that it either limits you to "one must fit all" or a very complex configuration wizard/serialization format.
Also I hoped to reuse as much as possible of the current m2e configuration to have a uniform view and a more seaming less integration with maven projects. e.g one can think about repositories configured in the pom.xml of the target project (or its parent).
If the project itself is not a maven one simply the workspace maven defaults (user settings, global settings) would come into effect.

Another point is, that if each entry is just one dependency it nicely maps to the section of a regular pom so it is easy to integrate this kind of location e.g. with tycho without the need to support additional configuration that might interfere.

[laeubi]

I have added a prefix now to the BSN

What I like about the prefix is that it hopefully let people use import-package instead of require bundle :-)

[mickaelistria]

What I like about the prefix is that it hopefully let people use import-package instead of require bundle :-)

👍

Also I hoped to reuse as much as possible of the current m2e configuration to have a uniform view

The issue is that the .target and the m2e configuration are totally not correlated: several Maven projects can use the same target, but have different Maven repos available, so that would result in different resolution according to the context; while PDE target platform is a singleton shared by all bundles in the workspace without contextual project-specific settings considered.
Is there anything I'm missing, or do you confirm this is an issue? (not necessarily a blocker that said)

If the project itself is not a maven one simply the workspace maven defaults (user settings, global settings) would come into effect.

Because of the above, I think it should currently be the only supported source to resolve artifacts, ignoring project specific settings.

[laeubi]

The issue is that ... PDE target platform is a singleton shared by all bundles in the workspace

I think that's the real issue I'd like to address hopefully in the future but changing PDE here might take some time ... my vision would be that m2e understands tycho (+pomless) maven builds and adds a project specific target configuration to the project as it is done with the ME2 Container.

I think it should currently be the only supported source to resolve artifacts, ignoring project specific settings

Yep at the moment only global maven settings are taken into account, at least if I understand the m2e code correctly (I'm using MavenPlugin.getMaven().getArtifactRepositories() here).

Of course this feature can be enhanced later on, but for now I don't want to add to much extra features here without additional support from PDE/Tycho.

[mickaelistria]

I think we can merge it now, but I prefer nitpicking before the merge:

• Wouldn't it be better to use wrapped.by.pde instead of packed.by.pde ?
• The combo for "Missing metadata: automated" is IMO unclear (same as Tycho discussion earlier). I'd suggest "On missing OSGi MANIFEST.MF: generate".
• If possible, merge the "include dependencies" and "dependency scope" so it would be "include dependencies with scope" and the combo would allow none. I think this may have implication on the underlying model and serialization.

[laeubi]

wrapped.by.pde

I fact m2e is supplying this to pde as a plugin, maybe just throw away the whole 'by' part and simply use 'wrapped.<...>'

I'll try to adjust the wizard as well.

WDYT about the possible IP issue? eclipsefdn/eca check suggest an extra review for contributions > 1k LOCs

[laeubi]

[mickaelistria]

Since this .target resolution is only devtime and is not very likely to leak its artifacts into the wild (they'll remain in user development area), it's OK to stick to wrapped.<...>

[mickaelistria]

Opened CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22669