Add project configurator for tycho packagings #967

kwin · 2022-10-05T18:21:54Z

This closes #605

kwin · 2022-10-05T18:22:28Z

org.eclipse.m2e.pde.connector/META-INF/MANIFEST.MF

@@ -13,5 +13,7 @@ Require-Bundle: org.eclipse.m2e.core;bundle-version="[2.0.0,3.0.0)",
 org.eclipse.core.resources;bundle-version="3.16.0",
 org.eclipse.m2e.maven.runtime;bundle-version="[3.8.6,4.0.0)",
 org.eclipse.pde.core,
- org.eclipse.jdt.core
+ org.eclipse.jdt.core,
+ org.eclipse.pde.ds.annotations;bundle-version="1.2.500",


don't know which version range to take here

Since you are using mainly internal classes any version range that does allow a future version has the potential to break.
IIRC in that case it is recommended to just specify a lower bound as you already did.

In fact I only use constant which are interned at build time, so this is not a real run time requirement, but I haven't found a way to tell PDE to only load a package in the build classpath....

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

HannesWell

Thank you for this PR.
Additionally this should also help for #929.

I applied a few comments below, but in general I think this is on a good way.

HannesWell · 2022-10-05T22:28:48Z

org.eclipse.m2e.pde.connector/META-INF/MANIFEST.MF

@@ -13,5 +13,7 @@ Require-Bundle: org.eclipse.m2e.core;bundle-version="[2.0.0,3.0.0)",
 org.eclipse.core.resources;bundle-version="3.16.0",
 org.eclipse.m2e.maven.runtime;bundle-version="[3.8.6,4.0.0)",
 org.eclipse.pde.core,
- org.eclipse.jdt.core
+ org.eclipse.jdt.core,
+ org.eclipse.pde.ds.annotations;bundle-version="1.2.500",


Since you are using mainly internal classes any version range that does allow a future version has the potential to break.
IIRC in that case it is recommended to just specify a lower bound as you already did.

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

org.eclipse.m2e.pde.connector/lifecycle-mapping-metadata.xml

org.eclipse.m2e.pde.connector/plugin.xml

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

fix configuration

clean up plugin.xml

github-actions · 2022-10-06T08:11:47Z

Unit Test Results

596 tests 590 ✔️ 9m 6s ⏱️
  94 suites     6 💤
  94 files     0 ❌

Results for commit f60fc82.

♻️ This comment has been updated with latest results.

laeubi · 2022-10-06T08:56:54Z

@HannesWell any more comments? For me it looks like a good first addition, we might improve/enhance this later on ...

HannesWell

I have a few more remarks below.

Furthermore please format the class with the standard Eclipse built-in formatter to align it with the other classes in this plugin. Mainly to use tabs instead of spaces for indentation. If you use the M2E-setup in a clean workspace, that should be set up by default.

In general it would be good to have tests (for all configurations in this Plug-in).
If you want to create some that would be great, otherwise I can do that in the next days.

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

get rid of SLF4J dependency some simplifications in code

kwin · 2022-10-07T17:44:39Z

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

+		MavenProject mavenProject = request.mavenProject();
+		IProject project = request.mavenProjectFacade().getProject();
+		String packaging = mavenProject.getPackaging();
+		if ("eclipse-plugin".equals(packaging) || "eclipse-test-plugin".equals(packaging)) {


probably there should also test class folders be added to the Java Build Path (compare with eclipse-tycho/tycho#1491). Is there any recommended code layout for unit tests inside eclipse-plugin?

Is there any recommended code layout for unit tests inside eclipse-plugin?

You mean in general or for testing this connector?
At the moment I don't think there is a general recommendation since Plug-in projects cannot (yet) specify test-dependencies and the usual pattern is to separate tests into fragments.
However I would make it similar to the maven structure and have a separate src-test or test-src folder where the sources are placed.

probably there should also test class folders be added to the Java Build Path (compare with eclipse-tycho/tycho#1491).

For a eclipse-test-plugin that would be reasonable, yes.
Do you want to include that into this PR or create a follow-up?

eclipse-plugin-test is nothing we should specifically support as it is actually nothing special and has only a meaning when executing Tycho. Even there I always would recommend to use eclipse-test instead.

Whether or not a folder is meant as a test-folder can not be derived from the Tycho configuration, you can guess it from the standard maven layout, but m2e already does that.

eclipse-plugin-test is nothing we should specifically support as it is actually nothing special and has only a meaning when executing Tycho. Even there I always would recommend to use eclipse-test instead.

Is eclipse-test a new packaging type? I don't know about that.

However since that topic seems not yet settled, lets merge this PR now and improve/enhance in with follow up PRs.

Is eclipse-test a new packaging type? I don't know about that.

Should be eclipse-plugin with test folder ;-)

Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>

HannesWell

Looks good now, thank you.

I just applied a few minor adjustments and clean-ups myself.
The most relevant are probably that the warning about multiple executions is added to the first and chosen one (which is then more clear for the user) and wrapping the Version creation into a try-catch block to handle invalid version strings.

Add project configurator for tycho packagings

14d4c67

This closes eclipse-m2e#605

kwin commented Oct 5, 2022

View reviewed changes

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

HannesWell requested changes Oct 5, 2022

View reviewed changes

laeubi reviewed Oct 6, 2022

View reviewed changes

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

laeubi reviewed Oct 6, 2022

View reviewed changes

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

laeubi reviewed Oct 6, 2022

View reviewed changes

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

laeubi reviewed Oct 6, 2022

View reviewed changes

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

kwin mentioned this pull request Oct 6, 2022

Clarify values for "declarative-services" mojo parameter "dsVersion" eclipse-tycho/tycho#1473

Closed

Apply feedback from PR

966c92e

fix configuration

kwin marked this pull request as ready for review October 6, 2022 07:54

fix version

bbac791

clean up plugin.xml

kwin requested review from laeubi and HannesWell and removed request for laeubi and HannesWell October 6, 2022 08:22

HannesWell reviewed Oct 7, 2022

View reviewed changes

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

emit markers for warnings

b30f501

get rid of SLF4J dependency some simplifications in code

kwin requested review from HannesWell and removed request for laeubi October 7, 2022 08:56

kwin commented Oct 7, 2022

View reviewed changes

Minor adjustments and clean-ups

f60fc82

Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>

HannesWell approved these changes Oct 7, 2022

View reviewed changes

HannesWell merged commit 0223a44 into eclipse-m2e:master Oct 8, 2022

HannesWell added this to the 2.1.0 milestone Oct 22, 2022

kwin mentioned this pull request Nov 1, 2022

m2e compatibility list #987

Open

lppedd mentioned this pull request Nov 6, 2022

Package m2e tycho extension to smooth import of pde plugins testforstephen/vscode-pde#41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add project configurator for tycho packagings #967

Add project configurator for tycho packagings #967

kwin commented Oct 5, 2022

kwin Oct 5, 2022

HannesWell Oct 5, 2022

kwin Oct 6, 2022

HannesWell left a comment

HannesWell Oct 5, 2022

github-actions bot commented Oct 6, 2022 •

edited

Loading

laeubi commented Oct 6, 2022

HannesWell left a comment

kwin Oct 7, 2022

HannesWell Oct 7, 2022

laeubi Oct 8, 2022

HannesWell Oct 8, 2022

laeubi Oct 8, 2022

HannesWell left a comment

Add project configurator for tycho packagings #967

Add project configurator for tycho packagings #967

Conversation

kwin commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2022 • edited Loading

Unit Test Results

laeubi commented Oct 6, 2022

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2022 •

edited

Loading