Skip to content
New issue

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

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

Already on GitHub? # to your account

Import-Package and other requirements not translated as Maven dependencies when generating pom #67

Closed
mickaelistria opened this issue Jul 4, 2022 · 18 comments

Comments

@mickaelistria
Copy link
Contributor

Currently, the pom generator (CBI p2Repo aggregator) only process Require-Bundle to derive Maven dependencies. Many bundles, more and more, do use Import-Package or even Require-Capability to define runtime requirements. Those should also be resolved and translated to Maven dependencies when generating the pom file to be deployed.

@laeubi
Copy link

laeubi commented Jul 4, 2022

I don't think this is generally valid. An import package is for specifically allowing different providers and even multiple providers can match a given import requirement and there is no real maven equivalent for this.

@mickaelistria
Copy link
Contributor Author

Yes but one would expect Maven artifacts to behave properly without specifying provider for this or that capability. I think we should try to add default providers for packages/capabilities as dependency, and people who want to deal with another provider would have to exclude the default one and add the replacement.
At least, this is how some libraries already believe in Maven, eg to replace logger.

@laeubi
Copy link

laeubi commented Jul 4, 2022

Yes but one would expect Maven artifacts to behave properly without specifying provider for this or that capability.

That's why I always said that "reverse resolve" maven artifacts after the fact is simply wrong.

I think we should try to add default providers for packages/capabilities as dependency

There is no such thing as "default provider", just keep DS as an example, why should Equinox be more "default" than Felix or any other? Beside that the DS provider is completely irrelevant for compile time and maven in particular as it is a runtime contain.

this is how some libraries already believe in Maven

Do we need to believe in Maven?

eg to replace logger

A maven build would depend on the Logger API but not on the Logger Implementation ...

@mickaelistria
Copy link
Contributor Author

That's why I always said that "reverse resolve" maven artifacts after the fact is simply wrong.

Talking is cheap, show me the code ;)

There is no such thing as "default provider"

That's true in theory, but some projects do recommend 1 particular provider (eg logger implementation) and ship it as a dependency so everything works according to the project recommendation out of the box. Integrators can still deal with exclude/extra dependency to bind another one.
Yes, it may not be something that is perfect, yet it's a common and efficient pattern.

Do we need to believe in Maven?

Sorry, I wanted to write "behave".

A maven build would depend on the Logger API but not on the Logger Implementation ...

I've seen projects that do not work that way and want a good logger by default, so they add it to dependencies. And it works.

@laeubi
Copy link

laeubi commented Jul 4, 2022

Talking is cheap, show me the code ;)

I have already described this in the previous discussion, nerveless it was decided to put more effort into keeping the old broken attempt ...

That's true in theory, but some projects do recommend 1 particular provider (eg logger implementation) and ship it as a dependency so everything works according to the project recommendation out of the box.

That's completely different, than binding a binary artifact to something it was never compiled with, this is plainly wrong and waiting for bad things to happen.

And it works.

Yeah "uber-jars" also work, so we probably just ship one big-fat jar that includes everything, so no more worries...

@merks
Copy link
Contributor

merks commented Jul 6, 2022

@mickaelistria

I am making good progress on improving and testing the SDK4Mvn.aggr. I've added enough mappings such that all IUs are mapped to existing group IDs:

image

The ** indicates something is not a valid/existing Maven group so that's only the fake JRE unit.

Many of the mappings are computed from the Maven metadata in p2.

I'm now resolving all requirements, including package requirements. I'm not sure how best to handle requirements that resolve to more than on IU, e.g., here are just a few:

org.eclipse.jdt.apt.pluggable.core -> package org.eclipse.jdt.internal.compiler.tool 0.0.0
  org.eclipse.jdt:ecj
  org.eclipse.jdt:org.eclipse.jdt.compiler.tool

org.eclipse.core.contenttype -> package org.eclipse.osgi.service.debug 0.0.0
  org.eclipse.platform:org.eclipse.osgi
  org.eclipse.platform:org.eclipse.equinox.supplement

