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.

Sign up for GitHub

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

Jump to bottom

Include required plugins automatically seem not consider already added capabilties #1604

Open
laeubi opened this issue Feb 9, 2025 · 7 comments · May be fixed by #1618
Open

Include required plugins automatically seem not consider already added capabilties #1604

laeubi opened this issue Feb 9, 2025 · 7 comments · May be fixed by #1618

Comments

@laeubi
Copy link
Contributor

laeubi commented Feb 9, 2025

If I use the "Include required plugins automatically" option I noticed that it does seem to add more than required in some situations.

For example assume a product hat has slf4j.api and two slf4j provider implementations are in the target then PDE adds another slf4j provider even if one was already chosen in the product, I noticed that when working on

with the product here:

<?xml version="1.0" encoding="UTF-8"?>
<?pde version="3.5"?>

<product uid="testslf4j" version="1.0.0.qualifier" type="bundles" includeLaunchers="true" autoIncludeRequirements="false">

   <configIni use="default">
   </configIni>

   <launcherArgs>
      <programArgs>-console
      </programArgs>
      <vmArgs>-Declipse.ignoreApp=true
-Dosgi.noShutdown=true
      </vmArgs>
      <vmArgsMac>-XstartOnFirstThread -Dorg.eclipse.swt.internal.carbon.smallFonts
      </vmArgsMac>
   </launcherArgs>

   <launcher>
      <win useIco="false">
         <bmp/>
      </win>
   </launcher>

   <vm>
   </vm>

   <plugins>
      <plugin id="my.logging.bundle"/>
      <plugin id="org.apache.aries.spifly.dynamic.bundle"/>
      <plugin id="org.apache.felix.gogo.command"/>
      <plugin id="org.apache.felix.gogo.runtime"/>
      <plugin id="org.apache.felix.gogo.shell"/>
      <plugin id="org.apache.felix.scr"/>
      <plugin id="org.eclipse.equinox.common"/>
      <plugin id="org.eclipse.equinox.console"/>
      <plugin id="org.eclipse.osgi"/>
      <plugin id="org.objectweb.asm"/>
      <plugin id="org.objectweb.asm.commons"/>
      <plugin id="org.objectweb.asm.tree"/>
      <plugin id="org.objectweb.asm.tree.analysis"/>
      <plugin id="org.objectweb.asm.util"/>
      <plugin id="org.osgi.service.component"/>
      <plugin id="org.osgi.util.function"/>
      <plugin id="org.osgi.util.promise"/>
      <plugin id="slf4j.api"/>
   </plugins>

   <configurations>
      <plugin id="org.apache.aries.spifly.dynamic.bundle" autoStart="true" startLevel="1" />
      <plugin id="org.apache.felix.scr" autoStart="true" startLevel="2" />
      <plugin id="org.eclipse.core.runtime" autoStart="true" startLevel="0" />
      <plugin id="org.eclipse.equinox.common" autoStart="true" startLevel="2" />
      <plugin id="org.eclipse.equinox.event" autoStart="true" startLevel="2" />
      <plugin id="org.eclipse.equinox.simpleconfigurator" autoStart="true" startLevel="1" />
   </configurations>

</product>

The product itself is "complete" so works with autoIncludeRequirements="false" already, no missing requirements are reported when validated. If one starts the product it is getting the expected message:

SLF4J(W): No SLF4J providers were found.
SLF4J(W): Defaulting to no-operation (NOP) logger implementation
SLF4J(W): See https://www.slf4j.org/codes.html#noProviders for further details.

Now I add <plugin id="org.eclipse.equinox.slf4j"/> and logging works and I get:

id	State       Bundle
7	ACTIVE      org.eclipse.equinox.slf4j_1.0.0.qualifier
17	RESOLVED    slf4j.api_2.0.16

if now one activates autoIncludeRequirements="true" one suddenly gets

id	State       Bundle
6	RESOLVED    slf4j.simple_2.0.16
8	ACTIVE      org.eclipse.equinox.slf4j_1.0.0.qualifier
18	RESOLVED    slf4j.api_2.0.16

@HannesWell can you take a look as this feature is extremely useful but might confusing in some situations like this.
@HannesWell
Copy link
Member

This is probably because DependencyManager considers all kind of requirements:

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

Line 199 in 975f72b

List<BundleWire> requiredWires = wiring.getRequiredWires(null);

And IIRC the slf4j.api bundle has a (OSGi) requirement to all it's providers through the Service-Loader mediator configuration.

In this specific case the life-line that prevents a real problem is that the Aries SPI-fly service-loader mediator only considers bundles in state active, which isn't usually the case (like in your example) as long as one has not configured it as auto-started in the product configuration (at least in my use-cases this was the case).

But of course in general it would be cleaner to not have the other providers in the product at all. I wanted to add meant to filter the considered required-wires so that the DependencyManager can be used in more places in PDE.
Then the service-loader requirements could be filtered too.

@laeubi
Copy link
Contributor Author

laeubi commented Feb 12, 2025

And IIRC the slf4j.api bundle has a (OSGi) requirement to all it's providers through the Service-Loader mediator configuration.

Should it not only require one?

@laeubi
Copy link
Contributor Author

laeubi commented Feb 15, 2025

@HannesWell I have now further checked this and the spec mentions

cardinality
String ['multiple' | 'single']
Allow the requirement to be satisfied by just one capability or provide wires to any capability that satisfies the requirement.

where multiple seems the default, I think slf4j should use single here to not pull in multiple providers (what is not supported). As you have made changes to slf4j in the past would you mind proposing such thing so that hopefully with a new release of slf4j the issue is resolved?

@laeubi
Copy link
Contributor Author

laeubi commented Feb 15, 2025

According to the javadoc single is the default...

The requirement directive used to specify the cardinality for a requirement. The default value is single

So something seems to be odd here...

@laeubi
Copy link
Contributor Author

laeubi commented Feb 15, 2025

I was now able to debug it and it is as suspected in the initial description, PDE does only consider requirements on the bundle level (based on the wiring in the target state), in this case the requirement is fulfilled by two bundles:

slf4j-simple (that is wired in the target state) and slf4j-equinox and thus it can't detect that as already being present.

@laeubi laeubi linked a pull request Feb 15, 2025 that will close this issue
Open
@HannesWell
Copy link
Member

I was now able to debug it and it is as suspected in the initial description, PDE does only consider requirements on the bundle level (based on the wiring in the target state), in this case the requirement is fulfilled by two bundles:

slf4j-simple (that is wired in the target state) and slf4j-equinox and thus it can't detect that as already being present.

Yes, after checking the required and provided capabilities in the MANIFEST.MF I also think that's what happens (sorry for not writing it earlier).
For clarification: slf4j.simple is wired to the slf4j.api bundle to satisfy it's service-loader requirement in the Target-Platform and therefore added to the launch. And the equinox.slf4j bundle is obviously added because it's listed in the product configuration.
So we end up with two providers in the runtime, while one would be sufficient.
But of course the TP doesn't know what the product needs.

@HannesWell I have now further checked this and the spec mentions

cardinality
String ['multiple' | 'single']
Allow the requirement to be satisfied by just one capability or provide wires to any capability that satisfies the requirement.

where multiple seems the default, I think slf4j should use single here to not pull in multiple providers (what is not supported). As you have made changes to slf4j in the past would you mind proposing such thing so that hopefully with a new release of slf4j the issue is resolved?

As you've already described later, that's what is already happening.
But since it seems to be common to specify a type attribute in the provider Manifests (e.g. type=simple), the other way round could be interesting in combination with a way to specify the provider to use explicitly.

For a flat class-path it's already possible to specify the factory explicitly.

https://github.com/qos-ch/slf4j/blob/4b4d533771e0501ffe1eb5e248e456f285357737/slf4j-api/src/main/java/org/slf4j/LoggerFactory.java#L233-L244

For OSGi runtimes the existing property could actually be re-used and the selection could iterate through available providers and test their class name.

@laeubi
Copy link
Contributor Author

laeubi commented Feb 15, 2025

the other way round could be interesting in combination with a way to specify the provider to use explicitly.

That is what I currently do, select one provider in the product configuration explicitly, the slf4j-simple is added additionally to what I already have selected :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't include more bundles if the requirement is already fulfilled laeubi/eclipse.pde
2 participants
@laeubi @HannesWell