smallrye
Add an ASM-shaded version of SmallRye Config
This PR also includes
- a fix for a property to match what's in to-jakarta.sh
- a local definition to avoid failures on non-english systems
I did checkout the PR in a /tmp/smallrye-config directory, ran the to-jakarta.sh script and then compared the current smallrye/smallrye-config/jakarta branch with the /tmp/smallrye-config and I can only see the changes I did in the PR and the fix
Isn't ASM forwards-compatible? IOW it should be okay for any consumers to use a later version, should it not?
ASM is a low level tool used by a vaste set of tools and app servers. It's sometimes tricky or difficult to align all versions to the latest.
ASM is a low level tool used by a vaste set of tools and app servers. It's sometimes tricky or difficult to align all versions to the latest.
Can you elaborate a bit? Shading is a bit of an extreme measure for an API which is aggressively compatible.
@dmlloyd I think that the intention is to avoid classpath conflicts (which is quite common in an app server context) with different versions of ASM provided by 3rd party libs. It is sometimes difficult to align these versions or exclude them.
@dmlloyd I think that the intention is to avoid classpath conflicts (which is quite common in an app server context) with different versions of ASM provided by 3rd party libs. It is sometimes difficult to align these versions or exclude them.
I understand the intention, but what I'm skeptical of is that it actually happens in practice. Like I said, ASM is aggressively compatible; do we have a concrete example where upgrading ASM to a version equal to or later than every library's requirement does not actually work in practice?
I think shading ASM (or other libraries) sets a bad precedent and leads to bloated deployments, and so should be very carefully considered.
@dmlloyd I think that the intention is to avoid classpath conflicts (which is quite common in an app server context) with different versions of ASM provided by 3rd party libs. It is sometimes difficult to align these versions or exclude them.
I understand the intention, but what I'm skeptical of is that it actually happens in practice. Like I said, ASM is aggressively compatible; do we have a concrete example where upgrading ASM to a version equal to or later than every library's requirement does not actually work in practice?
ASM 1, 2, 3, and 4 were all backwards incompatible. We've used ASM in OpenEJB/TomEE since version 1 at got a lot of complaints from users who had different versions in their app. It was the ASM team themselves that recommended shading and for a while that was mentioned on their website. We started shading at I think version 3 or 4. I went to check for that mention again and it looks like now there's a statement saying everything after 4 is backwards compatible with 4, so it seems they did in fact have a change of heart on the concept of backwards compatibility.
Looks like people would be fine unless their app needs ASM 1, 2 or 3.
ASM 1, 2, 3, and 4 were all backwards incompatible. We've used ASM in OpenEJB/TomEE since version 1 at got a lot of complaints from users who had different versions in their app. It was the ASM team themselves that recommended shading and for a while that was mentioned on their website. We started shading at I think version 3 or 4. I went to check for that mention again and it looks like now there's a statement saying everything after 4 is backwards compatible with 4, so it seems they did in fact have a change of heart on the concept of backwards compatibility.
Looks like people would be fine unless their app needs ASM 1, 2 or 3.
OK, that makes sense, thanks for the information.
Given the age of ASM 3 and its predecessors, I think at this point it makes more sense for programs which still rely on these older version (if any) to shade or upgrade than for libraries to do so. @jeanouii, is your use case involving ASM version 3 or earlier?
That was a complain from a user indeed. TomEE is an app server based on Tomcat. We don't really know or can control what users will be deploying. It may be an old version of Spring using an old version of ASM.
We have other libraries using ASM that we integrate but for such use case, they tend to provide an ASM-shaded version to avoid conflicts.
We can of course repackage under our own groupdId/artifactId the SmallRye Config library to include ASM shaded with it. But I was thinking it would benefit more users to avoid conflicts.
Hey @jeanouii
I'm having some issues building this in my local environment. The shade plugin installs the dependency reduced pom in place of the original pom, so when referring to the GAV without the classifier, that are things missing. I have no idea why this works on the CI. I had to add createDependencyReducedPom=false to make it build locally (maybe Maven version?).
I'm wondering if we should just shade ASM in separate module to avoid any other issues that we are not aware of. Also, do we want to shade the entire SmallRye Config namespace? Maybe shading smallrye-config-core in a separate module with ASM and then referencing it via the classifier and excluding ASM would be enough?
Hi Roberto,
Maybe the separate module approach is better to avoid side effect. Agreed.
We can restrict the usage to the core module. It's a bit more difficult to exclude and add the shaded version but that is doable. I'm fine with it as well.
Thanks a lot for moving this forward.
Doing it in the core module was just a suggestion since it is the one that directly depends / uses ASM. I guess that the artifact that you use in TomEE is the one that has the CDI integration. It is also acceptable if you prefer to do it in a separate module using the CDI integration dependency.
Yes I use the CDI for integration and I think overall for endusers, it's easier if we do it in the CDI module
Hum, didn't we agree to do this in a separate module to avoid some of the observed side effects?
Ah ok, of course. I can do the change. I wasn't sure about what you were saying.