org.eclipse.ui.workbench -> package org.osgi.service.component.annotations 1.2.0
  org.eclipse.platform:org.eclipse.osgi.services
  org.eclipse.pde:org.eclipse.pde.ds1_2.lib
  org.eclipse.pde:org.eclipse.pde.ds.lib

org.osgi.service.log.stream -> package org.osgi.service.log [1.4.0,2.0.0)
  org.eclipse.platform:org.eclipse.osgi
  org.eclipse.platform:org.eclipse.osgi.service

Note too that even if the package requirement has a version range, we can't map that onto a version range on the IU (group/artifact)...

Any thoughts on how we might deal with duplicate requirement resolutions? My sense is that we need some type of rule to to narrow the choices down to a single one or even to be able to filter them out...

Also, I think some of the handling of os/arch/ws specific requirement is bogus, but I need to investigate further...

@laeubi
Copy link

laeubi commented Jul 6, 2022

I'm now resolving all requirements, including package requirements. I'm not sure how best to handle requirements that resolve to more than on IU, e.g., here are just a few:

As mentioned above this is not resolvable, as every one of those is a valid provider but serving completely different purpose. Beside that P2 is currently not capable of processing split packages correctly as far as I know...

(reverse) mapping package imports is simply wrong as the compile time maven dependencies serve completely different purpose than selecting a suitable provider for a package import at runtime, e.g. it is not enough to just select one that provides the package, you also need to take into account use-clauses to not create an invalid class space...

@merks
Copy link
Contributor

merks commented Jul 6, 2022

I'm now resolving all requirements, including package requirements. I'm not sure how best to handle requirements that resolve to more than on IU, e.g., here are just a few:

As mentioned above this is not resolvable, as every one of those is a valid provider but serving completely different purpose. Beside that P2 is currently not capable of processing split packages correctly as far as I know...

The resolution I'm doing here is just a query so all bundles exporting the package will be in the list. And while the problem may not be solvable in general, most uses of package imports are just saying I don't care about the name of the bundle that provides this package.

(reverse) mapping package imports is simply wrong as the compile time maven dependencies serve completely different purpose than selecting a suitable provider for a package import at runtime, e.g. it is not enough to just select one that provides the package, you also need to take into account use-clauses to not create an invalid class space...

The package imports declared in the manifest and recorded in the p2 metadata are not compile time dependencies but runtime dependencies.

@laeubi
Copy link

laeubi commented Jul 6, 2022

most uses of package imports are just saying I don't care about the name of the bundle that provides this package

Use clauses are not about bundle-names but about related packages, and P2 simply ignores this what could lead to wrong package-provider-bundles to be choosen, see for example: https://spring.io/blog/2008/10/20/understanding-the-osgi-uses-directive/, org.eclipse.core.contenttype is probably such a split package here.

The package imports declared in the manifest and recorded in the p2 metadata are not compile time dependencies but runtime dependencies.

Correct, but the dependencies in a pom are compile time dependencies... so mapping them just because the match something in a P2 repo is not correct. Best example is from your list:

org.eclipse.ui.workbench -> package org.osgi.service.component.annotations 1.2.0

org.osgi.service.component.annotations has class-retention policy so:

  1. It should not be imported, as it is not required at runtime, and anyone doing it correctly will not record that in the P2 metadata
  2. On compile time (e.g. using the pom for your project) you will then have an error because you cannot recover that from the P2 metadtata and the compile time dependency is missing here...

@merks
Copy link
Contributor

merks commented Jul 6, 2022

And all this is not a problem when using plugin dependencies which are translated to maven dependencies already...

@laeubi
Copy link

laeubi commented Jul 6, 2022

And all this is not a problem when using plugin dependencies which are translated to maven dependencies already...

But this is not about plugin dependencies...

@merks
Copy link
Contributor

