List OSGI plugins in the export list #799

alshamams · 2023-10-13T06:07:56Z

The commit fixes OSGi plugins (that have been created using standard OSGi framework with 'Generate OSGi metadata automatically' feature) not being displayed in the export dialog box. This happens because an OSGi project has a pde.bnd file, unlike the eclipse plugins, which has build.properties. Therefore the previous behaviour was to check for the presence of a build.properties file for validating a plugin project. This assumption breaks with the new type.

The commit relies on the following assumptions in case of OSGi template projects:

The location of the pde.bnd file is always under the root folder of the plugin project
The filename is always constant, namely, pde.bnd.

Fixes: #600

github-actions · 2023-10-13T06:13:54Z

Test Results

    270 files ±0     270 suites ±0 1h 15m 29s ⏱️ + 8m 2s
  3 327 tests ±0   3 297 ✔️ ±0 30 💤 ±0 0 ❌ ±0
10 278 runs ±0 10 188 ✔️ ±0 90 💤 ±0 0 ❌ ±0

Results for commit 4904fda. ± Comparison against base commit 6465a65.

♻️ This comment has been updated with latest results.

gireeshpunathil · 2023-10-15T04:41:38Z

the second assumption (instruction file name is always pde.bnd) is definitely right due to:

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/natures/BndProject.java

Lines 24 to 26 in 27cf508

    
           public static final String INSTRUCTIONS_FILE_EXTENSION = ".bnd"; //$NON-NLS-1$ 
        
           public static final String INSTRUCTIONS_FILE = "pde" + INSTRUCTIONS_FILE_EXTENSION; //$NON-NLS-1$

the first assumption (the project structure) looks to be correct too, but pinging @laeubi to confirm just in case.

laeubi · 2023-10-15T09:16:13Z

Yes pde.bnd in the project root is a "marker file" that should always be constant.

HannesWell · 2023-10-20T17:44:40Z

...g.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/wizards/exports/PluginExportWizardPage.java

+		File bin = new File(model.getInstallLocation());
+		if (bin.exists()) {
+			File file = new File(bin.getParent(), ICoreConstants.BND_FILENAME_DESCRIPTOR);
+			return file.exists();
+		}
+		return false;


Wouldn't it be better to operate on IResources here?
I.e. check if model.getUnderlyingResource() returns non null and then check for the pde.bnd file of that resource's project?

Furthermore it is actually not necessary to check for the existance of the directory and file.
File, just like Path or IFile/IFolder/... are just handles/pointers and one can create them regardless of if they exist.
Just checking if the finally constructed (I)File is an existing file saves one call to the file-system (given the file exists).

If you have a project handle one can also use org.eclipse.pde.internal.core.natures.BndProject.isBndProject(IProject) (that will only validate the nature) or org.eclipse.pde.internal.core.bnd.BndProjectManager.getBndProject(IProject) if also want to validate is has a valid instruction file.

Wouldn't it be better to operate on IResources here?
I.e. check if model.getUnderlyingResource() returns non null and then check for the pde.bnd file of that resource's project?

As far as I can see on the callerside the project is already known!

Furthermore it is actually not necessary to check for the existance of the directory and file. File, just like Path or IFile/IFolder/... are just handles/pointers and one can create them regardless of if they exist. Just checking if the finally constructed (I)File is an existing file saves one call to the file-system (given the file exists).

I have implemented the changes suggested by @laeubi , which has made these changes unnecessary, and also doesn't attempt file access.

If you have a project handle one can also use org.eclipse.pde.internal.core.natures.BndProject.isBndProject(IProject) (that will only validate the nature) or org.eclipse.pde.internal.core.bnd.BndProjectManager.getBndProject(IProject) if also want to validate is has a valid instruction file.

I have implemented the changes, kindly look through and let me know @laeubi .

laeubi · 2023-10-27T05:41:40Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java

