-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 the RR dependency on the Kotlin support module a conditional dependency #18230
Make the RR dependency on the Kotlin support module a conditional dependency #18230
Conversation
Currently the complete build tree for a project including RR would look like this (includes the kotlin support module):
|
With this change it would look like this (no kotlin-related dependencies):
|
But adding
|
@@ -80,6 +80,7 @@ protected MavenArtifactResolver resolver() throws BootstrapMavenException { | |||
.setRemoteRepositoryManager(bootstrapProvider.remoteRepositoryManager()) | |||
//.setRepositorySystemSession(repoSession) the session should be initialized with the loaded workspace | |||
.setRemoteRepositories(repos) | |||
.setPreferPomsFromWorkspace(true) |
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.
This one is not really related to the dependency refactoring. I opened #18229 for this change.
<configuration> | ||
<dependencyCondition> | ||
<artifact>io.quarkus:quarkus-kotlin</artifact> | ||
</dependencyCondition> |
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.
This configures a condition for the extension to be enabled. It can actually be any artifact, not necessarily an extension, or a set of artifacts.
11a1293
to
be4b500
Compare
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.
I think this makes a lot of sense!
But probably https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/KotlinCoroutineIntegrationProcessor.java#L95 will need to be updated as well? Likely just to mention that quarkus-kotlin
should be included
Should it move to the |
Yeah, now that this PR introduces a dedicated deployment, that makes much more sense |
Ok, i'll look into moving it. |
The failure seems unrelated |
@aloubyansky if you want me to deal with the move, that is certainly fine with me :) |
be4b500
to
441a4c4
Compare
I think it'll be better if you do that @geoand It'll require some refactoring and moving things around. See my second hacky commit. |
No problem, I'll look into later on today or tomorrow |
…tive-kotlin-deployment
441a4c4
to
eec469b
Compare
@aloubyansky I force pushed into your branch. I basically changed the second commit and I think it's better now. Mind taking a look? |
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 441a4c4
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 16 #📦 extensions/vertx-http/deployment✖ |
This workflow status is outdated as a new workflow run has been triggered. |
Looks ok to me but the RR experts should still review this of course. |
Definitely, hopefully @stuartwdouglas can take a look |
@stuartwdouglas is this approach fine with you? |
Thanks folks! |
This PR presents an alternative approach to the current direct dependency from RR on the
quarkus-resteasy-reactive-kotlin
module.It turns
quarkus-resteasy-reactive-kotlin
into an extension (with an empty deployment module) with a condition for it to be enabled in an app (by the Quarkus bootstrap mechanism) only in case thequarkus-kotlin
extension is present on the classpath.fyi @evanchooly @geoand