Migrate from deprecated org.eclipse.aether.impl.guice.AetherModule to Sisu
#1733
Conversation
And drop deprecated AetherModule.
|
This uses sisu w/ index. Basically whatever you "throw" onto classpath (by adding resolver Edit: clear up "module" (guice? jpms? maven?) -- point is, it now uses Sisu index, and if new sisu index is present on classpath, sisu will discover it and add to system. |
|
Also, using asm-less guice and sisu, and provided latest asm (as asm was already present as transitive). |
basil
changed the title
org.eclipse.aether.impl.guice.AetherModule to Sisu
There was a problem hiding this comment.
Many thanks for helping us across through this migration! I can't say I understand everything that is going on here, and getting even close to this would have taken me several days, so this is highly appreciated.
The comments I left are optional, for your consideration. I will leave this open a little longer in case other maintainers have thoughts about this, but as-is it looks good to me.
| <maven.version>3.9.6</maven.version> | ||
| <maven-sisu.version>0.9.0.M3</maven-sisu.version> |
There was a problem hiding this comment.
According to https://github.com/apache/maven/blob/maven-3.9.6/pom.xml#L130 Maven 3.9.6 uses 0.9.0.M2, so I think this should be 0.9.0.M2 in order to follow the comment two lines earlier ("Keep them aligned to Maven version"):
| <maven-sisu.version>0.9.0.M3</maven-sisu.version> | |
| <maven-sisu.version>0.9.0.M2</maven-sisu.version> |
Please feel free to ignore this suggestion if I have misunderstood the intention of the comment above.
There was a problem hiding this comment.
Yup, let be consistent and have them truly aligned. Also, checked, and M2 does provide no_asm as well:
https://repo.maven.apache.org/maven2/org/eclipse/sisu/org.eclipse.sisu.inject/0.9.0.M2/
There was a problem hiding this comment.
In reality does not stir much (as Sisu versions were more important due shaded in Asm, but now you use own ASM), also I don't want to kill current CI jobs, so please merge this suggestion before merging the PR
There was a problem hiding this comment.
Oh, 0.9.0.M2 does not have "strict" mode, so if downgrading, this will need change:
remove "false" after INDEX here https://github.com/jenkinsci/acceptance-test-harness/pull/1733/files#diff-14b9e71c9ede5549e786ac9e299d0b2da2969ff999f9c0a9ad55a3849d8abca2R40
|
Yes, both Guice and Sisu contains shaded in ASM. Since Guice 4 and Sisu 0.9.0.M2 (? unsure) both projects offer "no asm shaded" artifacts as well (w/ classifier, Guice classes and Sisu no_asm). Basically you had 3 ASMs on classpath (2 shaded and 3rd was some transitive). Now you have one ASM that is latest. |
| .in(Singleton.class); | ||
| @Override | ||
| protected void configure() { | ||
| ClassSpace space = new URLClassSpace(RepositorySystem.class.getClassLoader()); |
There was a problem hiding this comment.
Maybe inspect: is TCCL "more appropriate here"?
|
One more thing post merge @basil and @timja I just realized: But in any case, I am pretty much sure that So I think your lengthy tests could be mildly improved, if you split the two: keep |
Migrate to Sisu and drop use of deprecated AetherModule (is removed in Resolver 2.x).
This PR uses "fully fledged" Sisu to create
RepositorySysteminstance, and will discover (using sisu index) dynamically any JSR330 component that is present on classpath at the momentAetherModuleis being constructed.Also, Guice and Sisu are changed (by exclusion from transitive dep and explicit addition as direct dependency) to "no asm" versions (these have no shaded in ASM), and since ASM was already on path (was 9.2) updated to latest 9.7. Now Guice and Sisu and that something (that pulled ASM in orginally) all use same ASM version 9.7.
Testing done
CI build
Submitter checklist
Fixes #1730