#439 - Adopt PDE configurations for maven-bundle-plugins #440

laeubi · 2021-12-09T19:15:38Z

This add initial code contribution and project setup to m2e. This code is copied from (with slight adjustments e.g. package name and constant inlining):

both files are EPL 1.0 and the contribution is below 1000 LOC (including configuration files), so I would assume no IP review is required, but like to get a verification by @mickaelistria and @fbricon as project lead.

If the legal part is done I'd like to test and maybe enhance the code later on, e.g. currently I have only used it with felix plugin but the same should apply to the bnd-bundle plugin.

mickaelistria

We'll need to check with IP team whether it's fine to just pick some other EPL code here.
I'm a bit worried about the file with missing copyright header being a blocker.

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

akurtakov · 2021-12-10T09:22:50Z

Whenever new code that is not submitted by author and is not coming from project governed by EF a CQ has to be open. Please open new one of type Project Content (Content to be maintained by an Eclipse Foundation Project) and refer to this PR.

laeubi · 2021-12-10T09:40:58Z

I opened https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23882

laeubi · 2021-12-10T11:01:28Z

The CQ require PMC approval: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23882

laeubi · 2021-12-10T16:35:15Z

The CQ require PMC approval

if @fbricon responsible for PMC for m2e? Can we ping someone else if not?

add initial code contribution and project setup Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>

HannesWell · 2021-12-10T18:30:06Z

Is it necessary to place the connector in a dedicated plug-in project?
Wouldn't it be possible to have it in the o.e.m2e.pde plug-in in a package named org.eclipse.m2e.pde.connector?
The TP related classes maybe would be better moved in their own package, o.e.m2e.pde.target (in a separate PR)?

laeubi · 2021-12-12T06:42:11Z

Is it necessary to place the connector in a dedicated plug-in project?

I think it is not strictly necessary but I don't see a reason why a dedicated plugin would harm here. Even though its unlikely if one likes to have the maven-bundle support but not the target support it could be installed separately.

HannesWell · 2021-12-12T12:54:14Z

Is it necessary to place the connector in a dedicated plug-in project?

I think it is not strictly necessary but I don't see a reason why a dedicated plugin would harm here. Even though its unlikely if one likes to have the maven-bundle support but not the target support it could be installed separately.

I don't consider it harmful as well.
But having more plug-ins just fills the dev-workspace and adds management overhead (more versions to bump, more metadata files, etc.). Of course only very few big plug-ins is probably as bad too many very small plug-ins. So apart from the technical boundaries/requirements (like separate installability, dependencies) the decision for a new plug-in it should be weigh up sensibly. And IMHO in this case a new plug-in is not worth it. I think it would be better to introduce multiple packages in org.eclipse.m2e.pde.
But this is of course also a matter of preferences/habits, so there is not one right solution.

laeubi · 2021-12-12T13:06:33Z

I don't think any "count" is a valid measure of putting code into separate plugins. Especially when it comes to (semantic) versioning, smaller items are generally better than bigger ones. Just because both contribute to PDE don't make them related. The target part contributes to the target handling of pde and this contributes to the m2e connectors so this are two separate concerns that should be live in separate plugins.

HannesWell · 2021-12-12T13:50:53Z

I don't think any "count" is a valid measure of putting code into separate plugins. Especially when it comes to (semantic) versioning, smaller items are generally better than bigger ones. Just because both contribute to PDE don't make them related. The target part contributes to the target handling of pde and this contributes to the m2e connectors so this are two separate concerns that should be live in separate plugins.

That also makes sense.
But then the o.e.m2e.pde should be renamed to o.e.m2e.pde.target (or similar) as well as the corresponding ui plug-in to reflect the fact that it is dedicated to that concern. Otherwise the name is actually too general. But that's off topic of this PR.

laeubi · 2021-12-13T14:00:06Z

But then the o.e.m2e.pde should be renamed to o.e.m2e.pde.target

