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.

Sign up for GitHub

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

Jump to bottom

Make SplitPackageProcessor configurable #19089

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Make SplitPackageProcessor configurable #19089

merged 1 commit into from
Jul 30, 2021

Conversation

manovotn
Copy link
Contributor

Allow to skip split package detection for user-defined packages.
Fixes #19031

I am OFC open to suggestions regarding config item name or placement in another config (although it IMO does belong under Arc at the moment).

Also, I don't think we can have an automated test for this, so I at least tested it with one of the reproducer apps that was provided in the original issue that spawned creation of this unfortunate processor :-)

@manovotn manovotn requested review from mkouba and famod July 29, 2021 11:34
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jul 29, 2021
@manovotn manovotn mentioned this pull request Jul 29, 2021
Closed
@geoand
Copy link
Contributor

geoand commented Jul 29, 2021 •
edited
Loading

Can this PR also add a build item that extensions can use to suppress warnings that come from runtime dependencies that users should not care about?

I could use it in #19092

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 29, 2021 •
edited
Loading

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

Failing Jobs - Building aaccdce

Status Name Step Test failures Logs Raw logs
✖ Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

famod
famod approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@famod famod left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully with the small reproducer from here: #19030 (comment) 👍

extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java Outdated Show resolved Hide resolved
@geoand
Copy link
Contributor

geoand commented Jul 30, 2021

@manovotn are you planning on adding the build item in this PR or should we merge it so I can then add it and tackle #19092?

@manovotn
Copy link
Contributor Author

manovotn commented Jul 30, 2021 •
edited
Loading

@geoand will add, on it shortly

@geoand
Copy link
Contributor

geoand commented Jul 30, 2021

Thanks!

@manovotn manovotn force-pushed the issue19031 branch 2 times, most recently from b3362f3 to 12d69bd Compare July 30, 2021 08:24
@manovotn
Copy link
Contributor Author

@geoand @gsmet @famod @mkouba this PR now provides build item to allow for exclusions by extensions. It also allows usage of .* expression in package name which will match the package based on String#startsWith. Obviously, this can be used by both, extensions and user config. Any feedback welcome :)

@geoand
Copy link
Contributor

geoand commented Jul 30, 2021

I think we should get this in as it's a very good improvement. We can always tweak it further if necessary

@manovotn manovotn force-pushed the issue19031 branch 2 times, most recently from 08b9aa9 to 5ff4745 Compare July 30, 2021 08:47
@manovotn
Copy link
Contributor Author

@geoand feel free to merge once CI passes so that you can continue on #19092
I hope I didn't block you long :)

@manovotn manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 30, 2021
@geoand
Copy link
Contributor

geoand commented Jul 30, 2021

Not at all, that a lot @manovotn !

I'll actually just a create a PR based on this one. That way (assuming it's rebased), I don't have to wait for CI to complete

mkouba
mkouba approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@geoand geoand mentioned this pull request Jul 30, 2021
Merged
gsmet
gsmet previously requested changes Jul 30, 2021
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other PR, it's an easy fix and the current behavior is incorrect.

extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/SplitPackageProcessor.java Outdated Show resolved Hide resolved
@manovotn manovotn requested a review from gsmet July 30, 2021 10:45
@manovotn
Copy link
Contributor Author

@gsmet updated, please re-review

@geoand geoand dismissed gsmet’s stale review July 30, 2021 11:48

suggestion was applied

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 30, 2021

Failing Jobs - Building 76fe53d

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@geoand geoand merged commit 25506d6 into quarkusio:main Jul 30, 2021
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Jul 30, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 30, 2021
@manovotn manovotn deleted the issue19031 branch July 30, 2021 14:18
@gsmet gsmet modified the milestones: 2.2 - main, 2.1.1.Final Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@famod famod famod approved these changes

@geoand geoand geoand approved these changes

@mkouba mkouba mkouba approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Milestone
2.1.1.Final
Development

Successfully merging this pull request may close these issues.

Make SplitPackageProcessor configurable
5 participants
@manovotn @geoand @mkouba @gsmet @famod