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 actions that publish IUs used in the build #354

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Oct 19, 2023

This adds actions for code that want to transform an PDE project into a set of requirements/capabilities for example to compute the required units of a target platform for such project.

@merks as discussed at EclipseCon 2023 this adds a new PdeAction that allows to determine the Requirements/Capabilites of a source project to be used to collect items for a Target Platform. This code is adapted from Tycho and slightly adjusted an do the following:

  1. it checks if there is a build.properties files that contains additional.bundle, if yes these are transformed into an Iu requiring this as optional items (as PDE handles them in an optional fashing as well)
  2. it checks for pde.bnd file and looks there for required buildpath entries
  3. Then it looks for the manifest at the (probabbly diferent configured) location, and falls back to the standard location if not found.

I was not able to really test this in a context of P2/Ooompf but if you want to give it a try let me know if anything needs adjustments.

@laeubi laeubi requested a review from merks October 19, 2023 12:42
@github-actions
Copy link

github-actions bot commented Oct 19, 2023

Test Results

       9 files  ±0         9 suites  ±0   33m 3s ⏱️ + 3m 21s
2 181 tests ±0  2 177 ✔️ ±0    4 💤 ±0  0 ±0 
6 633 runs  ±0  6 622 ✔️ ±0  11 💤 ±0  0 ±0 

Results for commit 6abe0c6. ± Comparison against base commit f108a32.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member

Since this is specific to and tightly coupled to PDE, wouldn't it then make sense to add it to PDE?

@laeubi
Copy link
Member Author

laeubi commented Oct 20, 2023

Since this is specific to and tightly coupled to PDE, wouldn't it then make sense to add it to PDE?

I don't think that's suitable here with the current state of PDE as it will bind to major parts of PDE in the worst case and is hard to discover/use for P2 users.

Maybe one time we will have an org.eclipse.pde.lib that contains all API and generic PDE functions that can easily be shared without pulling a lot of stuff ...

@HannesWell
Copy link
Member

HannesWell commented Oct 20, 2023

Since this is specific to and tightly coupled to PDE, wouldn't it then make sense to add it to PDE?

I don't think that's suitable here with the current state of PDE as it will bind to major parts of PDE in the worst case and is hard to discover/use for P2 users.

That's right, but I think it depends on who the target audience is.
Until know I only know of @merks that could use it in Oomph for Targelets, that assumable already depend on pde.core any way.
If the goal is to serve a broader audience that does not depend on pde.core any ways, then pde.core as a whole could indeed be an issue.

Can anybody assess if there is realistically a user besides Oomph?

@merks
Copy link
Contributor

merks commented Oct 21, 2023

I would have expected something like this to simply be integrated as part of the existing BundlesAction such that the existing APIs "just work" with either "format". That would seem better. After all, these APIs are there to be able to map a folder representing the source of an OSGi bundle into the corresponding p2 installable unit so ideally would not need to know directly ahead of time if the folder is using a new improved format.

@laeubi
Copy link
Member Author

laeubi commented Oct 21, 2023

would have expected something like this to simply be integrated as part of the existing BundlesAction such that the existing APIs "just work" with either "format".

BundlesAction actually was never meant to "analyze source projects" it is actually used to transform binary bundles into P2 units and as such it does not make sense to enhance it as it even would be harmful, so lets keep things to their concerns.

So if someone really likes to analyze the source for compilation and get all dependencies on of the source bundle PDEAction can be used (what includes BundlesAction).

If the goal is to serve a broader audience that does not depend on pde.core any ways, then pde.core as a whole could indeed be an issue.

The goal is to provide something for P2 so everyone writing P2 code can use it easily, if it would have been only for Ooomph it would better be placed there in a private package as PDE itself has not any use for that.

@HannesWell
Copy link
Member

would have expected something like this to simply be integrated as part of the existing BundlesAction such that the existing APIs "just work" with either "format".

BundlesAction actually was never meant to "analyze source projects" it is actually used to transform binary bundles into P2 units and as such it does not make sense to enhance it as it even would be harmful, so lets keep things to their concerns.

So if someone really likes to analyze the source for compilation and get all dependencies on of the source bundle PDEAction can be used (what includes BundlesAction).

That makes sense, I also assume that the BundlesAction (more or less) only operates on the specified OSGi metadata.
But to make that more clear, maybe it would be good to name it PDEProjectAction instead of PdeAction?

I didn't went trough this change in detail yet, but can the new action also handle classical PDE projects with a hand-written MANIFEST.MF?

If the goal is to serve a broader audience that does not depend on pde.core any ways, then pde.core as a whole could indeed be an issue.