probably yes, but this would require using some kind of patch feature to make P2 aware of it (I have never used that) so for now I won't complicate it too much just for the sake of naming.

HannesWell

Just tested it with a little Maven-project that uses the maven-bundle-plugin:manifest goal and it worked very well.

There is just an error markers in the Dependencies-Tab of the Manifest-Editor, even tough the plug-in/package from the Maven-project was proposed by the Add... dialogues (for Require-Bundle and Import-Package). So I think this is an PDE issue.

I also like the warnings, in the POM regarding the missing configuration. This is very helpful.

Some parts of the code could be simplified using present-day Java-11 methods. But I think this should be done in a subsequent PR, so that the initial move to Eclipse has as few changes as possible and we get a more clear git-history.

org.eclipse.m2e.pde.connector/build.properties

HannesWell · 2021-12-14T08:35:25Z

But then the o.e.m2e.pde should be renamed to o.e.m2e.pde.target

probably yes, but this would require using some kind of patch feature to make P2 aware of it (I have never used that) so for now I won't complicate it too much just for the sake of naming.

For the installation process an uninstallBundle p2-touchpoint instruction could be used. But another issue would be down-stream users that use require the m2e.pde bundle or import a package from it. At the moment the content is de-facto public API because it is not internal.

Which gives me another question: Is the code in this new m2e.pde.connector plug-in meant to be public API or should it better be internal?

laeubi · 2021-12-14T08:42:28Z

I consider none of the code in m2e.pde.* as public API beside that is very unlikely that anyone is already enhancing that stuff right now :-)

Don't know if one could mark a bundle as "internal" but I really don't like these eclipse extension as they are extremely confusing and against OSGi principles.

laeubi · 2021-12-14T11:13:14Z

There is just an error markers in the Dependencies-Tab of the Manifest-Editor, even tough the plug-in/package from the Maven-project was proposed by the Add... dialogues (for Require-Bundle and Import-Package). So I think this is an PDE issue.

Will you take a look at this?

I sometimes noted that PDE also marks bundles with unresolved requirements with such an error marker, e.g. if your maven-bundle import-package a dependency but this is not available at the current target platform (I opened Bug 577605 for such cases). If you double click the bundle it should open the manifest and show if there is any unresolved package.

laeubi · 2021-12-14T11:34:43Z

Some parts of the code could be simplified using present-day Java-11 methods. But I think this should be done in a subsequent PR, so that the initial move to Eclipse has as few changes as possible and we get a more clear git-history.

Yep exactly main goal is to get the code in even though it seems we would have been faster If I have rewritten it from scratch as it seem no one of the PMC likes to approve the CQ :-(

HannesWell · 2021-12-14T22:43:09Z

I consider none of the code in m2e.pde.* as public API beside that is very unlikely that anyone is already enhancing that stuff right now :-)

Don't know if one could mark a bundle as "internal" but I really don't like these eclipse extension as they are extremely confusing and against OSGi principles.

Are you referring to the package name element .internal. or the Manifest header-arguments ;x-internal/friend in the Manifest?
Personally I also don't like the internal-package names but at least it is a clear statement.

However I have noticed that this plug-in does not export its only package so it is not public (regardless of the name), so sorry for the disturbance in this regard. To keep the off-scope discussion about o.e.m2e.pde out of this PR I have created #454, we should continue there if there is more to say.

HannesWell · 2021-12-14T22:54:39Z

There is just an error markers in the Dependencies-Tab of the Manifest-Editor, even tough the plug-in/package from the Maven-project was proposed by the Add... dialogues (for Require-Bundle and Import-Package). So I think this is an PDE issue.

Will you take a look at this?

I can, but at the moment I have too many other things on my list (in my mind), that I would like to do before. So if this flaw persists and nobody else takes care of it, I can do it later :)

