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 composite builds of multimodule gradle projects #29233
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
fixes #29234 |
7ffdf1c
to
82cd8a5
Compare
Thanks for this contribution @tomas1885, this looks good to me. Could you also add an integration test here : https://github.com/quarkusio/quarkus/tree/main/integration-tests/gradle |
82cd8a5
to
83e8f74
Compare
Hi @glefloch, can you give me an hint on how to run a single integration test locally without running the whole gradle plugin test suite? |
Hi @glefloch, I added 2 integration tests for this fix, one for dev mode and another for build. |
@@ -213,7 +214,9 @@ private void setUpDeploymentConfiguration() { | |||
final DependencyHandler dependencies = project.getDependencies(); | |||
final Set<Dependency> deploymentDependencies = new HashSet<>(); | |||
for (ExtensionDependency extension : extensions) { | |||
if (extension instanceof LocalExtensionDependency) { | |||
if (extension instanceof IncludedBuildExtensionDependency) { | |||
deploymentDependencies.add(((IncludedBuildExtensionDependency) extension).getDeployment()); |
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.
Could we do it the way it's done below? i.e.
deploymentDependencies.add(dependencies.create(
extension.getDeploymentModule().getGroupId() + ":"
+ extension.getDeploymentModule().getArtifactId() + ":"
+ extension.getDeploymentModule().getVersion()));
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.
It might work, as it creates a similar object (the one returned from the extension object is an ExternalModuleDependecy while the one returned from the dependencies.create is a decorated object), in any case we would need to differentiate between a locally built extension and a composite built one. I would rather consolidate these 3 type into a self describing one, but didn't want to change the current code to that extent. I also dislike the addition of the boolean parameter to the loadExtensionInfo in the ToolingUtils class, but again, didn't want to change the code to that extent.
public static Project includedBuildProject(IncludedBuildInternal includedBuild, | ||
final ProjectComponentIdentifier componentIdentifier) { | ||
return includedBuild.getTarget().getMutableModel().getRootProject().findProject( | ||
componentIdentifier.getProjectPath()); |
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.
Is there a way to get hold of a project using non-internal API?
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 was also kind of hesitant to use this, as relaying on Internal implementation details usually has consequences, but haven't found a different way. I believe there's no other way, as the current Gradle project (under build) is by definition unrelated to the included build, so getting a reference to the other Gradle project can't be done through the current Gradle project object graphs.
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1 @@ | |||
deployment-artifact=org.acme.extensions\:quarkus-example-extension\:1.0 |
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.
deployment-artifact=org.acme.extensions\:quarkus-example-extension\:1.0 | |
deployment-artifact=org.acme.extensions\:quarkus-example-extension-deployment\:1.0 |
# extension-dependencies: | ||
# - "io.quarkus:quarkus-arc::jar" | ||
# - "io.quarkus:quarkus-core::jar" | ||
#artifact: "com.neema:quarkus-logzio::jar:@version@" |
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.
The elements commented above are supposed to be filled in by the generator, aren't they? The comments should be removed either way.
This comment has been minimized.
This comment has been minimized.
@aloubyansky Any hint on how to resolve the CI issue with the missing dependency version for the composite build integration test with the local extension? The actual integration test works, but it seems to fail due to missing version in the whole gradle integration tests POM. |
Good question. @famod do you have any suggestion? |
Yeah, either rename it so that it's not picked up or remove it from the list of artifact ids to be processed: https://github.com/quarkusio/quarkus/blob/main/integration-tests/gradle/update-dependencies.sh#L40 |
@tomas1885 thanks for your dedication so far :) we'll need your commits to be squashed before we merge it, btw. |
c91f5da
to
da58dab
Compare
@aloubyansky I squashed my commits but I see this PR carries extra commits from a main branch sync I did after the first commit for this branch, do I need to rebase everything on top? Should I resync with the latest main again? What's your preferred practice? |
We want only your changes in the PR. Rebasing and squashing is usually all that's necessary. |
da58dab
to
f871021
Compare
Hi @aloubyansky, I squashed the commit a few days ago, forgot to ping you, is there anything else that should be done here? |
dependencies { | ||
implementation enforcedPlatform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
Looks like I missed these versions. The junit dependencies are included in the quarkus-bom and they are 5.9.1 now. Could you simply remove the versions?
implementation platform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
implementation ("${quarkusPlatformGroupId}:quarkus-arc:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
Same here
implementation platform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
implementation ("${quarkusPlatformGroupId}:quarkus-arc:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
And here
dependencies { | ||
implementation enforcedPlatform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
And here
implementation platform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
implementation ("${quarkusPlatformGroupId}:quarkus-arc:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
And here
implementation platform("${quarkusPlatformGroupId}:${quarkusPlatformArtifactId}:${quarkusPlatformVersion}") | ||
implementation ("${quarkusPlatformGroupId}:quarkus-arc:${quarkusPlatformVersion}") | ||
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' | ||
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1' |
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.
And here
…n, to avoid being picked up by update-dependencies.sh Change composite build test project extension name deployment-artifact property to the correct one Remove comments from composite build test project extension manifest Fix composite build test project build integration test Add composite build with extension integration tests Fix basic composite build integration test Add integration tests for gradle composite builds (dev mode and build) Support composite builds of multimodule gradle projects. Support hot reloading and source watching composite builds. Support using local extensions as a composite build. Remove explicit jupiter version
f871021
to
bd18833
Compare
Thanks a lot @tomas1885 |
Support composite builds of multimodule gradle projects.
Support hot reloading and source watching composite builds.
Support using local extensions as a composite build.