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

Unconfigure slf4j.simple when m2e.logback.feature is installed #1534

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

Conversation

HannesWell
Copy link
Contributor

Disable slf4j.simple when present during the installation of the m2e.logback feature.
This prevents warnings about multiple slf4j providers found in the runtime and the potential to have slf4j.simple being selected as provider in use.

This scenario can for example happen if one installs m2e into an Eclipse SDK, as mentioned in eclipse-packaging/packages#43 (comment).

I didn't want to uninstall slf4j.simple since I assume this will cause problems when m2e.logback is later uninstalled.

Unfortunately, when I tested this with a local build slf4j.simple was not changed. My guess is that the metadata of the instructions for the product are applied after those of the m2e.logback feature and therefore the changes from it are effectively undone, since the product configuration sets the start-level again. But I'm not sure about that.
@laeubi can you tell what I'm doing wrong?
@jonahgraham FYI and if you have a hint about the p2-advices it is of course welcome.

@jonahgraham
Copy link

Thanks @HannesWell for pursuing this.

@jonahgraham FYI and if you have a hint about the p2-advices it is of course welcome.

Nothing to add :-( I'm not enough of a p2 expert to be able to add to what you have started here.

@github-actions
Copy link

github-actions bot commented Aug 28, 2023

Test Results

107 files   - 107  107 suites   - 107   10m 24s ⏱️ - 7m 52s
670 tests ±  0  649 ✅  -   1  18 💤 ± 0  2 ❌ +1  1 🔥 ±0 
670 runs   - 670  649 ✅  - 653  18 💤  - 18  2 ❌ +1  1 🔥 ±0 

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

Results for commit fe41662. ± Comparison against base commit 193f9b0.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented Aug 29, 2023

I don't think this can/should work on the m2e level, there are more slf4j bindings than "simple" and now you will even pull in "simple" for products never have used that before. So this has to be addressed in the product config of the corresponding EPP.

@HannesWell
Copy link
Contributor Author

I don't think this can/should work on the m2e level, [...] So this has to be addressed in the product config of the corresponding EPP.

For products that have m2e.logback installed it is already handled at the product level.
But how should it be handled if the product initially only has slf4j simple installed, like the SDK, and then the User installs m2e.logback?
Without any p2-instructions this m2e.logback will not work reliably and as far as I know there is nothing a user can do in the UI and modifying the bundles.info seems to be a bit low level.

there are more slf4j bindings than "simple"

That's right, but the EPP products now only use simple or m2e.logback.

now you will even pull in "simple" for products never have used that before.

That's not desired. I just want to do that if it is present. Would it suffice to make that non greedy?

@laeubi
Copy link
Member

laeubi commented Aug 29, 2023

As said I can't think about a way to do this on the p2 level from a p2 inf... One maybe can write a custom touchpoint but that's highly fragile and I'm not sure if one can use just-installed-touchpoints.

justj uses negative requirements (@merks can tell more) that might work but this will uninstall slf4j simple and actually one needs a negative requirement for any other slf4j provider.

Without any p2-instructions this m2e.logback will not work reliably and as far as I know there is nothing a user can do in the UI and modifying the bundles.info seems to be a bit low level.

The logback binding of m2e is, was and will always be a facility for expert users, thats why I always strongly have discouraged using it as a "user plugin", so one can assume that if one uses it outside of the predefined cases people should be able to handle the case properly.

@merks
Copy link
Contributor

merks commented Aug 29, 2023

Yes, a negative requirement will exclude/uninstall the thing matching that requirement. Note that the requirement can be any type of capability or property requirement; I'm not sure if that helps. I don't actually understand all provided capabilities involved...

It is possible to have meta-requirements that install bundles with touchpoints and in principle that should work, but this is definitely off the beaten path.

When multiple different IUs use touchpoints to configure the same thing differently, there is no telling which one will win:

  <touchpointData size='1'>
    <instructions size='4'>
      <instruction key='uninstall'>
        uninstallBundle(bundle:${artifact})
      </instruction>
      <instruction key='install'>
        installBundle(bundle:${artifact})
      </instruction>
      <instruction key='configure'>
        setStartLevel(startLevel:2);markStarted(started: true);
      </instruction>
      <instruction key='unconfigure'>
        setStartLevel(startLevel:-1);markStarted(started: false);
      </instruction>
    </instructions>
  </touchpointData>

@HannesWell
Copy link
Contributor Author

As said I can't think about a way to do this on the p2 level from a p2 inf... One maybe can write a custom touchpoint but that's highly fragile and I'm not sure if one can use just-installed-touchpoints.

But the already existing content of the p2.inf already does something very similar, it marks logback.classic as started and sets its startlevel two 2. I don't know why this needs so much configuration, but I didn't manage to achieve this with less.
And for slf4j.simple I want to to the same (just with opposite values), but only if it is already present. I was hoping that making the IU that does that optional and non-greedy would have that desired effect.

Without any p2-instructions this m2e.logback will not work reliably and as far as I know there is nothing a user can do in the UI and modifying the bundles.info seems to be a bit low level.

The logback binding of m2e is, was and will always be a facility for expert users, thats why I always strongly have discouraged using it as a "user plugin", so one can assume that if one uses it outside of the predefined cases people should be able to handle the case properly.

Yes the goal of this change is need less user-handling and thus 'hide' it better. But it is necessary to make the Maven-Console work.

Since I just released m2e 2.4.0 as planned, this will need to go into the next release anyway.
I had the intention to at least remove the provided logback configuration-file and make m2e.logback work with any logback-classic configuration provided.
If you have specific suggestions I can consider that as well.

When multiple different IUs use touchpoints to configure the same thing differently, there is no telling which one will win:

IIRC the IUs are sorted alphabetically by their ID within one phase? At least I can remember that in the past I could solve such ordering problem by changing the id to start with zzz.. But I'm not sure if this was just a special case.

@HannesWell HannesWell force-pushed the unconfigureSlf4jSimple branch from 389efd2 to 7b93696 Compare August 29, 2023 21:27
@laeubi
Copy link
Member

laeubi commented Aug 30, 2023

But the already existing content of the p2.inf already does something very similar, it marks logback.classic as started and sets its startlevel two 2

The point is that "something" happens on install of "something", so you can try to set the startlevel of slf4j-simple while installing logback, but currently you install slf4j -simple to set it to not autostarting but optional (=P2 can leave it out) and non-greedy (=p2 will leave this out).

Another approach would be to do what you try here not on install time but on runtime so letting the m2e-logback bundle search for other providers and setting their startlevel / autostart accordingly:
https://docs.osgi.org/specification/osgi.core/7.0.0/framework.startlevel.html
https://wiki.eclipse.org/Configurator#Configurator_Manipulator

@HannesWell HannesWell force-pushed the unconfigureSlf4jSimple branch from 7b93696 to 0656bff Compare February 17, 2024 21:46
Disable slf4j.simple when present during the installation of the
m2e.logback feature.
This prevents warnings about multiple slf4j providers found in the
runtime and the potential to have slf4j.simple being selected as
provider in use.
@HannesWell HannesWell force-pushed the unconfigureSlf4jSimple branch from 0656bff to fe41662 Compare September 28, 2024 17:25
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.

4 participants