Some parts of the code could be simplified using present-day Java-11 methods. But I think this should be done in a subsequent PR, so that the initial move to Eclipse has as few changes as possible and we get a more clear git-history.

Yep exactly main goal is to get the code in even though it seems we would have been faster If I have rewritten it from scratch as it seem no one of the PMC likes to approve the CQ :-(

Aren't @akurtakov and @vogella in the Eclipse PMC? Could you be so kind and have a look at this or refer to somebody else from PMC?

mickaelistria · 2021-12-15T08:52:26Z

Aren't @akurtakov and @vogella in the Eclipse PMC? Could you be so kind and have a look at this or refer to somebody else from PMC?

m2e is a project under technology. PMC members for Technology are listed at https://projects.eclipse.org/projects/technology/who .

HannesWell · 2021-12-16T14:58:46Z

m2e is a project under technology. PMC members for Technology are listed at https://projects.eclipse.org/projects/technology/who .

Thanks Mickael for that hint, I already wondered if there is more than one PMC, because I have the impression that the term Project is used redundantly.

So could @guw, @caniszczyk or @waynebeaton be so kind and have a look at the associated CQ at
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23882

org.eclipse.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/Activator.java

HannesWell

I tested this PR with the pure Maven m2e's maven-runtime projects as proposed in PR #466 and found one problem with clean and full builds:

In the beginning of a clean/full build the target/classes folder is purged and with it the META-INF/MANIFEST.MF file located within it. At the same time target/maven-bundle-plugin/org.apache.felix_maven-bundle-plugin_manifest_xx is not touched. Consequently the manifest Mojo does not re-generate the MANIFEST.MF in the build when supportIncrementalBuild is enabled, because the mojo only checks org.apache.felix_maven-bundle-plugin_manifest_xx and not if the MANIFEST.MF is present.
To workaround this, org.apache.felix_maven-bundle-plugin_manifest_xx has to be target/maven-bundle-plugin in each clean/full build before the manifest Mojo is executed.

HannesWell · 2021-12-28T19:36:04Z

org.eclipse.m2e.pde.feature/feature.xml

@@ -2,7 +2,7 @@
 <feature
 id="org.eclipse.m2e.pde.feature"
 label="%featureName"
- version="1.19.0.qualifier"
+ version="1.20.0.qualifier"


The minor version of m2e itself (i.e. the m2e-sdk feature) should also be bumped to be 20.
Actually I expected the tycho-p2-extras-plugin:compare-version-with-baselines goal to consider minor and major versions too.

HannesWell · 2021-12-28T22:18:53Z

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

+ }
+
+ private boolean isFelixBundleGoal(MojoExecution execution) {
+ return FELIX_MANIFEST_GOAL.equals(execution.getGoal());


The name is actually misleading because it checks on the manifest goal. A name like isFelixManifestGoal() would be more suitable.

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

laeubi · 2021-12-29T15:06:11Z

the manifest Mojo does not re-generate the MANIFEST.MF in the build when supportIncrementalBuild is enabled, because the mojo only checks org.apache.felix_maven-bundle-plugin_manifest_xx and not if the MANIFEST.MF is present.

I think this is a bug in the Felix Bundle plugin, if you agree, would you mind to open a ticket at felix (and maybe provide a PR to fix this)?

HannesWell · 2021-12-29T15:59:24Z

the manifest Mojo does not re-generate the MANIFEST.MF in the build when supportIncrementalBuild is enabled, because the mojo only checks org.apache.felix_maven-bundle-plugin_manifest_xx and not if the MANIFEST.MF is present.

I think this is a bug in the Felix Bundle plugin, if you agree, would you mind to open a ticket at felix (and maybe provide a PR to fix this)?

Fully agree that this is a bug in the Felix maven-bundle-plugin. I can create an issue and provide a PR (I wanted to report/fix some other minor issues anyway, so now I have one more reason).
But if if this is fixed only future versions of the maven-bundle-plugin will work correctly.
It's probably discouraged to provide a workaround for old versions, but to make users aware of it we could emit a warning when a flawed version is used.

laeubi · 2021-12-29T16:09:14Z

But if if this is fixed only future versions of the maven-bundle-plugin will work correctly.

I won't mind to only support latest version given that this was flawed before and we have only limited dev-resources. I even don't see a reason to use old versions of the bundle-plugin... We should also not complicate the code with to much else/if/workaround/... and simply tell people when complain to use always the latest version of the plugin.

HannesWell

the manifest Mojo does not re-generate the MANIFEST.MF in the build when supportIncrementalBuild is enabled, because the mojo only checks org.apache.felix_maven-bundle-plugin_manifest_xx and not if the MANIFEST.MF is present.

I think this is a bug in the Felix Bundle plugin, if you agree, would you mind to open a ticket at felix (and maybe provide a PR to fix this)?

I opened issue Apache FELIX-6493 and submitted PR apache/felix-dev#124. Fell free to review it.

With those fixes the suggested workaround is not necessary any more and the manifest generation works well and even the error markers at the Maven-OSGi-Projects in the Manifest-Editor dependencies tab respectively the Target-Platform-State view magically vanished (I'm not sure why exactly and if this is permanently).

But if if this is fixed only future versions of the maven-bundle-plugin will work correctly.

I won't mind to only support latest version given that this was flawed before and we have only limited dev-resources. I even don't see a reason to use old versions of the bundle-plugin... We should also not complicate the code with to much else/if/workaround/... and simply tell people when complain to use always the latest version of the plugin.

I agree that we should not make it too complicated and a workaround is not necessary, but I think a warning would make the user experience much better. Especially because it could take some time until the new maven-bundle-plugin is available and is widely adopted. I think it would be a pity if this great feature would not work in the beginning and users first have to search the internet for a solution while we could simply give them a hint with a corresponding warning. :)

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

HannesWell · 2021-12-30T13:02:59Z

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

+ if (problems.size() > 0) {
+ this.markerManager.addErrorMarkers(request.getPom(), IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problems);
+ }


This code does not consider that the resource of a problem could be the parent pom (or grand-parent etc.).
The following suggestion does consider this.

Suggested change

if (problems.size() > 0) {

this.markerManager.addErrorMarkers(request.getPom(), IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problems);

}

if (!problems.isEmpty()) {

IMavenProjectRegistry projectManager = MavenPlugin.getMavenProjectRegistry();

for (MavenProblemInfo problem : problems) {

String[] gav = problem.getLocation().getResourceId().split(":");

IFile pom = projectManager.getMavenProject(gav[0], gav[1], gav[2]).getPom();

markerManager.addErrorMarker(pom, IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problem);

}

}

HannesWell · 2021-12-30T13:05:24Z

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java

+ IMarker.SEVERITY_WARNING, location);
+ problems.add(problem);
+ }
+ hasManifestExecution = true;


