Skip to content

feat(GH-188): Implement package type providers #223

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

dwalluck
Copy link
Contributor

This uses the Java Service Provider Interface via ServiceLoader to register new types, where each type gets its own class.

This uses the Java Service Provider Interface via `ServiceLoader` to
register new types, where each type gets its own class.
@dwalluck dwalluck marked this pull request as draft March 23, 2025 13:59
@ppkarwasz
Copy link
Contributor

ppkarwasz commented Mar 23, 2025

My 2 cents about the API changes:

  • The only class that the user needs to see in order to implement a new type is PackageTypeProvider. Since users that implement this interface are service providers, I would put it in a spi package. The package must have an @Export annotation, so it is marked as exported by BND.
  • The remaining classes can be in an non-exported package. I would suggest calling it internal, so even non-JPMS users understand that the package is not part of the public API. The ServiceLoader JavaDoc even recommends this:

    It is strongly recommended that the module does not export the package containing the service provider.

  • Nit: I find putting each implementation of PackageTypeProvider a little bit verbose. These classes need to be public, but they can be nested classes of a single top level class.
  • JPMS and OSGi don't read the META-INF/services files. To declare a class as service, the @ServiceProvider class from biz.aQute.bnd.annotation can be used (see documentation):
    @ServiceProvider(value = PackageTypeProvider.class, resolution = Resolution.MANDATORY)
    This will generate all the necessary JPMS, OSGi and even the META-INF/services file. The current BND goal jar does this, when it creates the JAR, which is too late for unit tests, so we need either keep a manually generated META-INF/services file or switch to the bnd-process goal.
  • The class that calls ServiceLoader needs to be annotated with:
    @ServiceConsumer(value = PackageTypeProvider.class, resolution = Resolution.MANDATORY, cardinality = Cardinality.MULTIPLE)

@dwalluck dwalluck changed the title GH-188: Implement package type providers feat(GH-188): Implement package type providers Mar 27, 2025
@dwalluck
Copy link
Contributor Author

dwalluck commented Mar 27, 2025

@ppkarwasz I made most of the changes you suggested, but I need to know how to update the pom.xml and bnd.bnd files:

[WARNING] bnd.bnd [0:0]: jpms.jarname calculated module names from file name: [org.osgi.resource-1.0.0, org.osgi.service.serviceloader-1.0.0]
[ERROR] bnd.bnd [0:0]: Requirement osgi.extender resolution not correct
[ERROR] bnd.bnd [0:0]: Requirement osgi.serviceloader cardinality not correct
[ERROR] bnd.bnd [0:0]: Requirement osgi.serviceloader resolution not correct

@dwalluck
Copy link
Contributor Author

@ppkarwasz There seems to be duplicated classes between this and org.osgi.annotation.bundle. I am not sure which to prefer, since I think both dependencies end up being necessary.

@dwalluck
Copy link
Contributor Author

  • This will generate all the necessary JPMS, OSGi and even the META-INF/services file. The current BND goal jar does this, when it creates the JAR, which is too late for unit tests, so we need either keep a manually generated META-INF/services file or switch to the bnd-process goal.

@ppkarwasz I think it's better to auto-generate this file if we can.

value = PackageTypeProvider.class,
resolution = Requirement.Resolution.MANDATORY,
cardinality = Requirement.Cardinality.MULTIPLE)
public final class PackageTypeFactory implements PackageTypeProvider {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It bothers me to have the factory class also implementing some type of provider, since it's not actually added to the set of providers.

This is trying to provider the default normalization that needs to happen before all type-specific normalization.

But, maybe there is a better way to do this (add it to the list in the first position?).

Comment on lines +542 to +552
try {
PackageURL packageURL = normalize();
namespace = packageURL.getNamespace();
name = packageURL.getName();
version = packageURL.getVersion();
qualifiers = packageURL.getQualifiers();
subpath = packageURL.getSubpath();
} catch (MalformedPackageURLException e) {
throw new ValidationException("Normalization failed", e);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of a hack right now.

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 4, 2025

@ppkarwasz I am stuck on the OSGi errors. I don't have experience creating a bnd file manually as opposed to plugins that generate the MANIFEST.MF bundle headers.

@dwalluck dwalluck marked this pull request as ready for review April 4, 2025 16:05
@ppkarwasz
Copy link
Contributor

I am stuck on the OSGi errors. I don't have experience creating a bnd file manually as opposed to plugins that generate the MANIFEST.MF bundle headers.

I'll check it out.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

The BND errors are due to the wrong constants being used.

Comment on lines 40 to 41
import org.osgi.annotation.bundle.Requirement.Cardinality;
import org.osgi.annotation.bundle.Requirement.Resolution;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import org.osgi.annotation.bundle.Requirement.Cardinality;
import org.osgi.annotation.bundle.Requirement.Resolution;
import aQute.bnd.annotation.Cardinality;
import aQute.bnd.annotation.Resolution;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was using something from aQute, but it said deprecated use the OSGi ones, but now I can't find it.

import java.util.Map;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;
import org.osgi.annotation.bundle.Requirement.Resolution;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import org.osgi.annotation.bundle.Requirement.Resolution;
import aQute.bnd.annotation.Resolution;

@ppkarwasz
Copy link
Contributor

What bothers me a little with the current proposal is that type-specific normalization and validation only occurs when normalize and canonicalize is called. Looking at the "Tests" section of the spec and in particular:

  • parsing the test purl should return the components parsed from the test canonical purl

I deduce that the components should only and always be stored in normalized form (and be valid). So the PackageTypeProvider interface should probably a validate-and-normalize method for each component, so that it can be used in the constructors.

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.

2 participants