@@ -60,6 +60,9 @@ public interface ICoreConstants {
 	/** Constant for the string <code>build.properties</code> */
 	String BUILD_FILENAME_DESCRIPTOR = "build.properties"; //$NON-NLS-1$

+	/** Constant for the string <code>pde.bnd</code> */
+	String BND_FILENAME_DESCRIPTOR = "pde.bnd"; //$NON-NLS-1$


There is already org.eclipse.pde.internal.core.natures.BndProject.INSTRUCTIONS_FILE so you should at laest reference it here

Suggested change

String BND_FILENAME_DESCRIPTOR = "pde.bnd"; //$NON-NLS-1$

String BND_FILENAME_DESCRIPTOR = BndProject.INSTRUCTIONS_FILE;

laeubi · 2023-10-27T05:42:33Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java

@@ -60,6 +60,9 @@ public interface ICoreConstants {
 	/** Constant for the string <code>build.properties</code> */
 	String BUILD_FILENAME_DESCRIPTOR = "build.properties"; //$NON-NLS-1$

+	/** Constant for the string <code>pde.bnd</code> */


This seems not used anymore in your PR maybe revert this change?

laeubi · 2023-10-27T13:14:16Z

@alshamams can you please rebase/squash and cleanup the mentioned no longer needed part? The I think this can be merged.

alshamams · 2023-10-31T09:45:46Z

@alshamams can you please rebase/squash and cleanup the mentioned no longer needed part? The I think this can be merged.

Hi @laeubi ,
I have reverted the change and rebased after squashing.

laeubi

For me it looks all sane @HannesWell @gireeshpunathil any concerns to merge?

HannesWell

No concern from me, just a minor, Not required suggestion.

...g.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/wizards/exports/PluginExportWizardPage.java

HannesWell · 2023-11-01T18:45:56Z

Thanks @alshamams please squash all commits into one, then we can submit this.

The commit fixes OSGi plugins (that have been created using standard OSGi framework with 'Generate OSGi metadata automatically' feature) not being displayed in the export dialog box. This happens because an OSGi project has a pde.bnd file, unlike the eclipse plugins, which has build.properties. Therefore the previous behaviour was to check for the presence of a build.properties file for validating a plugin project. This assumption breaks with the new type.

alshamams · 2023-11-01T19:12:02Z

Thanks @alshamams please squash all commits into one, then we can submit this.

Done @HannesWell

HannesWell

Thanks @alshamams.

In general you can actually always squash locally or just amend existing commits and force push as one commit (unless you eventually want to make a change with multiple commits that are self-contained by themself).
After force-push you can still see in the Github UI what as changed by clicking the Compare button in the feed:

Since Gireesh and Christoph were fine with this change too, I'll submit it as soon as the build completes.

gireeshpunathil approved these changes Oct 15, 2023

View reviewed changes

HannesWell reviewed Oct 20, 2023

View reviewed changes

laeubi reviewed Oct 27, 2023

View reviewed changes

alshamams force-pushed the plugin_export_bugfix branch from b03916d to d8751a7 Compare October 31, 2023 09:22

laeubi approved these changes Oct 31, 2023

View reviewed changes

HannesWell approved these changes Oct 31, 2023

View reviewed changes

...g.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/wizards/exports/PluginExportWizardPage.java Outdated Show resolved Hide resolved

alshamams force-pushed the plugin_export_bugfix branch from f7064ef to 4904fda Compare November 1, 2023 19:09

HannesWell approved these changes Nov 1, 2023

View reviewed changes

HannesWell merged commit 3ed2ae9 into eclipse-pde:master Nov 1, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List OSGI plugins in the export list #799

List OSGI plugins in the export list #799

alshamams commented Oct 13, 2023

github-actions bot commented Oct 13, 2023 •

edited

Loading

gireeshpunathil commented Oct 15, 2023

laeubi commented Oct 15, 2023

HannesWell Oct 20, 2023

HannesWell Oct 20, 2023

laeubi Oct 22, 2023

laeubi Oct 22, 2023

alshamams Oct 26, 2023

alshamams Oct 26, 2023

laeubi Oct 27, 2023

laeubi Oct 27, 2023

laeubi commented Oct 27, 2023

alshamams commented Oct 31, 2023

laeubi left a comment

HannesWell left a comment

HannesWell commented Nov 1, 2023

alshamams commented Nov 1, 2023

HannesWell left a comment

	String BND_FILENAME_DESCRIPTOR = "pde.bnd"; //$NON-NLS-1$
	String BND_FILENAME_DESCRIPTOR = BndProject.INSTRUCTIONS_FILE;

List OSGI plugins in the export list #799

List OSGI plugins in the export list #799

Conversation

alshamams commented Oct 13, 2023

github-actions bot commented Oct 13, 2023 • edited Loading

Test Results

gireeshpunathil commented Oct 15, 2023

laeubi commented Oct 15, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi commented Oct 27, 2023

alshamams commented Oct 31, 2023

laeubi left a comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell commented Nov 1, 2023

alshamams commented Nov 1, 2023

HannesWell left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2023 •

edited

Loading