The following code would give the user a warning to use the latest Maven-Bundle-Plugin (assuming the mentioned fixes will be integrated there). Maybe the message could be better.

Suggested change

hasManifestExecution = true;

MavenVersion version = MavenVersion.parseMavenString(execution.getPlugin().getVersion());

if (version != null && version.toReleaseVersion().compareTo(new MavenVersion("5.1.4")) < 0) {

SourceLocation location = SourceLocationHelper.findLocation(execution.getPlugin(), "version");

problems.add(new MavenProblemInfo(

"The Maven-Bundle-Plugin only integrates well with Eclipse-M2E with version 5.4.0 or later",

IMarker.SEVERITY_WARNING, location));

}

hasManifestExecution = true;

laeubi · 2021-12-30T13:36:52Z

I agree that we should not make it too complicated and a workaround is not necessary, but I think a warning would make the user experience much better. Especially because it could take some time until the new maven-bundle-plugin is available and is widely adopted. I think it would be a pity if this great feature would not work in the beginning and users first have to search the internet for a solution while we could simply give them a hint with a corresponding warning. :)

I think we can at least add a warning marker for older version and suggest an upgrade. But for such improvements I really want to wait until EMO approval of the code.

mickaelistria · 2022-01-04T23:02:16Z

I really want to wait until EMO approval of the code.

That's now good to merge from a legal POV according to recent comment on the CQ.

laeubi · 2022-01-05T06:40:51Z

@HannesWell I'll merge this as-is now, so the code is in the repository can you create separate PR(s) for your enhancements?

- add warnings about missing configuration options - support bnd-bundle plugin as well

HannesWell

@HannesWell I'll merge this as-is now, so the code is in the repository can you create separate PR(s) for your enhancements?

Yes, please go ahead. I'll create a PR afterwards.

laeubi requested review from mickaelistria and fbricon December 9, 2021 19:17

laeubi force-pushed the issue_439 branch 3 times, most recently from 5d306a2 to 70dbb2f Compare December 10, 2021 07:25

mickaelistria marked this pull request as ready for review December 10, 2021 09:13

mickaelistria requested changes Dec 10, 2021

View reviewed changes

...se.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/PDEMavenBundlePluginConfigurator.java Show resolved Hide resolved

laeubi force-pushed the issue_439 branch from 70dbb2f to 95b252e Compare December 10, 2021 16:32

eclipse-m2e#439 - Adopt PDE configurations for maven-bundle-plugins

48631df

add initial code contribution and project setup Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>

laeubi force-pushed the issue_439 branch from 95b252e to 48631df Compare December 10, 2021 16:39

laeubi requested a review from mickaelistria December 10, 2021 16:40

HannesWell requested changes Dec 14, 2021

View reviewed changes

org.eclipse.m2e.pde.connector/build.properties Outdated Show resolved Hide resolved

HannesWell mentioned this pull request Dec 14, 2021

[PDE] mark package org.eclipse.m2e.pde as internal #454

Merged

HannesWell approved these changes Dec 14, 2021

View reviewed changes

vogella reviewed Dec 16, 2021

View reviewed changes

org.eclipse.m2e.pde.connector/src/org/eclipse/m2e/pde/connector/Activator.java Show resolved Hide resolved

HannesWell requested changes Dec 29, 2021

View reviewed changes

HannesWell mentioned this pull request Dec 29, 2021

