Remove -nouses directive from maven-bundle-plugin#581
Remove -nouses directive from maven-bundle-plugin#581garydgregory merged 1 commit intoapache:masterfrom
Conversation
In commit 740bcf1 the instruction '-nouses' was added (but without any rationale) but using that is extremely dangerous. Without any use clause, all packages are treated as independent from each other, that means the OSGi resolver is free to wire these packages to different classloaders if used by a consumer (or its dependencies). This now completely removes the offending clause from the instructions, if any project has a real usecase for this (even though I can't think of cases it would be useful) they still can enable it on their demands.
|
I would rather migrate from
|
Feel free to propose such a change, but this will break all current users of the common-parent so maybe better make this additional. In the end both use the same underlying library and felix-maven plugin has some features that bnd-maven plugin do not offer so it is not a 1:1 replacement, in practice there is often no advantage in using one over the other. In any way, using the |
Good point 💯 |
|
It's one thing to say it looks good to you, but did you test it against components? 😉 |
|
Before this change the exports of now they are: |
|
Maybe someone should ask Niall on the dev list why he needed the "nouses" clause... |
|
In our codebase it’s customary to remove them as they are hard to read and can lead to some broken resolves (i dont recall the details), so from me it would be apreciated to keep them off - but I dont know this particular case/reason. |
|
I just wanted to quote the spec might be a good first source for derive some normative things:
The most important part is this one here
That means that omitting them as "they are hard to read" is really worst thing one can do as it opens the bundle to all kind of class space inconsistencies (and these are really hard to debug). A Manifest is not meant to be read by users, so one better use some tools if really interested in the results. |
|
Maybe this is a basic over-specification problem baked in OSGi, but because a class depends on one class in a package doesn't necessarily mean it depends on the whole package. |
I don't understand this... you can't depend on a part of a package in java, so if you reference a class |
|
Of course you can. You might be conflating this with using JPMS and/or OSGi. |
I'm not sure what you are talking about here, in any way this is about OSGi metadata that is ignored by all other java code so it is quite useless to discuss what you theoretically can do in special cases, but then you are responsible for ClassCastException yourself. So the problem is not that you can't load the classes from different classlaoders, they will just not work together what is the whole purpose of So if you take the example from the spec
and there is no use clause then |
In OSGi you can depend on a single package, so you might get another package from the same bundle (jar) via other classloaders (in OSGi you can get one classloader per "wire" aka import relationship). (Not sure why JPMS is brought into this issue?) |
The package imnport in the meta data is always on the whole package. But the problem is different packages might not come the same way, even when they are provided by one bundle (however in practice if you declare most bundles toplevel thats a seldom issue - thats why we avoid the uses, but I can understand that the commons artifacts might want to be strict in what they declare. Just make sure to write a release note when artifacts use the new parent). |
|
@ecki |
Maybe:
Not sure if we also want to mention that it had that before. |
|
I just wanted to quote the spec
I agree thats the spec and it might be better for some corner cases where you have multiple bundles or cycles to declare them properly. (having said that it might not be enough to depend on the discovery of the bundle plugin for this).
I did not say “it reads better” was the only/main reason for the change in CP, I just know in our case its not a problem to remove them and it solved some issues (which I don’t recall .) – and it made the lists more manageable. But we really should know the real motivation before reverting it.
Therefore I cant veto the change, but it’s a good idea to warn about the changed behavior of all commons artifacts released with that new parent.
Gruss
Bernd
|
I think it might getting to detailed here, but yes that's exactly the problem. If you have no JPMS is just "similar" that each module (classpath) has similar restrictions (for good reasons), so the so called "split package" (e.g. having package A+B from different classlaoders/modules) is not allowed.
This is nothing new, the earliest mention in the specification I could found was Release 4 from August 2005, I would just add a note that it is no longer removed by default and if all risks are properly known might be enabled individually. |
No I mean i am not sure if in the release note warning about this revert if we also want to mention that it was present before (after all the |
|
It was added here at 2008 so if you can call this recent I'm not sure :-) So it might be that 2005 it was added by the OSGi spec, then it needed some time to show up in manifests and the people wanted to get rid of it because it was "different than before"... so 2008 this was disabled for commons.... and now 20 years later we can only guess. |
Yeah at least nobody had an issue with it since then (asuming that config from the parent is used in for all commons artifacts). But yes we don't need to mention that detail. |
|
Btw I just remembered a concrete problem, if a package has a wide surface its uses might drag in (depend on) packages which are in the actual environment not used, therefore increasing the dependency graph. (Not sure if it would fail if not provided). It’s arguable bad design to have such optional dependencies but as a dependent party you can’t be picky. I can imagine for the usual commons bundle that should be quite seldom, though. And also, in the case of our applications with a few thousand packages the resolver gets very slow and resource intensive if all of them had multiple uses declarations, |
|
That should not be a problem, as a package in use is just a hint to the resolver that says:
So if the package is not there (because its optional) or not used (because its only used by the provider) then nothing will be required. |
|
It sounds like we have concensus to bring this in. Let's wait another day or so, and if no one objects, I'll merge this in and create a release candidate. |
now state 'uses' definitions for package imports, this doesn't affect JPMS #581
|
I created the release candidate, please see the dev mailing list https://lists.apache.org/thread/3soxzd1bpg6z8vozww68phdcvj4mto0w |
|
Thank you! |
4 participants
In commit 740bcf1 the instruction '-nouses' was added (but without any rationale) but using that is extremely dangerous. Without any use clause, all packages are treated as independent from each other, that means the OSGi resolver is free to wire these packages to different classloaders if used by a consumer (or its dependencies).
This now completely removes the offending clause from the instructions, if any project has a real usecase for this (even though I can't think of cases it would be useful) they still can enable it on their demands.
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.