Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for building plugins with using bnd as manifest generator #536

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Mar 28, 2023

Currently PDE only supports a "Manifest-First" approach whne building plugins, but actually most of the data in a manifest can be derived from the compiled class files and sources.

This adds a very basic aproach to support "Code-First" but still retain the basic concepts of PDE like a target platform and the integration with the existing PDE ecosystem.

How to try this out

  1. create a new java project
  2. create a file bnd.bnd in the root of the project (see below for an example)
  3. add the nature org.eclipse.pde.BndNature (this should automatically add the
<buildCommand>
	<name>org.eclipse.pde.BndBuilder</name>
</buildCommand> 

You can now configure the project in the BND file, for example similar to this:

Bundle-SymbolicName: my.first.bnd.test
Bundle-Name: This is a test showing the new auto-generated Manifest feature
Bundle-Version: 1.0.0.qualifier

-buildpath: org.eclipse.osgi,\
     ... other bundle names you want on the classpath ...

Whats not included but can be added later on based on this:

  • auto completion / basic syntax highlight
  • quickfixes to add missing buildpath entries like for a Manifest-First-Plugin
  • conversion / configuration of existing project
  • enhancing of the new project wizard to support the new type
  • open the manifest editor and show a new tab when double-click on a bnd.bnd file
  • ...

@github-actions
Copy link

github-actions bot commented Mar 28, 2023

Test Results

   222 files   -        6     222 suites   - 6   17m 35s ⏱️ - 8m 54s
   663 tests  - 2 618     644 ✔️  - 2 600  0 💤  - 24  8 ±0  11 🔥 +6 
1 927 runs   - 2 739  1 908 ✔️  - 2 721  0 💤  - 24  8 ±0  11 🔥 +6 

For more details on these failures and errors, see this check.

Results for commit 5b2131a. ± Comparison against base commit 3578bac.

This pull request removes 2624 and adds 6 tests. Note that renamed tests count towards both.
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test2
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test3
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test4
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test5
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test6
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingAnalysisAntTaskTests ‑ test7
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test1
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test2
org.eclipse.pde.api.tools.anttasks.tests.ApiToolingApiFreezeAntTaskTests ‑ test3
…
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.WorkspaceModelManagerTest ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.WorkspaceProductModelManagerTest ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.core.tests.internal.classpath.ClasspathResolutionTest ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.ui.tests.classpathresolver.ClasspathResolverTest ‑ Unknown test
AllPDEMinimalTests org.eclipse.pde.ui.tests.project.DynamicPluginProjectReferencesTest ‑ Unknown test

♻️ This comment has been updated with latest results.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very interesting.
Could you add a correspondingly set up project as test case?

I have not yet looked into the details of this change, but have some questions/thoughts:
Will the bnd-lib only be used to generate the Manifest of the project or will it also build the jar and consequently handle the inclusion of resources, which would conflict with the build.properties?

What I personally like about PDE's Manifest first approach is that it gives us the what-you-see-is-what-you-get principle. The drawback is that good metadata are hard to maintain manually and as you said often the metadata can be derived from the code, which actually means that they have to be synchronized manually. On the other hand I personally would like to avoid having another configuration file in the project (ok for bnd-only projects the manifest is probably only generated into the bin-folder or similar).
BND is great in computing good OSGi metadata, but every time I set it up for a project (actually always only using the bnd-maven-plugin/maven-bundle-plugin I end up checking the generated Manifest and since it is not nicely formatted, that's a hard task. Maybe that is because I don't use the bnd-tools and therefore lack tool support for bnd or it is just what I'm used to from PDE.

So what I personally would like to have in a bright future is a mixture of both, that the Manifest is in principle maintained manually and one can for example specify the BSN there or which packages are exported etc. But at the same time the values are enhanced/completed automatically by PDE (internally probably computed by the BND-lib) with data derived from the code, for example uses-constraints and versions for exported packages or Imported-Packages (this way they are always up to date and even easier to use than Require-Bundle). This would IMHO combine the best of both worlds and PDE users would not have to learn BND.

PDE already does that in the simple case of Service-Component headers, so it would actually not be that new, but the suggested cases are probably harder since there are likely many edge-cases to handle when merging manually and derived/computed metadata.

That being said, nothing speaks about this PR to add support for bnd files to fully compute the manifest.

@laeubi
Copy link
Contributor Author

laeubi commented Mar 29, 2023

Could you add a correspondingly set up project as test case?

I used a bundle of equinox for testing this feature org.eclipse.equinox.event.zip

Will the bnd-lib only be used to generate the Manifest of the project or will it also build the jar and consequently handle the inclusion of resources, which would conflict with the build.properties?

It will generate the manifest and any DS-components or metatypes and a like similar to what bnd-process-goal does. PDE is actually never building a jar, so this then works as if one would maintain this files manually in the usual way, but build.properties is not needed here (or at least it is not planed to require one), but that's something I'm currently not have investigated in depth.

What I personally like about PDE's Manifest first approach is that it gives us the what-you-see-is-what-you-get principle. The drawback is that good metadata are hard to maintain manually and as you said often the metadata can be derived from the code, which actually means that they have to be synchronized manually.

Actually I think we need to improve PDE to make things more "visible", e.g. BndTools shows exported packages by having a small symbol in the content tree and I think PDE should support some kind of "show the manifest" menu entry when you select the project (currently one needs to go to the target state), or show a virtual item in the navigator (not yet checked how hard this would be).

So what I personally would like to have in a bright future is a mixture of both, that the Manifest is in principle maintained manually and one can for example specify the BSN there or which packages are exported etc.

You can already do this in the bnd.bnd file, just that it is not named "manifest", so a different apraoch would be to open the "Manifest editor" also if you choose the bnd.bnd, and have a new tab there as it is already done for build.properties and plugin.xml files, I just have not looked into how this works in detail.

So as mentioned above (I have extended the list now) I hope this being the trigger of many more changes / enhancements and any help is appreciated :-)

@laeubi laeubi force-pushed the support_building_of_bnd branch 2 times, most recently from aa694e1 to e23846f Compare March 29, 2023 07:12
Currently PDE only supports a "Manifest-First" approach whne building
plugins, but actually most of the data in a manifest can be derived from
the compiled class files and sources.

This adds a very basic aproach to support "Code-First" but still retain
the basic concepts of PDE like a target platform and the integration
with the existing PDE ecosystem.
@laeubi
Copy link
Contributor Author

laeubi commented Mar 29, 2023

@HannesWell I now have adjusted the code so it also do resource processing, so the equivalent of "bin.includes" in a bnd.bnd file would be:

-includeresource: plugin.properties

but with the advanced functionality as described here:
https://bnd.bndtools.org/instructions/includeresource.html

all content is then copied into the output folder so JDT/PDE should find it without special treatment.

@laeubi laeubi merged commit e1b12df into eclipse-pde:master Mar 29, 2023
@HannesWell
Copy link
Member

@HannesWell I now have adjusted the code so it also do resource processing, so the equivalent of "bin.includes" in a bnd.bnd file would be:

-includeresource: plugin.properties

but with the advanced functionality as described here: https://bnd.bndtools.org/instructions/includeresource.html

all content is then copied into the output folder so JDT/PDE should find it without special treatment.

Great. 👍🏽 Maybe this was ambiguous in my previous comment, but I think it is better to not build the jar for performance reasons.
I think we discussed that already in another context, but instead of actually coping resources wouldn't it be faster to just hard-link to them from the bin-folder? I'm not sure if all OS support such hard-links but that should be much faster and accurate at the same time.
The same could be also done for the bin.includes in the build.properties and would avoid discrepancies between the Plugin in the workspace and a Plug-in built to a jar.

What I personally like about PDE's Manifest first approach is that it gives us the what-you-see-is-what-you-get principle. The drawback is that good metadata are hard to maintain manually and as you said often the metadata can be derived from the code, which actually means that they have to be synchronized manually.

Actually I think we need to improve PDE to make things more "visible", e.g. BndTools shows exported packages by having a small symbol in the content tree and I think PDE should support some kind of "show the manifest" menu entry when you select the project (currently one needs to go to the target state), or show a virtual item in the navigator (not yet checked how hard this would be).

Fully agree, this also bothers me often that it is cumbersome to get the nicer manifest-editor opened for a bundle at many places.

So what I personally would like to have in a bright future is a mixture of both, that the Manifest is in principle maintained manually and one can for example specify the BSN there or which packages are exported etc.

You can already do this in the bnd.bnd file, just that it is not named "manifest", so a different apraoch would be to open the "Manifest editor" also if you choose the bnd.bnd, and have a new tab there as it is already done for build.properties and plugin.xml files, I just have not looked into how this works in detail.

Actually you are right. One improvement would IMHO be to format the generated manifest in a nicer way. I know some in the BND area argue that the MANIFEST.MF is a build output artifact and therefore should not be nice, but I believe it is important to be able to understand the manifest easily.
The drawback of just using the plain bnd-syntax is that users have to learn it. It is definitively very powerful, but the entry barrier is higher.
We probably have to think about that a bit and also how to handle PDE's build.properties bin.includes and BND's includeresource instruction together. My guess at the moment is that they are additive.
Btw. I think it should be stated that the generated Manifest has to be committed into version, because the Manifest-first approach for Tycho does not work anymore. Or do you have any magic plan for that (which I can hardly believe because on at least needs either a pom.xml or a MANIFEST.MF, other both)

@laeubi
Copy link
Contributor Author

laeubi commented Mar 30, 2023

Maybe this was ambiguous in my previous comment, but I think it is better to not build the jar for performance reasons.

The "Jar" is actually a memory object and "building" it is required to let BND perform certain actions, but there is never really a real jar file created, it is all transparently translated to the resource API of eclipse so you always gets an exploded folder as a result.

I think we discussed that already in another context, but instead of actually coping resources wouldn't it be faster to just hard-link to them from the bin-folder? I'm not sure if all OS support such hard-links but that should be much faster and accurate at the same time.

This would complicate things a lot for very little gain as the usual case is that resources included in a jar are quite small (usually a few kilobytes maybe megabytes) and we need to rely on native support for hardlinks (eclipse itself only supports softlinks right now), also it might be that people modify the file in the bin folder and getting confused that the source is edited as well and so on.

One improvement would IMHO be to format the generated manifest in a nicer way. I know some in the BND area argue that the MANIFEST.MF is a build output artifact and therefore should not be nice, but I believe it is important to be able to understand the manifest easily.

This should then be done at BND itself, currently there is no "format the manifest" (afaik) but PDE does format some headers as it is written but thats all done as specialized code path.

The drawback of just using the plain bnd-syntax is that users have to learn it. It is definitively very powerful, but the entry barrier is higher.

Every Manifest header is a valid syntax for bnd, so one do not really need to learn anything new, but you can use additional things, e.g instead of listing all exported packages you can use:

Export-Package: my.company.*

We probably have to think about that a bit and also how to handle PDE's build.properties bin.includes and BND's includeresource instruction together. My guess at the moment is that they are additive.

BND by default includes a file named build.properties, so if you like the build.properties you can write:

-includeresource: ${bin.includes}

and you are done, but usually bnd requires less things mentioned (e.g. no ., not META-INF, ...) so I think its better to not mix things to much here.

Btw. I think it should be stated that the generated Manifest has to be committed into version, because the Manifest-first approach for Tycho does not work anymore. Or do you have any magic plan for that (which I can hardly believe because on at least needs either a pom.xml or a MANIFEST.MF, other both)

Yes Tycho contains already some magic, and you neither need a MANIFEST.MF nor a pom.xml the bnd.bnd is enough as it contains everything required in this context:

it currently uses a bnd workspace to detect what has to be done, but with the new nature this can also be done based on the nature.

@iloveeclipse
Copy link
Member

Please note, this PR most likely caused a whole bunch of test fails and crashes in the last IBuild, all seem to be related to bnd dependencies missing at test runtime.

@laeubi
Copy link
Contributor Author

laeubi commented Mar 30, 2023

Please note, this PR most likely caused a whole bunch of test fails and crashes in the last IBuild, all seem to be related to bnd dependencies missing at test runtime.

I have checked here:
https://download.eclipse.org/eclipse/downloads/drops4/I20230329-1800/testresults/html/org.eclipse.pde.ui.tests_ep428I-unit-win32-java17_win32.win32.x86_64_17.html

and only see a few failures related to the URI problem that @HannesWell is currently working on, also the overview page only shows a few failures I can't directly relate to this change:
https://download.eclipse.org/eclipse/downloads/drops4/I20230329-1800/testResults.php

@iloveeclipse
Copy link
Member

@laeubi
Copy link
Contributor Author

laeubi commented Mar 30, 2023

@iloveeclipse please be more specific then... You claimed this change "caused a whole bunch of test fails and crashes" if I look at the page I see we have success rates > 99 %

If I look at PDE it reports the "URI is not hierarchically" here for two failing test:

https://download.eclipse.org/eclipse/downloads/drops4/I20230329-1800/testresults/html/org.eclipse.pde.ui.tests_ep428I-unit-win32-java17_win32.win32.x86_64_17.html

while for example I see three other platform succeed, so it can't be a general dependency problem.

The I see a lot of (for example here) " Could not initialize class org.eclipse.jdt.internal.core.JavaModelManager" but not a single pde in the stacktracke nor any mentioning of bnd classes not found (that where used already before in this commit here three weeks ago:

7ec2f75#diff-0bc962190317f532adb62e552a649bf36dc62d3b449e22555a7bc16439efb189

So if there is some evidence that this is caused by missing dependencies I can't see this ...

@iloveeclipse
Copy link
Member

don't you see BndResourceChangeListener in all logs?

@laeubi
Copy link
Contributor Author

laeubi commented Mar 30, 2023

don't you see BndResourceChangeListener in all logs?

I see this, but I don't see any "bnd dependencies missing at test runtime", this is a PDE class, but what I see is that we have a start-order dependency that requires JDT to always start before PDE and the listener now trigger this the other way round.

What happens is that PDE is activated and try to call JDT:

Caused by: java.lang.ExceptionInInitializerError
	at org.eclipse.jdt.core.JavaCore.addElementChangedListener(JavaCore.java:3460)
	at org.eclipse.pde.internal.core.JavaElementChangeListener.start(JavaElementChangeListener.java:46)
	at org.eclipse.pde.internal.core.PDECore.start(PDECore.java:305)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:818)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:1)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:569)
	at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:810)
	... 139 more