merks commented Jul 6, 2022

It's a question nevertheless because often the platform is changing bundle requirements to package imports purely to avoid depending on the name of the bundle, which changes when it comes directly from Maven... As with the recent junit changes. So I assume the platform is hoping the dependencies they had before don't disappear, so I need to understand what @mickaelistria thinks about this.

@laeubi
Copy link

laeubi commented Jul 6, 2022

Yes I think what you did here is all correct, I just wanted to explain why the problem is not solvable (with the given data in P2) as you can't know if the bundle has used this JUnit bundle/artifact when it was compiled, you can only find that it has something had on the compile classpath that provided the JUnit classes and at runtime wants a bundle exporting the package, but these two must not match so you will always get ambiguities!

@mickaelistria
Copy link
Contributor Author

I think we need something that works good enough even if it's not always perfect. As @merks mentions, platform uses more and more Import-Package and we still want Maven artifacts to come with a decent dependency tree for the artifacts to be usable in Maven.
So I think we indeed need to figure out some rule that will be able to select 1 provider we want to make default for a given requirement, even in case multiple providers exist. Indeed, usually split package and uses clauses (which are not supported well by p2) should be able to disambiguate many of such cases. For the case listed above, we need to check whether support for split/uses should be enough to decide of 1 provider. If all MANIFEST.MF are sufficient to disambiguate it, then it means solving this problem is a matter of improving p2 (which may not be trivial either) or of implementing a different approach to resolve dependencies. One possibility for bundles would be eg to load them all in an Equinox instance and query the OSGi resolver (not trivial either, but maybe more accessible than improving p2).

@laeubi in another issue, I mentioned my attempt with dependency:tree which should be a good source of information for those cases, or even to replace the p2->mvn strategy implemented in the resolver; but also shared that currently Tycho is not very capable of building a dependency:tree so we cannot rely on it for the moment; but still need something that works fine soon-ish.

@mickaelistria
Copy link
Contributor Author

About the replacement of Require-Bundle by Import-Package, it's not really a requirement but more an intermediary step to simplify migration to other providers (Bundle-SymbolicName org.junit.xxx from Orbit becomes junit-xxx from Central). But if it becomes an issue downstream for eg Maven artifacts, it would be totally acceptable to revert to Require-Bundle after migration to Central is complete).

@laeubi
Copy link

laeubi commented Jul 6, 2022

@mickaelistria I'm not sure whats wrong with dependency:tree (beside that its not a nice tree yet) and the info will be present in the (generated) pom, but this is disabled for most (if not all) eclipse projects, so the data is lost afterwards. I'm currently improving the P2 capabilities so we might produce a "nicer" tree, still I wonder why a tree is better than a flat list for this purpose? So maybe if there is an issue with the dependency:tree best would be an issue in Tycho+Example to work on it.

it would be totally acceptable to revert to Require-Bundle after migration to Central is complete).

Require-Bundle is bad, we should really not step-back here just to try guessing things we throw away at step zero...

@mickaelistria
Copy link
Contributor Author

still I wonder why a tree is better than a flat list for this purpose?

We need to capture only root dependencies be add them as dependencies in pom. Transitive dependencies are better being ignored.

merks added a commit to merks/p2repo-aggregator that referenced this issue Jul 11, 2022
dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregators features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

eclipse-platform/eclipse.platform.releng#67
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jul 11, 2022
dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregators features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi/p2repo-aggregator#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

eclipse-platform#67
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jul 15, 2022
dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregators features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi/p2repo-aggregator#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

eclipse-platform#67
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jul 17, 2022
dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregator's features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi/p2repo-aggregator#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

eclipse-platform#67
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jul 29, 2022
dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregator's features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi/p2repo-aggregator#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

eclipse-platform#67
@merks
Copy link
Contributor

merks commented Jul 29, 2022

I had to reintroduce a mapping for bouncycastle.

@merks merks closed this as completed Sep 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants