jbangdev / jbang

Unleash the power of Java - JBang Lets Students, Educators and Professional Developers create, edit and run self-contained source-only Java programs with unprecedented ease.
https://jbang.dev
MIT License
1.43k stars 159 forks source link

fix: fix dependency resolution re depMgmt #1853

Closed cstamas closed 1 week ago

cstamas commented 3 weeks ago

Issue is related how Maven3 works, as JBang creates "fake" root and adds direct dependencies to it. This in Maven3 implies that depMgmt entries of added dependencies are NOT applied.

Example: jbang io.quarkus:quarkus-tls-registry:jar:3.15.1 will use Jackson 2.16... instead of wanted 2.17...

Fix: I missed that depMgmt of BOMs is already slurped up, so basically just do the same to all deps, while BOMs are added first (to prevail). Also, no need to "apply" depMgt and any sort of magic, just let Resolver it's job. There is need for magic, as unlike Maven, JBang allows use of GA and will deduce V from BOM.

Example now uses Jackson 2.17... as can be seen in gist w/ PR applied locally: https://gist.github.com/cstamas/024db87af357b658492a9d9b60857bfa

cstamas commented 3 weeks ago

Oh, "I see what have you done" :smile: returning "magic" as JBang is not Maven and it allows use of GA it seems...

quintesse commented 3 weeks ago

Oh, "I see what have you done" 😄 returning "magic" as JBang is not Maven and it allows use of GA it seems...

Could you explain what "we have done"? 😅 What does "allow use of GA" mean and why does Maven not allow it?

cstamas commented 3 weeks ago

Expressing something like this is impossible in Resolver (or at least, ModelBuilder "fills in the gaps" if needed, but that is Maven not Resolver alone): https://github.com/jbangdev/jbang/blob/main/src/test/java/dev/jbang/cli/TestEdit.java#L121-L123

In short: import a BOM and then infer V for GA from it. (to clarify: doing this in Maven POM is possible but I was and am sticking to Resolver APIs, that is invoked AFTER Maven ModelBuilder processes the POM into model, and performs these substitutions... Resolver "alone" is NOT doing these, that's what I tried to say here.)

quintesse commented 3 weeks ago

Is there a way you think we should have done it to be able to work with BOMs?

cstamas commented 3 weeks ago

Is there a way you think we should have done it to be able to work with BOMs?

Unsure what is the right answer here :smile: But I dislike that @pom means BOM. As I understand the syntax is @type (ie. @fatjar), and while POM is type "pom", what happens if you want to use POM that is not BOM, but instead is grouping dependencies (a common pre-BOM pattern)?

quintesse commented 3 weeks ago

Is there a way you think we should have done it to be able to work with BOMs?

Unsure what is the right answer here 😄 But I dislike that @pom means BOM. As I understand the syntax is @type (ie. @fatjar), and while POM is type "pom", what happens if you want to use POM that is not BOM, but instead is grouping dependencies (a common pre-BOM pattern)?

Yeah, @maxandersen would be able to explain this better, but IIRC it was done because we needed to treat BOMs differently and we thought "pom" as a type wasn't actually used by Maven so we could use it ourselves. Only years later we found out that was incorrect.

We're still discussing if this is something we can "correct" (JBang is not 1.0 yet so we could make a breaking change).

maxandersen commented 1 week ago

okey - this fix almost looks too simple to be good but I can't find fault with it.

It will cause some changes to users dependency resolution but I think to the better.

What it does is that JBang now will honor any dependencies management section (which is what Maven4 will do to) meaning that when you do jbang agav:1.3.2 will result the same as if import scoped agav:1.3.2 and all its dependencies managed dependencies first and then resolve.

Where as before it would not honor the managed deps of transitive dependencies.

This makes things much more consistent and remove/reduce need for having to use bom-pom's as one can now just rely on the included dependency owns managed dependencies.