Caused by: java.lang.NullPointerException: Cannot invoke "org.eclipse.core.runtime.Plugin.getStateLocation()" because the return value of "org.eclipse.jdt.core.JavaCore.getPlugin()" is null
	at org.eclipse.jdt.internal.core.search.indexing.IndexManager.getJavaPluginWorkingLocation(IndexManager.java:624)
	at org.eclipse.jdt.internal.core.search.indexing.IndexManager.getSavedIndexesDirectory(IndexManager.java:628)
	at org.eclipse.jdt.internal.core.search.indexing.IndexManager.<init>(IndexManager.java:125)
	at org.eclipse.jdt.internal.core.JavaModelManager.<init>(JavaModelManager.java:1777)
	at org.eclipse.jdt.internal.core.JavaModelManager.<clinit>(JavaModelManager.java:1207)
	... 146 more

but as JavaModelManager has a static initialize accessing JavaCore.getPlugin() is null at this moment (because another activator is currently running -> PDE) everything fails.

So I'll try to make the listener more lazy here, but the JDT problem will happen for any plugin that unluckily starts before JDT is started (what might be unusual in a normal IDE setup) and access the JavaModelManager (either direct or indirectly) in its activation process.

@iloveeclipse
Copy link
Member

I see this, but I don't see any "bnd dependencies missing at test runtime",