The goal is to provide something for P2 so everyone writing P2 code can use it easily, if it would have been only for Ooomph it would better be placed there in a private package as PDE itself has not any use for that.

I have to admit I'm torn in this case.

I understand your point that its good to provide the action with as few dependencies as possible in a relatively prominent place where it can be found by others. But at the same time I cannot tell, if there are other users than Oomph but that can also be due to a lack of imagination and who knows what the future brings.
The good thing is the containing package is not 'public' and thus this is not 'public API'.

On the other hand having this action, that is tightly coupled to how PDE internally handles the project and the features it provides, located in P2 will probably make it harder to maintain.

@laeubi
Copy link
Member Author

laeubi commented Oct 21, 2023

I didn't went trough this change in detail yet, but can the new action also handle classical PDE projects with a hand-written MANIFEST.MF?

It handles all variants of "classic" or "new" ways of specify the manifest

On the other hand having this action, that is tightly coupled to how PDE internally handles the project and the features it provides, located in P2 will probably make it harder to maintain.

These things change in years (if not decades) and for example P2 contain lots of more complex stuff from PDE (features, products, ...) without any code-share so I don't think there is any issue here with these trivial things.

@laeubi
Copy link
Member Author

laeubi commented Dec 13, 2023

@merks do we want to continue on this? If not suitable for P2 directly this might instead be added to Oompf directly as I understand its mostly needed there.

@merks
Copy link
Contributor

merks commented Dec 13, 2023

I don't know how to test this. 😢 A zip file a sample would be helpful...

What's the actual plan for the Platform in terms of keeping the generated MANIFEST.MF checked in? Of course my primary concern is the extent to which a source project's full requirements and full capabilities are known purely from the source as is currently the case with a MANIFEST.MF...

@laeubi
Copy link
Member Author

laeubi commented Dec 13, 2023

This is my platform demonstrator / incubation project for automatic manifest generation:

eclipse-equinox/equinox#246

for testing build.properties, you can use org.eclipse.jface that has a (soon obsolete) additional.bundles entry

What's the actual plan for the Platform in terms of keeping the generated MANIFEST.MF checked in?

I think there is not any concrete type of plan.

Of course my primary concern is the extent to which a source project's full requirements and full capabilities are known purely from the source as is currently the case with a MANIFEST.MF...

This is independent from that, please note that MANIFEST.MFonly describes runtime requirements / capabilities, while this is to discover compile time requirements that you will probably never discover from the manifest, for example I can have an class/source annotation requirement in additional.bundles (build properties) / -buildpath (pde.bnd) that will never show up in the MANIFEST.MF but still will give you a broken workspace as the project can't compile. Same for having a specific bundle in -buildpath (pde.bnd), that resuts in an import-package, from only looking at the manifest you probably will choose another provider that is needed for compilation!

So this is purely to transform such build requirements into something P2 / Ooompf can understand if it want to provision a workspace where the project should be compile (provided capabilities might be added later as well).

@merks
Copy link
Contributor

merks commented Dec 13, 2023

Very good. I will try to find time in the next day or two...

This adds actions for code that want to transform an PDE project into a
set of requirements/capabilities for example to compute the required
units of a target platform for such project.
@merks
Copy link
Contributor

merks commented Dec 14, 2023

I'm doing some testing, but I don't understand the design intent. A "normal" bundle publisher takes the MANIFEST.MF into account and also things like the p2.inf.

But here I get this as input, note there is no MANIFEST.MF:

image

The only thing that's produced is some information about the build path which is associated with some "synthetic IU"

bnd-bundle-requirements-413729c5-8d44-45d0-a946-4927c65f5404
[osgi.bundle; org.eclipse.osgi 0.0.0, osgi.bundle; org.osgi.service.event 0.0.0]

The rest of the information is the pde.bnd file is ignored completely.

Is there something special to be done for a bnd.bnd?

@laeubi
Copy link
Member Author

laeubi commented Dec 14, 2023

The rest of the information is the pde.bnd file is ignored completely.

Correct, because the build path (and the test build path) describes everything the project needs to compile and producing a manifest, so in this case it requires that the bundles org.eclipse.osgi and org.osgi.service.event are present in the target platform.

@merks
Copy link
Contributor

merks commented Dec 14, 2023

I think perhaps we've overlooked a fundamental point here. These things are used (by Oomph/targlets) before there exists a target platform. In particular, this would be used when there exists only a clone, freshly cloned from git, and only on disk, not in a workspace, no builders, no generators, no workspace, no nothing. With the existing infrastructure, the MANIFEST.MF exists and all works extremely well. If this is to be a replacement for that, and the MANIFEST.MF does not even exist (yet), then whatever information is used to generate the MANIFEST.MF (later) needs to be used to synthesize the InstallableUnit (in the action) that will eventually be synthesized from the generated MANIFEST.MF. Is that feasible?

@laeubi
Copy link
Member Author

laeubi commented Dec 14, 2023

I think perhaps we've overlooked a fundamental point here. These things are used (by Oomph/targlets) before there exists a target platform. In particular, this would be used when there exists only a clone, freshly cloned from git, and only on disk, not in a workspace, no builders, no generators, no workspace, no nothing.

I don't think we have overlooked something but you misintepreted the intend, it tells you it want bundle X in a (potential) target platform.

I never written this replace MANIFEST.MF, it gives you additional information you never will find in any MANIFEST.MF ... and actually it even includes MANIFEST.MF if there where some (what its not in this example).

@merks
Copy link
Contributor

merks commented Dec 14, 2023

I don't need to know just build requirements, I need the full IU itself, e.g., the BSN, version, provided capabilities and the required capabilities. The MANIFEST.MF is absent, but was like this before and from that I know everything that needs to be known:

Bundle-ManifestVersion: 2
Bundle-Name: %bundleName
Bundle-Version: 1.7.0.qualifier
Bundle-SymbolicName: org.eclipse.equinox.event
Import-Package: org.eclipse.osgi.framework.eventmgr;version="1.1.0",
 org.eclipse.osgi.util;version="1.1.0",
 org.osgi.framework;version="1.6.0",
 org.osgi.service.event;version="[1.3,1.5)",
 org.osgi.service.log;version="1.3.0",
 org.osgi.util.tracker;version="1.5.0"
Export-Package: org.eclipse.equinox.internal.event;x-internal:=true,
 org.eclipse.equinox.internal.event.mapper;x-internal:=true
Bundle-Vendor: %bundleVendor
Bundle-Localization: plugin
Bundle-RequiredExecutionEnvironment: JavaSE-17
Service-Component: OSGI-INF/component.xml
Bundle-ActivationPolicy: lazy
Provide-Capability: 
 osgi.service;
  objectClass:List<String>="org.osgi.service.event.EventAdmin";
  uses:="org.osgi.service.event",
 osgi.implementation;
  osgi.implementation="osgi.event";
  uses:="org.osgi.service.event";
  version:Version="1.4"
Require-Capability: osgi.extender;
 filter:="(&(osgi.extender=osgi.component)(version>=1.0)(!(version>=2.0)))"
Automatic-Module-Name: org.eclipse.equinox.event

Some of that information is present in the pde.bnd:

Bundle-Name: %bundleName
Bundle-Version: 1.6.300.qualifier
Bundle-SymbolicName: org.eclipse.equinox.event
Bundle-Vendor: %bundleVendor
Bundle-Localization: plugin
Bundle-ActivationPolicy: lazy
Automatic-Module-Name: org.eclipse.equinox.event
Export-Package: org.eclipse.equinox.internal.event;x-internal:=true, \
				org.eclipse.equinox.internal.event.mapper;x-internal:=true

-dsannotations-options: version;maximum=1.3
-runee: JavaSE-1.8
-includeresource: plugin.properties,\
               about.html
               
-buildpath: org.eclipse.osgi, \
			org.osgi.service.event
		
# pomless configuration options
pom.model.property.tck.artifact = org.osgi.test.cases.event

But not all of it and I have no idea how some of that information is recovered. Certainly not from locally-available information purely in the file system.

So in the end, this does not appear useful for the intended purpose. Perhaps if the logic considered the additional headers in the pde.bnd it would be slightly more useful, but in the current form, it appears that the omission of the MANIFEST.MF from the git repository leads to a non-recoverable loss of information.

@laeubi
Copy link
Member Author

laeubi commented Dec 14, 2023

I don't need to know just build requirements,

As said this is in addition to anything else, so it is not "just" it is "also"

The MANIFEST.MF is absent

You can just put the manifest there if you like for your tests, but as said this is not to replace it (even though in my example its not there because I have not checked it in as its not relevant for the build test) but to enhance its information.

Perhaps if the logic considered the additional headers in the pde.bnd it would be slightly more useful

That's a completely different topic.

@laeubi
Copy link
Member Author

laeubi commented Jun 16, 2024

@merks there is now a more complete demo thanks to @fipro78 here:

https://github.com/eclipse-tycho/tycho/tree/main/demo/pde-automatic-manifest

this might help to better understand / test what this does.

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.

3 participants