-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support for project modules that produce multiple JARs (with classifiers) #22673
Conversation
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 8578ea9
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/qute/deployment
! Skipped: extensions/mailer/deployment extensions/resteasy-classic/resteasy-qute/deployment extensions/resteasy-reactive/quarkus-resteasy-reactive-qute/deployment and 3 more 📦 extensions/qute/deployment✖
⚙️ Native Tests - Misc4 #- Failing: integration-tests/webjars-locator
📦 integration-tests/webjars-locator✖
|
So the main thing to look at is |
The Qute failure is clear how to fix but the webjars issue isn't. I am still investigating it. |
@aloubyansky I don't think the webjar locator issue is related to your PR. Rostislav added the native testing recently and it has been unstable since then. We will remove the native testing for now. |
The failure isn't even related to native apparently. We are using |
Ok, that's a pretty strong expectation from an internal API https://github.com/classgraph/classgraph/blob/latest/src/main/java/nonapi/io/github/classgraph/classloaderhandler/QuarkusClassLoaderHandler.java#L119-L132 |
I am not sure we want to rely on that logic. Basically, the resources the webjars scanner is looking for are on the classpath but it is trying to use all sorts of tricks to not load them through the ClassLoader API and this PR breaks it. I'll probably try to find a workaround. |
8578ea9
to
e1a00a8
Compare
I ended up removing the dependency on the webjars-locator-core replacing it with our own discovery of the webjars on the classpath. |
The LiquibaseFunctionalityNativeIT is reproducible locally. My change passed the CI in my working branch before I updated the PR though. I am wondering whether I rebased before updating and pulled in some other change. |
It's also reproducible on the main branch
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building e1a00a8
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/vertx-http/deployment
! Skipped: core/test-extension/deployment extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 290 more 📦 extensions/vertx-http/deployment✖
⚙️ Native Tests - Data3 #- Failing: integration-tests/liquibase
📦 integration-tests/liquibase✖
|
e1a00a8
to
0e08466
Compare
Rebased just to get another CI run. |
0e08466
to
f959510
Compare
I added a couple more fixes:
I've been running CI multiple times with these changes in my working branches w/o getting any failure. So, this should be good to merge. |
@aloubyansky Sounds good! Shall I give it another try with my project? |
I won't say no to that :) |
Looking good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a large patch but it fixes quite a lot of complex issues and after discussion with @geoand @aloubyansky @famod , we want to get it in for 2.7.
It has already been through a first round of reviews in the first PR and got through some important testing.
This is #22336 plus the fix for
ApplicationArchiveImpl
exposing paths from open JARs that are now switched to PathTree-based API as well.