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

Don't add parent packages, only the ones that are actually present #76

Closed
wants to merge 1 commit into from

Conversation

bosschaert
Copy link

Previously the code added all the packages, also packages which were just found as parent directories in the Jar file. As packages aren't hierarchical this isn't correct. Only packages in which .class files are found should be added.

Previously the code added all the packages, also packages which were
just found as parent directories in the Jar file. As packages aren't
hierarchical this isn't correct. Only packages in which .class files
are found should be added.
@tjwatson
Copy link
Member

tjwatson commented Jan 2, 2024

What problem does this pose? I worry about packages they may contain resource only.

@karlpauls
Copy link
Contributor

I think the actual problem is that it currently adds directories - that makes it so that packages get claimed for a bundle that actually only has a sub-package. Let's say we have a bundle A with org/foo/Foo.class and bundle B with org/foo/bar/Bar.class. We don't want bundle B to be identified for the org.foo package (only for the org.foo.bar package). In other words, I guess I agree that the requirement for a .class file is too strong - it would be better to just require something else than a dir (i.e., filtering out all directories).

@tjwatson
Copy link
Member

tjwatson commented Jan 3, 2024

I see it adding too many directories with only other sub-directories (no files) , but I am unclear what observable consequence there is to this.

Refreshing my memory of this, the code only reads from this map packageToAtomosContent in the method org.apache.felix.atomos.impl.base.AtomosBase.getAtomosKey(Class<?>). So it will only really be used for looking up a package name that actually has a loaded Class. What I think the observable consequence may be is when a parent package is contained in another bundle and the order is such that the bundle with the child package is processed first and there for takes over the map entry for that package name. Is that something you all ran into when discovering this issue?

@rotty3000
Copy link
Contributor

when a parent package is contained in another bundle and the order is such that the bundle with the child package is processed first and there for takes over the map entry for that package name.

I've observed this exact problem when bnd's JPMS plugin was over stocking a module description with "packages" of empty dirs. However, you need to account for both resources and class files alike when computing what is a package and what is not.

.. and yet even this can go wrong. Think of jars which include schema files from other projects (looking at you JavaEE/Jakata and friends) so you should leave some form of escape hatch.

@karlpauls
Copy link
Contributor

Yes, the issue is that it is used for the lookup of the bundle that provides a given class (via FrameworkUtils). So in my scenario above, if bundle B gets the spot for org.foo it is returned for the bundle lookup of the Foo.class via FrameworkUtil.

if (s.length() > 1 && s.endsWith("/") && s.indexOf('-') < 0)
{
String pkg = s.substring(0, s.length() - 1).replace('/', '.');
if (s.endsWith(".class")) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this to be if (!s.endswith("/") && s.contains("/")) {

In other words all paths that are not directories (don't end with "/") and are not part of the default package (paths with no "/" at all).

Copy link
Author

Choose a reason for hiding this comment

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

I think that would probably work yes.

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