SplitPackageProcessor picks up duplicates via test-jar that are not actually in that test-jar

[famod]

Describe the bug

With 2.1.0, when running tests in some of our modules, we are seeing this:

2021-07-27 16:57:00,819 WARN  [io.qua.arc.dep.SplitPackageProcessor] (build-28) Detected a split package usage which is considered a bad practice and should be avoided. Following packages were detected in multiple archives: 
 - "de.somecompany.someproject.register.user" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.persistence.entity.id" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.catalog" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.somelib.persistence.transaction" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.somelib.logging" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.application.ehba" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.persistence.entity" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.password" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.application.smcb" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.confirmingauthority" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.somelib.persistence.entity.id" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.context" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.user.mapper" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.persistence.liquibase" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.persistence.entity.catalog" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.validation" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.persistence" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.file" found in [de.somecompany.someproject:register-core]
 - "de.somecompany.someproject.register.confirmingauthority.mapper" found in [de.somecompany.someproject:register-core]

What's interesting is that only a single artifact is listed (de.somecompany.someproject:register-core).
That register-core module is producing a test-jar that is present on the test classpath of the module that is issuing the warnings (here: register-api).
That test-jar of register-core has a distinct package, but maybe the processor is somehow confused with this?

Expected behavior

No such warnings.

Actual behavior

Many warnings.

How to Reproduce?

See #19030 (comment)

Output of uname -a or ver

Linux W4DEUMSY9003463 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
But also happens on Jenkins.

Output of java -version

OpenJDK 64-Bit Server VM AdoptOpenJDK-11.0.11+9 (build 11.0.11+9, mixed mode)

GraalVM version (if different from Java)

n/a

Quarkus version or git rev

2.1.0.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.1

Additional information

n/a

[quarkus-bot]

/cc @manovotn, @mkouba

[famod]

I guess I should debug this:

quarkus/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/SplitPackageProcessor.java

Line 56 in d0555c7

for (ClassInfo classInfo : archive.getIndex().getKnownClasses()) {

[mkouba]

What's interesting is that only a single artifact is listed (de.somecompany.someproject:register-core).

It's very likely that the jar has the same groupId and artifactId. @manovotn we should probably include the classifier if available.

[mkouba]

I don't think it's "overzealous". It does not fail the build but logs some warnings if something is wrong and if I'm not mistaken a split package is problematic not only for dependency injection but in general in Java 11+.

[famod]

I don't think it's "overzealous".

I don't mind changing the title (or if you do). 🙂

we should probably include the classifier if available.

We don't use the classifier, but <type>test-jar</type>. This type correctly ends up in the ApplicationArchive's AppArtifactKey`.

I did a quick debug session and I think the actual problem is that archive.getIndex().getKnownClasses() is returning way to many known classes!

We use jandex-maven-plugin for the main part of register-core. But for the test-jar we use src/test/resources/META-INF/beans.xml (because we also build a third jar from that module with even fewer classes, but that is not on the CP here).

Somehow, (it seems) that the index is "too big".

[mkouba]

We don't use the classifier, but <type>test-jar</type>. This type correctly ends up in the ApplicationArchive's AppArtifactKey`.

I see. So the type should be added as well ;-).

[famod]

The type is there already in AppArtifactKey! I have yet to check ApplicationArchive's hashCode.

PS: I don't see the processor doing anything with GAV etc., which is actually good in this case.

[famod]

I have yet to check ApplicationArchive's hashCode.

Interesting, it doesn't seem to have one: https://github.com/quarkusio/quarkus/blob/2.1.0.Final/core/deployment/src/main/java/io/quarkus/deployment/ApplicationArchiveImpl.java
But this doesn't really play a role here, actually.

Anyway, I think I now do understand why the index contains packages that are not in that test-jar:
We include only a part of src/test/java, not everything! So the index is probably looking at all of it, not just the classes that are actually in the test-jar.
/cc @aloubyansky just in case

[manovotn]

We don't use the classifier, but <type>test-jar</type>. This type correctly ends up in the ApplicationArchive's AppArtifactKey`.

I see. So the type should be added as well ;-).

Actually, the current information would have been enough, but as part of this commit an ordering was imposed which led to creation of the set that now has the problem with uniqueness of its elements.

Here is a PR that adds type and classifier to each archive - #19054

[aloubyansky]

@famod are you going to look into adding hashCode/equals to the ApplicationArchiveImpl?

[famod]

@manovotn

which led to creation of the set that now has the problem with uniqueness of its elements

How is that a problem here? Why should register-core (the main jar) be considered equal to register-core test-jar? Or am I getting you wrong?

@aloubyansky

are you going to look into adding hashCode/equals to the ApplicationArchiveImpl?

No time for that, sorry. As I stated previously, this is not the main problem here (which doesn't mean it shouldn't be addressed), it's the index for that test-jar that contains too many known classes.

[manovotn]

How is that a problem here? Why should register-core (the main jar) be considered equal to register-core test-jar? Or am I getting you wrong?

You are correct that they shouldn't be equal. All I am saying is that the Set<String> that is used in current impl introduces this problem because both your JARs will generate the same String.

[famod]

@manovotn @mkouba @aloubyansky can anyone of you point me to where an index is created when a beans.xml is found?
TBH, I'd prefer if someone with more knowledge in that area would pick up this task but I can at least try to support.

[aloubyansky]

I could have a look. There is no reproducer readily available, is there?

[famod]

No. I can try to hack something later.

[famod]

@aloubyansky I recycled an older example project (which has a weird name 😉 ): https://github.com/famod/modmono-quarkus/tree/issue-19030-test-jar-index (make sure to switch to branch issue-19030-test-jar-index!)

mvn clean install will yield during testing in dist module:

WARN  [io.qua.arc.dep.SplitPackageProcessor] (build-5) Detected a split package usage which is considered a bad practice and should be avoided. Following packages were detected in multiple archives:
- "com.github.famod.modmono_quarkus.core" found in [com.github.famod.modmono-quarkus:modmono-quarkus-core]

or with 999-SNAPSHOT a bit more detailed:

WARN  [io.qua.arc.dep.SplitPackageProcessor] (build-21) Detected a split package usage which is considered a bad practice and should be avoided. Following packages were detected in multiple archives:
- "com.github.famod.modmono_quarkus.core" found in [com.github.famod.modmono-quarkus:modmono-quarkus-core::jar, com.github.famod.modmono-quarkus:modmono-quarkus-core:tests:jar]

[famod]

So FTR, as discussed on Zulip, a solution could be to not re-index for test runs but to just use the existing index.

I have to say though that this might not work in my case because I have two test-jars and while I can tell jandex-maven-plugin to create an index for all test classes, I'm not sure I can have two different ones for the test "branch".