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

[LOGGING-198] Add proper OSGi uses to export package #337

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

Conversation

laeubi
Copy link

@laeubi laeubi commented Jan 23, 2025

Currently there are no use clauses on the export package header, this can lead to classloading problems.

This now adds the appropriate use clause to the export header, due to a bug in commons-parent this currently can not be calculated automatically.

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible but is a best-practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that commits might be squashed by a maintainer on merge.

Currently there are no use clauses on the export package header, this
can lead to classloading problems.

This now adds the appropriate use clause to the export header, due to a
bug in commons-parent this currently can not be calculated
automatically.
@laeubi
Copy link
Author

laeubi commented Jan 23, 2025

I created a backport of this (but sadly there is no way to create a PR yet as no branch exits in this repo):

https://github.com/laeubi/commons-logging/tree/1.x

to create the base branch I did:

git fetch --all --tags
git checkout -b 1.x LOGGING_1_2

then applied my changes, so if the 1.x branch could be create in this repo I should be able to offer a PR for this as well.

@laeubi
Copy link
Author

laeubi commented Jan 23, 2025

@garydgregory can you take a look here?

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.

Looks good to me.

I am just wondering if we can introduce some integration tests that prevent this issue from happening again.

@garydgregory
Copy link
Member

garydgregory commented Jan 23, 2025

Looks good to me.

I am just wondering if we can introduce some integration tests that prevent this issue from happening again.

Yes indeed. Please add something like https://github.com/apache/commons-compress/tree/master/src/test/java/org/apache/commons/compress/osgi
Otherwise, this will keep on happening.

@garydgregory garydgregory changed the title LOGGING-198 - add proper uses to export package [LOGGING-198] Add proper OSGi uses to export package Jan 23, 2025
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@laeubi
Copy link
Author

laeubi commented Jan 23, 2025

I don't think this is something I can offer here, it will require a whole bunch of setups and different version to actually show the problem of bad meta-data.

@garydgregory
Copy link
Member

I don't think this is something I can offer here, it will require a whole bunch of setups and different version to actually show the problem of bad meta-data.

Are you saying that tests like the Commons Compress example would not catch this kind of issue? I guess we can't know until a test exists...

@laeubi
Copy link
Author

laeubi commented Jan 23, 2025

Are you saying that tests like the Commons Compress example would not catch this kind of issue? I guess we can't know until a test exists...

The commons compress test only the bundle itself, this issue you will only see when different bundles are used, e.g. to show the problem in this test you will need (or maybe similar):

  1. this bundle in version 1.2 and 1.3
  2. a bundle importing 1.2 api but 1.3 impl
  3. a bundle using that bundle to emit a log message

What then (if anything setup correctly) will happen is that it gets wired to 1.2 API but 1.3 impl and when the log message is issued you will see java.lang.ClassCastException somewhere logged (maybe on system out) and you hopefully can capture this in an assert. So rather complex and convoluted setup ... I don't say its impossible but I question the usefulness, because once the problem is solved you won't be able to run the test anymore because then the OSGi framework itself will not run the test anymore because it detects the problem before and refuses to start the bundles at all :-)

@garydgregory
Copy link
Member

Hello @laeubi and all:

Please test 1.3.5-SNAPSHOT from https://repository.apache.org/content/repositories/snapshots/ or a local build from git master

@laeubi
Copy link
Author

laeubi commented Jan 29, 2025

If I download the snapshot I still see no uses:

Export-Package: org.apache.commons.logging;version="1.3.5.SNAPSHOT",
 org.apache.commons.logging.impl;version="1.3.5.SNAPSHOT"

@laeubi
Copy link
Author

laeubi commented Jan 29, 2025

With a local build I see this:

Export-Package: org.apache.commons.logging;version="1.3.5.SNAPSHOT",org.
 apache.commons.logging.impl;version="1.3.5.SNAPSHOT";uses:="javax.servl
 et,org.apache.avalon.framework.logger,org.apache.commons.logging,org.ap
 ache.log,org.apache.log4j"

so it looks like the snapshots are not yet updated!?

@garydgregory
Copy link
Member

@laeubi
Copy link
Author

laeubi commented Jan 29, 2025

@garydgregory the download is looking good now!

@laeubi
Copy link
Author

laeubi commented Jan 29, 2025

Is there any plans for a new 1.3.5 release?

And then only question remains is there any chance for a fix in the 1.2.x ... I assume the main problem would be to actually build the release, but it would also work to just download the old jar and update the manifest + version manually.

For 1.2.x the manifest should look like this:

Bundle-Version: 1.2.1
Export-Package: org.apache.commons.logging;version="1.2.1",org.apache.commons.logging.impl;uses:="javax.servlet,org.apache.avalon.framework.logger,org.apache.commons.logging,org.apache.log,org.apache.log4j";version="1.2.1"

@garydgregory
Copy link
Member

@laeubi
I'll cut a 1.3.5 release candidate over the next couple of days. Why do want a 1.2.x release?

@laeubi
Copy link
Author

laeubi commented Jan 29, 2025

I'll cut a 1.3.5 release candidate over the next couple of days.

Great!

Why do want a 1.2.x release?

Some people still use 1.2.x and it has the same problem in manifest

grafik

so we came up with version 1.2.x and 1.3.x used in the same framework and that was where I discovered the issue initially, but I can understand if you say they should better upgrade than care too much about old versions.

@garydgregory
Copy link
Member

@laeubi
Because a new version on a release line exists, it does not mean projects will update. Many projects still use the version of Log4j that has Log4Shell, despite fixes being available on all release lines.

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.

3 participants