Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SplitPackageProcessor configurable #19031

Closed
famod opened this issue Jul 27, 2021 · 12 comments · Fixed by #19089
Closed

Make SplitPackageProcessor configurable #19031

famod opened this issue Jul 27, 2021 · 12 comments · Fixed by #19089
Assignees
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Milestone

Comments

@famod
Copy link
Member

famod commented Jul 27, 2021

Description

With 2.1.0 we are getting:

[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: 
- "org.w3._2000._09.xmldsig_" found in [de.somecompany.someproject:register-webservice-a, de.somecompany.someproject:register-webservice-b]

because we a generating stubs from WSDL in two different Maven modules (via cxf-codegen-plugin).

I do get the point of avoiding split packages, but in this case I don't think our valuable project time is well invested in trying to separate (or unify) those generated packages. They are not subject to any injection anyway.

Therefore it would come in handy, if that processor could be configured in some way, e.g. add exclusions etc.

Implementation ideas

No response

@famod famod added the kind/enhancement New feature or request label Jul 27, 2021
@famod famod added area/arc Issue related to ARC (dependency injection) and removed triage/needs-triage labels Jul 27, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 27, 2021

/cc @manovotn, @mkouba

@mkouba
Copy link
Contributor

mkouba commented Jul 28, 2021

We could easily implement this but I'm not quite it's worth the effort, i.e. to introduce a config property just to suppress some warnings. In theory, you should be able to set the log level to ERROR for the io.quarkus.arc.deployment.SplitPackageProcessor logger to suppress all warnings...

@famod
Copy link
Member Author

famod commented Jul 28, 2021

Sure, I can suppress all warnings but this way I might miss warnings that are actually more relevant for me/my project.

@famod
Copy link
Member Author

famod commented Jul 28, 2021

Sure, I can suppress all warnings

Well, spoke too soon: I failed at silencing it via logging config. Even tried adding:

<argLine>-Dquarkus.log.category."io.quarkus.arc.deployment.SplitPackageProcessor".level=ERROR</argLine>

to surefire to get rid of the warnings in #19030 but nope...

@mkouba
Copy link
Contributor

mkouba commented Jul 28, 2021

Sure, I can suppress all warnings but this way I might miss warnings that are actually more relevant for me/my project.

No, I mean warnings from the logger of name io.quarkus.arc.deployment.SplitPackageProcessor.

to surefire to get rid of the warnings in #19030 but nope...

For tests you need to follow the steps described here: https://quarkus.io/guides/logging#how-to-configure-logging-for-quarkustest

@famod
Copy link
Member Author

famod commented Jul 28, 2021

Sure, I can suppress all warnings but this way I might miss warnings that are actually more relevant for me/my project.

No, I mean warnings from the logger of name io.quarkus.arc.deployment.SplitPackageProcessor.

Sorry for being imprecise: I meant I would not see any warnings from that processor, even the ones that might be more relevant for me.

to surefire to get rid of the warnings in #19030 but nope...

For tests you need to follow the steps described here: https://quarkus.io/guides/logging#how-to-configure-logging-for-quarkustest

Thanks for the pointer but LogManager is already there since day one.
Besides, the warning from my initial comment/description is issued by quarkus-maven-plugin when building the app, so no tests involved there.

@manovotn
Copy link
Contributor

So basically a configurable whitelist for the processor.
I am not sure it is entirely sensible and would prefer for users to either ignore all warning or solve them. OTOH, I get your point here.

It is definitely implementable, the property could pass in a list of Strings which represent different packages you want the processor to skip. I'll look into it.

@manovotn manovotn self-assigned this Jul 28, 2021
@famod
Copy link
Member Author

famod commented Jul 28, 2021

@manovotn if there was an option to switch it off it would be ok for me. I would then enable it on demand.
(to not waste your time on such a minor thing)

@geoand
Copy link
Contributor

geoand commented Jul 29, 2021

Something tells me that @manovotn got more than he bargained for when he decided to help out on the split package issue 😆

@manovotn
Copy link
Contributor

@geoand It all started with good intention to let users know about split packages which may break Arc functionality.
Unfortunately, road to hell is also paved with good intentions :-D

@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Jul 30, 2021
@gsmet gsmet modified the milestones: 2.2 - main, 2.1.1.Final Aug 3, 2021
@dhoffer
Copy link

dhoffer commented Aug 27, 2021

@manovotn Can someone explain why split packages are a problem for Quarkus? We have some shared code in separate jars that use the same package names. That is perfectly fine for Java what issues does this cause for Arc/Quarkus?

@manovotn
Copy link
Contributor

That is perfectly fine for Java

That's not true, JDK 11 with JPMS would complain :)

Anyway, IIRC, the issue was with proxying of beans if there types were split in packages. That could end up being an illegal access error. Some of the linked issues had a reproducer that showed that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants