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

Use slf4j from upstream Maven instead of Orbit #598

Merged
merged 1 commit into from
Oct 1, 2022

Conversation

mickaelistria
Copy link
Contributor

And bump to latest release

@mickaelistria mickaelistria marked this pull request as ready for review September 28, 2022 15:23
@mickaelistria mickaelistria linked an issue Sep 28, 2022 that may be closed by this pull request
2 tasks
@HannesWell
Copy link
Member

I assume it is very likely that we can't use SLF4J 2 yet because dependencies (if not of Platform then in the SimRel) are not yet ready for SLF4J 2.
In the meantime we could use SLF4J 1.7.36 from Maven-Central. If everything else is then ready we just have to bump the version here.

@mickaelistria
Copy link
Contributor Author

If Platform can move to slf4j-2, it will. SimRel will follow.

@mickaelistria
Copy link
Contributor Author

@HannesWell it looks like -unlike Orbit- upstream slf4j defines in its MANIFEST that 1 impl is required. OK, why not... I've added the nop one; but now it will cause that the nop will be included in Eclipse IDE. Does that still allow other bindings to work? I'm a bit confused by the API requiring 1 impl, I'm afraid it causes limitations in OSGi.

@HannesWell
Copy link
Member

HannesWell commented Oct 1, 2022

@HannesWell it looks like -unlike Orbit- upstream slf4j defines in its MANIFEST that 1 impl is required. OK, why not... I've added the nop one; but now it will cause that the nop will be included in Eclipse IDE. Does that still allow other bindings to work? I'm a bit confused by the API requiring 1 impl, I'm afraid it causes limitations in OSGi.

It will cause the nop-binding to be included in the Eclipse-Platform p2-repo, that's right. But as long it is only in the repo (and not in a feature) it should be ignored when it comes to building/launching products or assembling repos with deps, if another binding with 'higher priority' is available. For example if somebody specifies another binding in a feature included in a product or if an (transitively included) plugin requires a specific binding (like m2e.logback requires logback) and the resolver does a good job in minimizing the number of bundles, then the binding with 'higher priority/demand' should be used and the noop one should be ignored. Because M2E requires logback (as mentioned above) the noop binding probably/hopefully will not even make it into the SimRel repo and only logback and probably/maybe log4j will be present. Log4J is also required by other Eclipse projects like XText or Passage, like M2E requires logback.

that does not necessarily mean that this

Btw. could you please sort out the formatting changes, so that we get a smaller change? With that many changes it is hard to understand what has actually been changed. If the target should be reformatted, I suggest to do that in a dedicated commit.

Edit:

It is then the applications builders task to include the desired binding into the product explicitly (e.g. by including it into a feature). Bundles that are build against other logging APIs than slf4j can then be satisfied by corresponding bridges.

SLF4J is better in that sense since it does not have the impl package and just uses the Service Loader (Mediator).

@mickaelistria
Copy link
Contributor Author

Thanks for the insights. I've fixed the formatting and other unrelated changes. Is it OK to merge in your opinion?

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for the insights. I've fixed the formatting and other unrelated changes. Is it OK to merge in your opinion?

Yes it looks good, thanks!
Nevertheless we should keep an eye on the slf4j story in upcoming SimRel milestones/RCs until this has settled, to make sure it is as expected.

The main points are:

  • only one slf4j.api bundle. Probably the latest from Maven-Central.
  • Only one logging implementation per framework (logback, log4j-2, log4-1) and ideally only those from Maven-Central to make sure the approaches for binding/bridging used in the Orbit-bundles and those from Maven-Central is not mixed.

@mickaelistria mickaelistria merged commit 2a9abb3 into eclipse-platform:master Oct 1, 2022
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.

Migrate to SLF4J-2 from Maven-Central
2 participants