Skip to content

Replace Pax Logging with Logback in Pax Exam tests #421

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

Merged
merged 3 commits into from
May 14, 2025

Conversation

glimmerveen
Copy link
Contributor

@glimmerveen glimmerveen commented May 12, 2025

Related to PR #420, on getting control on the logging. This changes disables Pax Exam's preference for Pax Logging, and changes it with Logback (in the latest SLF4j 1.7.x compatible version). With this change, SLF4j and Logback are now available for both the unit tests and the pax exam tests.

Configured explicit versions of all 'mavenBundle's declared in the ComponentTestBase, as otherwise Pax will depend on Maven's LATEST, which is not always compatible. By using fixed versions (compatible with what is in the pom file), I also fixed test Felix6274Test, that I noticed failing (both on master and in #419)

…nges disables Pax Exam's preference for Pax Logging, and changes it with Logback (in the latest SLF4j 1.7.x compatible version). With this change, SLF4j and Logback are now available for both the unit tests and the pax exam tests.

Configured explicit versions of all 'mavenBundle's declared in the ComponentTestBase, as otherwise Pax will depend on Maven's LATEST, which is not always compatible.
@glimmerveen glimmerveen mentioned this pull request May 12, 2025
…here are unresolved bundles within the test runtime. This to avoid trying to run in a test in an inconsistent/incomplete environment (as some of the bundles were unresolved due to an incompatible version being used).
@stbischof
Copy link
Contributor

stbischof commented May 13, 2025

This helps a lot!
I approve the run of the test. Usually will take 40 minutes..

@stbischof
Copy link
Contributor

@laeubi can you please review before i merge

@laeubi
Copy link

laeubi commented May 13, 2025

If it works and as only tests are affected it looks fine. Tools like dependabot still might complain/suggest updates. @grgrzybek can maybe give some more advice here regarding pax-logging/pax-exam updates in general.

@paulrutter
Copy link
Contributor

If it works and as only tests are affected it looks fine. Tools like dependabot still might complain/suggest updates. @grgrzybek can maybe give some more advice here regarding pax-logging/pax-exam updates in general.

That is a good point; for changes like these, we should be careful with merging dependabot updates.

@glimmerveen
Copy link
Contributor Author

glimmerveen commented May 13, 2025

If it works and as only tests are affected it looks fine. Tools like dependabot still might complain/suggest updates. @grgrzybek can maybe give some more advice here regarding pax-logging/pax-exam updates in general.

That is a good point; for changes like these, we should be careful with merging dependabot updates.

A couple observations/suggestions from my side:

  • I downgraded logback to 1.2.x as that is the latest logback version that is compatible with SLF4j 1.7.x. It is possible to go to a newer logback version (1.5.x), but in an OSGi runtime we'll need to add additional components in order to deal with SLF4j's 2.x use of ServiceLoader (SPIFly + ASM). I opted for keeping the changeset small, but if it is preferred, I can update the PR and incorporate this.
  • Currently I put the version of the bundles in the test code (as some were already managed in this way), it is also possible to extract the version from the Maven project (by adding org.apache.servicemix.tooling:depends-maven-plugin execution). Benefit of this approach is that the Maven dependencies are leading, and also aligns any dependabot updates, with what is actually used within the Pax Exam tests, and with the change from 789cb77 we also ensure that if updates no longer resolve, tests will fail (hopefully catching versions updates that are not simple drop-in replacements).

@glimmerveen
Copy link
Contributor Author

  • Currently I put the version of the bundles in the test code (as some were already managed in this way), it is also possible to extract the version from the Maven project (by adding org.apache.servicemix.tooling:depends-maven-plugin execution). Benefit of this approach is that the Maven dependencies are leading, and also aligns any dependabot updates, with what is actually used within the Pax Exam tests, and with the change from 789cb77 we also ensure that if updates no longer resolve, tests will fail (hopefully catching versions updates that are not simple drop-in replacements).

I have tried this locally already, and it is possible to directly connect the versions used within the Maven build with the versions used within the Pax Exam tests. This has the benefit that if a version is updated in Maven, it is reflected in the tests as well (and if the version is not compatible with the rest, the test will fail).