[Maven-Runtime] convert projects into standard Maven-projects (#423) #466

Merged

HannesWell requested changes Dec 30, 2021

View reviewed changes

laeubi force-pushed the issue_439 branch from 8da2083 to 0aa889f Compare January 5, 2022 06:44

eclipse-m2e#439 - Adopt PDE configurations for maven-bundle-plugins

671ff1d

- add warnings about missing configuration options - support bnd-bundle plugin as well

laeubi force-pushed the issue_439 branch from 0aa889f to 671ff1d Compare January 5, 2022 06:46

HannesWell approved these changes Jan 5, 2022

View reviewed changes

laeubi merged commit 0493981 into eclipse-m2e:master Jan 5, 2022

HannesWell mentioned this pull request Jan 5, 2022

[PDE-Connector] support warning in parent, improve code and clean-up #479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#439 - Adopt PDE configurations for maven-bundle-plugins #440

#439 - Adopt PDE configurations for maven-bundle-plugins #440

laeubi commented Dec 9, 2021 •

edited

Loading

mickaelistria left a comment

akurtakov commented Dec 10, 2021

laeubi commented Dec 10, 2021

laeubi commented Dec 10, 2021

laeubi commented Dec 10, 2021

HannesWell commented Dec 10, 2021

laeubi commented Dec 12, 2021

HannesWell commented Dec 12, 2021

laeubi commented Dec 12, 2021

HannesWell commented Dec 12, 2021

laeubi commented Dec 13, 2021

HannesWell left a comment

HannesWell commented Dec 14, 2021 •

edited

Loading

laeubi commented Dec 14, 2021

laeubi commented Dec 14, 2021

laeubi commented Dec 14, 2021

HannesWell commented Dec 14, 2021

HannesWell commented Dec 14, 2021

mickaelistria commented Dec 15, 2021

HannesWell commented Dec 16, 2021

HannesWell left a comment

HannesWell Dec 28, 2021

HannesWell Dec 28, 2021

laeubi commented Dec 29, 2021

HannesWell commented Dec 29, 2021

laeubi commented Dec 29, 2021

HannesWell left a comment

HannesWell Dec 30, 2021

HannesWell Dec 30, 2021

laeubi commented Dec 30, 2021

mickaelistria commented Jan 4, 2022

laeubi commented Jan 5, 2022

HannesWell left a comment •

edited

Loading

- if (problems.size() > 0) {
- this.markerManager.addErrorMarkers(request.getPom(), IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problems);
- }
+ if (!problems.isEmpty()) {
+ IMavenProjectRegistry projectManager = MavenPlugin.getMavenProjectRegistry();
+ for (MavenProblemInfo problem : problems) {
+ String[] gav = problem.getLocation().getResourceId().split(":");
+ IFile pom = projectManager.getMavenProject(gav[0], gav[1], gav[2]).getPom();
+ markerManager.addErrorMarker(pom, IMavenConstants.MARKER_LIFECYCLEMAPPING_ID, problem);
+ }
+ }

- hasManifestExecution = true;
+ MavenVersion version = MavenVersion.parseMavenString(execution.getPlugin().getVersion());
+ if (version != null && version.toReleaseVersion().compareTo(new MavenVersion("5.1.4")) < 0) {
+ SourceLocation location = SourceLocationHelper.findLocation(execution.getPlugin(), "version");
+ problems.add(new MavenProblemInfo(
+ "The Maven-Bundle-Plugin only integrates well with Eclipse-M2E with version 5.4.0 or later",
+ IMarker.SEVERITY_WARNING, location));
+ }
+ hasManifestExecution = true;

#439 - Adopt PDE configurations for maven-bundle-plugins #440

#439 - Adopt PDE configurations for maven-bundle-plugins #440

Conversation

laeubi commented Dec 9, 2021 • edited Loading

mickaelistria left a comment

Choose a reason for hiding this comment

akurtakov commented Dec 10, 2021

laeubi commented Dec 10, 2021

laeubi commented Dec 10, 2021

laeubi commented Dec 10, 2021

HannesWell commented Dec 10, 2021

laeubi commented Dec 12, 2021

HannesWell commented Dec 12, 2021

laeubi commented Dec 12, 2021

HannesWell commented Dec 12, 2021

laeubi commented Dec 13, 2021

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell commented Dec 14, 2021 • edited Loading

laeubi commented Dec 14, 2021

laeubi commented Dec 14, 2021

laeubi commented Dec 14, 2021

HannesWell commented Dec 14, 2021

HannesWell commented Dec 14, 2021

mickaelistria commented Dec 15, 2021

HannesWell commented Dec 16, 2021

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Dec 28, 2021

Choose a reason for hiding this comment

HannesWell Dec 28, 2021

Choose a reason for hiding this comment

laeubi commented Dec 29, 2021

HannesWell commented Dec 29, 2021

laeubi commented Dec 29, 2021

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Dec 30, 2021

Choose a reason for hiding this comment

HannesWell Dec 30, 2021

Choose a reason for hiding this comment

laeubi commented Dec 30, 2021

mickaelistria commented Jan 4, 2022

laeubi commented Jan 5, 2022

HannesWell left a comment • edited Loading

Choose a reason for hiding this comment

laeubi commented Dec 9, 2021 •

edited

Loading

HannesWell commented Dec 14, 2021 •

edited

Loading

HannesWell left a comment •

edited

Loading