Please read carefully my first comment.

Please note, this PR most likely caused a whole bunch of test fails and crashes in the last IBuild, all seem to be related to bnd dependencies missing at test runtime.

My assumption regarding missing dependencies was wrong. The rest not.

I glad you finally see:

  • we have lot of fails
  • it is pde problem
  • it is caused by this PR

So please provide a patch.

@laeubi
Copy link
Contributor Author

laeubi commented Mar 30, 2023

My assumption regarding missing dependencies was wrong.

Yes but based on these assumptions I was not able to relate this to any dependency problem and therefore it is quite confusing if not given a reference why it is assumed to be a dependency problem.

it is pde problem

it is not see above, JDT accesses a possibly null value in a static constructor that is not guaranteed to be initialized always and therefore has a start-order-dependency problem.

it is caused by this PR

This PR reveals the problem previously hidden.

So please provide a patch

I already added a workaround here that should hides the problem again:

@laeubi
Copy link
Contributor Author

laeubi commented Apr 4, 2023

Support for creating new project has now been added with:

@mnlipp
Copy link

mnlipp commented Oct 13, 2023

Is this the reason why eclipse has started to open .bnd files with PDE (which then says it has no editor) instead of the editor for bnd-files that comes with the bnd-plugin?

I can do "open with" as a (tedious) workaround but it seems impossible to change the default association.

@laeubi
Copy link
Contributor Author

laeubi commented Oct 13, 2023

it seems impossible to change the default association

Then please open a bug at eclipse-platform, PDE can only provide editor associations but the rest is not provided by PDE.

If you still think there is a bug at PDE please open a dedicated issue with exact steps to reproduce the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants