Introduce a processor that checks for split package issues #18617

manovotn · 2021-07-12T14:52:36Z

Resolves #18510, resolves #18553

Introduces an additional processor that checks all application archives and tries to detect split package issues. This is because split package is a bad practice that can cause several issues within Arc and is unsupported.
If any such packages are found, we log a warning.

Note that this is still just a best effort because, as described in #18510 (comment), there can be a 3rd party library which is not indexed but some classes from it will still be part of the application.

Tested with tweaked reproducer presented here - #18510 (comment)

Example output:

[WARNING] [io.quarkus.arc.deployment.SplitPackageProcessor] Detected a split package usage which is considered a bad practice and should be avoided. Following packages were found in multiple archives - [com.me.app.sub]

quarkus-bot · 2021-07-12T14:52:37Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not end up with dot

This message is automatically generated by a bot.

quarkus-bot · 2021-07-12T14:58:45Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `c5e5873`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	`Build`	⚠️ Check →	Logs	Raw logs

quarkus-bot · 2021-07-12T15:00:01Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `320e475`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	`Cache Maven Repository`	⚠️ Check →	Logs	Raw logs

geoand

LGTM

quarkus-bot · 2021-07-12T18:18:30Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `e3eac3d`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 16	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/reactive-pg-client/deployment

✖ io.quarkus.reactive.pg.client.PgPoolProducerTest. - More details - Source on GitHub

mkouba · 2021-07-13T09:34:28Z

Did you try to run this new build step with some quickstarts so that we can eventually estimate the impact on perf? If you run the build with -X you should at least see the approx. time the build step takes, e.g. something like:

Finished step "io.quarkus.vertx.http.deployment.VertxHttpProcessor#finalizeRouter" in 5 ms

@geoand I think that there was even an option to dump a report with build step times or something like that, or?

geoand · 2021-07-13T09:35:39Z

@geoand I think that there was even an option to dump a report with build step times or something like that, or?

Yup, see https://quarkus.io/guides/writing-extensions#printing-step-execution-time

mkouba · 2021-07-13T09:36:07Z

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

+            splitPackages.addAll(singleArchiveSplitPackages);
+        }
+        if (!splitPackages.isEmpty()) {
+            LOGGER.warnf("Detected a split package usage which is considered a bad practice and should be avoided. " +


I think that it could more useful to list the application artifacts for a specific split package. WDYT?

@mkouba Not sure I understand. Do you mean listing out all classes which have split package issues?
I thought that might get too verbose because you can easily have 50 classes inside given package in your app and only a singular class in lib (with same package) but you end up listing 51 classes as opposed to typing out a single package that causes the issue.

I mean something like:

Detected a split package usage which is considered a bad practice and should be avoided. Following packages were found in multiple archives: - "com.me.app.sub" found in [archiveA, archiveB] - "org.acme.foo" found in [archiveC, archiveB]

mkouba · 2021-07-13T09:44:00Z

@geoand I think that there was even an option to dump a report with build step times or something like that, or?

Yup, see https://quarkus.io/guides/writing-extensions#printing-step-execution-time

Hm, but this only works for the recorder methods executed at runtime, right?

geoand · 2021-07-13T09:50:16Z

Hm, but this only works for the recorder methods executed at runtime, right?

Correct

manovotn · 2021-07-13T10:05:33Z

Did you try to run this new build step with some quickstarts so that we can eventually estimate the impact on perf? If you run the build with -X you should at least see the approx. time the build step takes, e.g. something like:
Finished step "io.quarkus.vertx.http.deployment.VertxHttpProcessor#finalizeRouter" in 5 ms
@geoand I think that there was even an option to dump a report with build step times or something like that, or?

No, I didn't. Will pick few random quickstarts and report back; most likely later today.

manovotn · 2021-07-13T15:19:56Z

@mkouba I have added a second commit that outputs what you suggested. There is a TODO catch in the code though. Apparently in some cases (and I don't know what those cases can be) the application archive is "artificial" making it impossible to write out nice message. So with the current state, you'd get something like:

[WARNING] [io.quarkus.arc.deployment.SplitPackageProcessor] Detected a split package usage which is considered a bad practice and should be avoided. Following packages were detected in multiple archives: 
- "com.me.app.sub" found in [com.me:demo-lib, /target/classes]

The /target/classes is a hardcoded term I added because that's how it work for a test mode scenario. I am not sure there are other cases but still, this output should be more informative than the previous state :-)

As for the perf comparison, I didn't get to it yet but don't worry, it hasn't vanished from my TODO list.

mkouba · 2021-07-13T15:33:57Z

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

+                    AppArtifactKey a = iterator.next().getArtifactKey();
+                    if (a == null) {
+                        // can be null for instance in test mode where all application classes go under target/classes
+                        // TODO not sure if this can happen in more cases?


So it should be easy to detect the root archive if you compare the ApplicationArchivesBuildItem.getRootArchive(). And for other archives we could probably just use the first path from the ApplicationArchiveImpl.getPaths() collection? And if empty then just output unknown archive. @geoand WDYT?

Yeah, that sounds reasonable to me

Good point with root archive but you still need to somehow represent it in the output and I am not sure saying "current project" would give user much enlightenment :D

just use the first path from the ApplicationArchiveImpl.getPaths()

How is the first path most reliable and how do you ever get empty paths?

And for other archives...

Well, I found the group and artifact IDs to be easily understandable. Or do you think otherwise?

Good point with root archive but you still need to somehow represent it in the output and I am not sure saying "current project" would give user much enlightenment :D

target/classes is OK, except it does not make sense for gradle users ;-). Anything like "root application archive" or "application classes" is IMO good enough.

Well, I found the group and artifact IDs to be easily understandable. Or do you think otherwise?

Of course, if it's available then just use it! I was just talking about the branch where a == null...

How is the first path most reliable and how do you ever get empty paths?

I'm not quite sure TBH :-) but it seems that for most of the archives the first path is the "archive location".

@mkouba Updated with suggestions (and squashed commits). The detection is now way more fine grained and from from group/artifact ID to application classes with a fallback to paths and uses uknown archive as a last resort.

manovotn · 2021-07-14T09:25:39Z

@mkouba @geoand So I tested this in several quickstarts - I executed each of them few times over to account for deviations.
Times to it took to execute SplitPackageProcessor were ranging from 3 ms to 11 ms with an average of 6 ms so nothing too serious if you ask me.

Fun fact - I discovered that we have a split package issue in panache ;-)

2021-07-14 10:48:36,851 WARN  [io.qua.arc.dep.SplitPackageProcessor] (build-9) Detected a split package usage which is considered a bad practice and should be avoided. Following packages were detected in multiple archives: 
- "io.quarkus.hibernate.orm.panache" found in [io.quarkus:quarkus-hibernate-orm-panache-common, io.quarkus:quarkus-hibernate-orm-panache]

@yrodiere or @FroMage might be interested in that^

geoand · 2021-07-14T09:28:13Z

@mkouba @geoand So I tested this in several quickstarts - I executed each of them few times over to account for deviations.
Times to it took to execute SplitPackageProcessor were ranging from 3 ms to 11 ms with an average of 6 ms so nothing too serious if you ask me.

Great, thanks for checking @manovotn!

mkouba

Tens of milliseconds seem to be acceptable ;-)

manovotn requested review from mkouba and geoand July 12, 2021 14:52

quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jul 12, 2021

manovotn changed the title ~~Introduce a processor that checks for split package issues.~~ Introduce a processor that checks for split package issues Jul 12, 2021

manovotn force-pushed the issue18510 branch from c5e5873 to 320e475 Compare July 12, 2021 14:58

manovotn force-pushed the issue18510 branch from 320e475 to e3eac3d Compare July 12, 2021 14:59

geoand approved these changes Jul 12, 2021

View reviewed changes

mkouba reviewed Jul 13, 2021

View reviewed changes

Introduce a processor that checks for split package issues.

5492651

manovotn force-pushed the issue18510 branch from f1f8ef7 to 5492651 Compare July 14, 2021 09:39

mkouba approved these changes Jul 14, 2021

View reviewed changes

manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 14, 2021

manovotn merged commit fca7799 into quarkusio:main Jul 14, 2021

quarkus-bot bot added this to the 2.1 - main milestone Jul 14, 2021

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 14, 2021

gastaldi mentioned this pull request Jul 15, 2021

Split package warning from core extensions #18732

Closed

mkouba mentioned this pull request Jul 20, 2021

field injection of package-private fields for subclasses in dependent modules fails with IllegalAccessError #7284

Closed

manovotn mentioned this pull request Dec 7, 2021

IllegalAccessError while accessing protected field with maven multi-module #21996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a processor that checks for split package issues #18617

Introduce a processor that checks for split package issues #18617

manovotn commented Jul 12, 2021

quarkus-bot bot commented Jul 12, 2021

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `c5e5873`

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `320e475`

geoand left a comment

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `e3eac3d`

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/reactive-pg-client/deployment

mkouba commented Jul 13, 2021

geoand commented Jul 13, 2021

mkouba Jul 13, 2021

manovotn Jul 13, 2021 •

edited

mkouba Jul 13, 2021

mkouba commented Jul 13, 2021

geoand commented Jul 13, 2021

manovotn commented Jul 13, 2021

manovotn commented Jul 13, 2021

mkouba Jul 13, 2021

geoand Jul 13, 2021

manovotn Jul 13, 2021

mkouba Jul 13, 2021

manovotn Jul 14, 2021

manovotn commented Jul 14, 2021

geoand commented Jul 14, 2021

mkouba left a comment

Introduce a processor that checks for split package issues #18617

Introduce a processor that checks for split package issues #18617

Conversation

manovotn commented Jul 12, 2021

quarkus-bot bot commented Jul 12, 2021

quarkus-bot bot commented Jul 12, 2021 • edited

Failing Jobs - Building c5e5873

quarkus-bot bot commented Jul 12, 2021 • edited

Failing Jobs - Building 320e475

geoand left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jul 12, 2021 • edited

Failing Jobs - Building e3eac3d

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/reactive-pg-client/deployment

mkouba commented Jul 13, 2021

geoand commented Jul 13, 2021

mkouba Jul 13, 2021

Choose a reason for hiding this comment

manovotn Jul 13, 2021 • edited

Choose a reason for hiding this comment

mkouba Jul 13, 2021

Choose a reason for hiding this comment

mkouba commented Jul 13, 2021

geoand commented Jul 13, 2021

manovotn commented Jul 13, 2021

manovotn commented Jul 13, 2021

mkouba Jul 13, 2021

Choose a reason for hiding this comment

geoand Jul 13, 2021

Choose a reason for hiding this comment

manovotn Jul 13, 2021

Choose a reason for hiding this comment

mkouba Jul 13, 2021

Choose a reason for hiding this comment

manovotn Jul 14, 2021

Choose a reason for hiding this comment

manovotn commented Jul 14, 2021

geoand commented Jul 14, 2021

mkouba left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `c5e5873`

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `320e475`

quarkus-bot bot commented Jul 12, 2021 •

edited

Failing Jobs - Building `e3eac3d`

manovotn Jul 13, 2021 •

edited