This setup where the Pax Exam tests also act as a consistency check for the bundles it uses, also has its effect on the version used during compilation: concrete example, in order for this to work, I needed to update the dependency to org.osgi.util.promise from 1.0.0 to 1.1.0 (because the latter is the minimum version required by org.osgi.util.pushstream, which in turn is a dependency from org.osgi.service.log version 1.4.0 (which was already used)). In order words, the versions used during compilation were not all mutually consistent at runtime.

I don't see an immediate downside to updating the version of promise, however it is a change that is reflected in the requirements of the SCR bundle; is it acceptable that Felix SCR to start requiring the promise API in version 1.1.0?

@stbischof
Copy link
Contributor

@tjwatson is it okay to use promise 1.1.0?

@tjwatson
Copy link
Member

@tjwatson is it okay to use promise 1.1.0?

I don't see an issue because promise 1.3.0 has been out for nearly 3 years and 1.0 version is pretty old.

I needed to update the dependency to org.osgi.util.promise from 1.0.0 to 1.1.0 (because the latter is the minimum version required by org.osgi.util.pushstream, which in turn is a dependency from org.osgi.service.log version 1.4.0 (which was already used)). In order words, the versions used during compilation were not all mutually consistent at runtime.

This is a rather unfortunate consequence of this approach. Nothing in SCR needs pushstream. That version of org.osgi.service.log 1.4.0 was the mistake when we included org.osgi.service.log.stream in the API artifact which then pulled in the dependency on org.osgi.util.pushstream. Later in org.osgi.service.log v1.5.0 the package org.osgi.service.log.stream got split out into its own API artifact org.osgi:org.osgi.service.log.stream so we could reduce the dependencies of the core org.osgi.service.log package to not pull in pushstreams.

SCR works perfectly fine without any org.osgi.service.log.stream or org.osgi.util.pushstream packages installed.

With all that being said, it should be fine to update our SCR dependency on org.osgi.util.promise to 1.1.0.

@stbischof
Copy link
Contributor

@paulrutter can i have your okay?

…taining these versions from the SCR maven project itself.

This relies on the depends-maven-plugin on generating a properties file, that is read by the .versionAsInProject() Pax mavenBundle option API.
Needed to update two existing dependencies, and add two test dependencies in order for the bundles used to be mutually consistent versions.
@glimmerveen
Copy link
Contributor Author

@tjwatson is it okay to use promise 1.1.0?

I don't see an issue because promise 1.3.0 has been out for nearly 3 years and 1.0 version is pretty old.

I needed to update the dependency to org.osgi.util.promise from 1.0.0 to 1.1.0 (because the latter is the minimum version required by org.osgi.util.pushstream, which in turn is a dependency from org.osgi.service.log version 1.4.0 (which was already used)). In order words, the versions used during compilation were not all mutually consistent at runtime.

This is a rather unfortunate consequence of this approach. Nothing in SCR needs pushstream. That version of org.osgi.service.log 1.4.0 was the mistake when we included org.osgi.service.log.stream in the API artifact which then pulled in the dependency on org.osgi.util.pushstream. Later in org.osgi.service.log v1.5.0 the package org.osgi.service.log.stream got split out into its own API artifact org.osgi:org.osgi.service.log.stream so we could reduce the dependencies of the core org.osgi.service.log package to not pull in pushstreams.

SCR works perfectly fine without any org.osgi.service.log.stream or org.osgi.util.pushstream packages installed.

With all that being said, it should be fine to update our SCR dependency on org.osgi.util.promise to 1.1.0.

I have pushed commit b61b3e4 that contains these specific changes.

@paulrutter
Copy link
Contributor

@paulrutter can i have your okay?

Yes, it seems my concern of updating dependencies is at least covered by having tests that would break in case it would be incompatible.

@stbischof
Copy link
Contributor

i am okay with separate commits, because they have value when show into the history.

@stbischof stbischof merged commit fe0772d into apache:master May 14, 2025
3 checks passed
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.

5 participants