[http-client] Shade Core Differently

[SentryMan]

Now doesn't have clashing packages with the other generators. Resolves #741

[rbygrave]

Yeah, I'll approve but why do we need shading in the first place here? [as there are no annotations that the prisms need in these packages right? so why the shading in the first place?]

[SentryMan]

classes from http generater core is being used, if we unshaded it's possible for the classes used to not exist and thus fail compilation

[SentryMan]

It relates to #120, where in that case the generator had a hard dependency on jackson and yet failed to find jackson classes while compiling in my project. From this I learned that relying on dependencies inside an AP is no bueno.

[rbygrave]

relying on dependencies inside an AP is no bueno ... use the AP through the maven compiler plugin it works fine.

Hmm, well imo this [AP through the maven-compiler] is the way the world is moving and especially maven 4. Need to keep that in mind.

[SentryMan]

imo this [AP through the maven-compiler] is the way the world is moving

What makes you say that?

[rbygrave]

The proc=full change which follows the theme of "Integrity by default", is suggesting to me that the Java architects ultimately want to move to explicit opt in for annotation processors. In maven that means registered with the maven-compiler-plugin ... and with maven 4 this is going to be both easy and explicit.

So yes the "provided scope implicit registration of annotation processors" ... is going to be increasingly discouraged.

[cbarlin]

One of the things I've found in my experience is that IDEs have a very hard time working out what you mean when marking a dependency as <scope>provided</scope> and having it in the module-info as requires static - you can unintentionally end up requiring something "for real" pretty easily.

However, because neither of those is transitive what I ended up doing in my annotation processor is creating a dependency for the generator that only contains the generated prisms (think http-generator-prisms) and nothing else. Since none of the items used to create those prisms (e.g. jackson) are available when compiling the actual processor it becomes very apparent if you've accidentally added a hard dependency. It's a bit ugly since there's an extra pom/dependency/etc, but it made the "line" very clear.

the generator had a hard dependency on jackson and yet failed to find jackson classes while compiling in my project.

I'm not sure if maven doesn't pull transitive dependencies of something that's provided, but that's what this sounds like. I can kinda see the logic as to why that would be the case - since everything that needs must also be provided ("provided" = "provided by the platform you'll run this on"), and it's the only item you're using, why grab anything else?

The proc=full change

Having proc=full causes all sorts of interesting things, in addition to the "Integrity by Default" angle. Two of the more interesting side-effects I've seen are:

• ProcessingEnvironment.getElementUtils() has full access to everything on both the project side and the annotation processor side, whereas without proc=full it can only look on the project side. If the annotation processor depends on, say, Jackson, ProcessingEnvironment.getElementUtils would only be able "find" a TypeElement for one of Jackson's classes if it's also a dependency of the project (or if proc=full)
• Depending on your test setup (have maven configured to run them in the same VM as it runs in), if both an annotation processor and your tests try and load something via the service loader it's possible for your tests to fail because the service loader "remembers" that a class exists but can't find it because it was only available for the annotation processor

Still, I get that dropping the shading is a potentially breaking change - especially since the documentation on the Avaje website does things via proc=full. Perhaps dropping the shading is something that can be done as part of a major version change? Though, having it shaded doesn't hurt anyone as long as the packages don't conflict and shading doesn't stop you from using it on the maven 3 additionalProcessorPaths/maven 4 <type>processor</type>...

[SentryMan]

is suggesting to me that the Java architects ultimately want to move to explicit opt in for annotation processors.

What do you mean want to, they've already done it. APT is disabled unless you use proc=full or the maven annotation processor paths. Do you anticipate that that they will remove the flag from javac?

One of the things I've found in my experience is that IDEs have a very hard time working out what you mean when marking a dependency as provided

My issue was on the command line, not an IDE.

and having it in the module-info as requires static

Jackson was requires transitive at the time. If you mean the generator itself, you should not be adding the generator to your module-info, static or not.

I'm not sure if maven doesn't pull transitive dependencies of something that's provided, but that's what this sounds like.

Oh no it does, only transitive provided dependencies of a provided dependency are not pulled. Which made the issue even more strange.

Perhaps dropping the shading is something that can be done as part of a major version change?

Don't feel like it, as I exclusively use proc=full.

[rbygrave]

the documentation on the Avaje website does things via proc=full

To be clear SentryMan and I disagree on the proc=full. I don't use proc=full anywhere these days. I'm ok'ish with the docs as they are but I'm super keen for Maven 4 which then really cleans this part up I think.

Perhaps dropping the shading is something that can be done as part of a major version change?

Yes, I think it is actually a good idea to look at this. As I see it, shading is kinda a hack and it will likely bite us at some point in the future so yes I think we need to look at removing it. The "dependency ... that only contains the generated prisms" sounds like a good option.

[SentryMan]

it will likely bite us

if and when it does then I will gladly discard it. Intentionally introducing a breaking change on a mere "maybe" is unpalatable for me. The baseline for this project is java 11, the proc full change only happened in jdk 23+. Most people in my estimation are using java 17/21.

[rbygrave]

breaking change

Why do you think its going to be a breaking change to remove the shading?

[SentryMan]

Because of the edge case where the project can fail to compile when using the provided scope due to missing classes.

[rbygrave]

Yeah, I want to actually see that / do not understand what you mean yet.

only transitive provided dependencies of a provided dependency are not pulled.

This statement didn't actually make sense to me, in that a annotation processor isn't going to dependency that has provided scope - this is the thing to avoid (so we avoid it).

So I don't see where the issue is yet.

[SentryMan]

Look at #120 again, we did not have Jackson as a provided dependency and yet still the generator failed to find the classes

[rbygrave]

Yeah, and that is extremely weird right, as in not expected at all ... so that makes me want to see if we can actually reproduce that somehow.

To me, it feels like you are approximately saying annotation processors can't have any dependencies and I'm saying - "Nah mate, that should work fine" and I'm unconvinced.