init custom gradle tooling model #10459

glefloch · 2020-07-03T15:56:30Z

This branch aims to expose a custom model containing project structure. This model is generating using the gradle API.

This allows gradle users to:

run/debug tests from IDE
run/debug main Quarkus.run (simple / multi-module)

This branch closes:

Add support for running Unit Tests in Gradle project with IntelliJ builder/runner #8737, QuarkusTest does not work in Gradle projects when invoked from IDE #9738, Quarkus should be able to run tests in different folders than the "preset" ones #8950 related to run tests
Quarkus cant be launched with Main-Class with gradle #10602, Launching Quarkus Kotlin Gradle App from IDE fails #10123 related to run main

@aloubyansky can you have a look? this should not evolve more (excepts with review comment and some test additions)

aloubyansky · 2020-07-06T06:33:12Z

devtools/gradle/src/main/java/io/quarkus/gradle/AppModelGradleResolver.java

-                        && appArtifact.getGroupId().equals(a.getModuleVersion().getId().getGroup())) {
-                    appArtifact.setPath(a.getFile().toPath());
+            for (io.quarkus.gradle.workspace.model.Dependency dependency : model.getAppDependencies()) {
+                // TODO there used to be a check on appArtifact.getType()


Is it not available in the model?

aloubyansky · 2020-07-06T06:35:54Z

devtools/gradle/src/main/java/io/quarkus/gradle/AppModelGradleResolver.java

-                        && appArtifact.getType().equals(a.getType())
-                        && appArtifact.getGroupId().equals(a.getModuleVersion().getId().getGroup())) {
-                    appArtifact.setPath(a.getFile().toPath());
+            for (io.quarkus.gradle.workspace.model.Dependency dependency : model.getAppDependencies()) {


This is actually very limiting, since the scope is limited to the app model only. I'll need to check whether the devtools could be affected.

Have you tried running the full testsuite with these changes? Or more specifically all the gradle tests including the tooling ones, such add/list extensions, create project? I'm wondering whether we should be changing this method at all. What we need to providing at the end is the AppModel and the workspace structure.

I run some tests on my machine on a sample project and everything was ok. Regarding the testsuite, I still have some failures. I will revert that change as you suggest.

aloubyansky · 2020-07-06T06:42:28Z

devtools/gradle/src/main/java/io/quarkus/gradle/AppModelGradleResolver.java

+        if (!appArtifact.isResolved()) {
+            PathsCollection.Builder paths = PathsCollection.builder();
+            for (SourceSet sourceSet : model.getSourceSets()) {
+                sourceSet.getSourceDirectories().stream().filter(File::exists).map(File::toPath).forEach(paths::add);


Are these java sources?

these are more BuildOutput such as build/classes/java/main

well actually it's not, but I will update so it points to classes and not java files

aloubyansky · 2020-07-06T06:46:35Z

devtools/gradle/src/main/java/io/quarkus/gradle/tasks/QuarkusDev.java

@@ -284,7 +284,8 @@ public void startDev() {
            StringBuilder classPathManifest = new StringBuilder();

            final AppModel appModel;
-            final AppModelResolver modelResolver = extension().getAppModelResolver(LaunchMode.DEVELOPMENT);
+            final QuarkusModel quarkusModel = extension().getQuarkusModel(LaunchMode.DEVELOPMENT);
+            final AppModelResolver modelResolver = new AppModelGradleResolver(quarkusModel);


Looks like this could stay the way it was?

aloubyansky · 2020-07-06T06:55:16Z

devtools/gradle/src/main/java/io/quarkus/gradle/workspace/builder/QuarkusModelBuilder.java

+        }
+
+        final SourceSet mainSourceSet = QuarkusGradleUtils.getSourceSet(project, SourceSet.MAIN_SOURCE_SET_NAME);
+        final SourceSet testSourceSet = QuarkusGradleUtils.getSourceSet(project, SourceSet.TEST_SOURCE_SET_NAME);


Looks like this could depend on the launch mode.

aloubyansky · 2020-07-06T07:00:09Z

...ndent-projects/bootstrap/app-model/src/main/java/io/quarkus/bootstrap/model/ArtifactKey.java

@@ -0,0 +1,13 @@
+package io.quarkus.bootstrap.model;
+
+public interface ArtifactKey {


Is this used anywhere else then in AppArtifactKey?

aloubyansky · 2020-07-06T07:01:27Z

devtools/gradle/src/main/java/io/quarkus/gradle/workspace/model/QuarkusModel.java

+
+    Collection<Dependency> getAppDependencies();
+
+    Collection<String> getExtensions();


What is this for? It looks like it also includes all the enforced platforms.

aloubyansky · 2020-07-06T07:07:50Z

devtools/gradle/src/main/java/io/quarkus/gradle/workspace/builder/QuarkusModelBuilder.java

+        final SourceSet mainSourceSet = QuarkusGradleUtils.getSourceSet(project, SourceSet.MAIN_SOURCE_SET_NAME);
+        final SourceSet testSourceSet = QuarkusGradleUtils.getSourceSet(project, SourceSet.TEST_SOURCE_SET_NAME);
+
+        final Collection<org.gradle.api.artifacts.Dependency> platformExtension = getPlatformExtension(project);


I am not sure what platformExtension signifies here. These are application direct extension dependencies, maybe let's call it that?

Yes, I agree with you

aloubyansky · 2020-07-06T07:08:30Z

devtools/gradle/src/main/java/io/quarkus/gradle/workspace/builder/QuarkusModelBuilder.java

+            platformExtension.addAll(getExtensions(configuration.getFirstLevelModuleDependencies(), appDependencies));
+        }
+
+        final Collection<Dependency> platformDependencies = collectExtensionDependencies(project, platformExtension);


Let's call them either extension dependencies or deployment dependencies.

glefloch · 2020-07-06T08:29:03Z

@aloubyansky thanks for the review, I will update my branch :)

aloubyansky · 2020-07-09T20:56:08Z

independent-projects/bootstrap/app-model/pom.xml

+        <dependency>
+            <groupId>org.gradle</groupId>
+            <artifactId>gradle-core-api</artifactId>
+            <version>6.1.1</version>


I didn't realize we'd introduce Gradle deps in the app-model. This is not a good thing. For Maven we have quarkus-bootstrap-maven-resolver that depends on app-model and adds the Maven deps. I think we should do the same for Gradle.

This should be getting now into the launcher JAR. Have you noticed how much bigger it becomes with the Gradle part?

Actually, we don't need this dependency anymore, we only need the gradle-tooling-api. But, yes I think externalizing it to a module could be a good idea.

aloubyansky · 2020-07-21T12:23:26Z

devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java

+                project.getBuildDir().getAbsoluteFile(), getSourceSourceSet(mainSourceSet), convert(mainSourceSet));
+    }
+
+    private Set<Workspace> getAdditionalWorkspaces(Project project, String mainProjectName) {


There is only one workspace that may include multiple projects.

Yes, I will introduce a project or module type to only keep one worksapce

aloubyansky · 2020-07-21T12:27:26Z

devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java

+        return workspaces;
+    }
+
+    private Set<org.gradle.api.artifacts.Dependency> getDirectExtension(Project project) {


This method should be renamed. It's not about extensions here. All it does it collects the enforced platforms. It should reflect that in the name.

👍 , I will rename it

aloubyansky · 2020-07-22T13:38:09Z

devtools/gradle/src/main/java/io/quarkus/gradle/tasks/QuarkusPrepare.java

                Optional<Method> initAndRun = Arrays.stream(codeGenerator.getMethods())
                        .filter(m -> m.getName().equals(INIT_AND_RUN))
                        .findAny();
                if (!initAndRun.isPresent()) {
                    throw new GradleException("Failed to find " + INIT_AND_RUN + " method in " + CodeGenerator.class.getName());
                }
-
+                sourcesDirectories.forEach(System.out::println);


Is this a left-over?

aloubyansky · 2020-07-22T13:42:00Z

...s/bootstrap/gradle-resolver/src/main/java/io/quarkus/bootstrap/resolver/model/Workspace.java

+
+    WorkspaceModule getMainModule();
+
+    Set<WorkspaceModule> getAllModule();


This should be plural, i.e. getAllModules

glefloch · 2020-07-24T09:50:38Z

@aloubyansky, I fixed the gradle windows issue. It was a problem with the Process running the wrapper command.

CI should be ok now :)

aloubyansky · 2020-07-24T11:08:54Z

bom/application/pom.xml

@@ -201,6 +201,7 @@
        <webjars-locator-core.version>0.46</webjars-locator-core.version>
        <log4j2-jboss-logmanager.version>1.0.0.Beta1</log4j2-jboss-logmanager.version>
        <avro.version>1.10.0</avro.version>
+        <gradle-tooling.version>6.5</gradle-tooling.version>


Looks like this property could be removed from this pom. But let's see if there are other things to change in this PR first.

aloubyansky · 2020-07-24T11:21:33Z

devtools/gradle/src/main/java/io/quarkus/gradle/AppModelGradleResolver.java

-                    }
-                    appArtifact.setPaths(paths.build());
-                }
+                throw new AppModelResolverException("Requested artifact does not match loaded model");


It'd be good to include the conflicting artifacts into the message.

aloubyansky · 2020-07-24T11:33:03Z

devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java

+            }
+
+            final Set<ResolvedDependency> resolvedChildren = d.getChildren();
+            if (!resolvedChildren.isEmpty()) {


I had addChildExtension and walked the whole tree because we also used to collect all the extension properties. Given that you are doing it elsewhere, I think you could have changed this if to addChildExtension && !resolvedChildren.isEmpty(). I don't think you need firstLevel arg in your impl.

I.e. you could have removed addChildExtension too. We are simply collecting the first met extension dependencies but not its children.

aloubyansky · 2020-07-24T11:38:20Z

devtools/gradle/src/test/resources/multi-module-project/settings.gradle

+        gradlePluginPortal()
+    }
+    plugins {
+        id 'io.quarkus' version '999-SNAPSHOT'


We can't be using specific quarkus versions in the test configs. This will break the release process.

aloubyansky · 2020-07-24T11:38:36Z

devtools/gradle/src/test/resources/simple-module-project/settings.gradle

+        gradlePluginPortal()
+    }
+    plugins {
+        id 'io.quarkus' version '999-SNAPSHOT'


Here as well.

aloubyansky · 2020-07-24T11:42:41Z

...endent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/utils/BuildToolHelper.java

+            if (Files.exists(currentPath.resolve(BuildTool.MAVEN.getBuildFile()))) {
+                return true;
+            }
+            currentPath = currentPath.getParent();


Did you copy this from somewhere? This is not a good idea. What kind of path is expected to be passed in? Project root or any path inside a project?
A little bit better version would be checking each directory for all the recognizable build files instead of walking up to the root of the FS for each build file.

yes I copier that from here:

quarkus/independent-projects/bootstrap/maven-resolver/src/main/java/io/quarkus/bootstrap/resolver/maven/workspace/LocalProject.java

Line 217 in 42dbdd0

private static Path locateCurrentProjectPom(Path path, boolean required) throws BootstrapMavenException {

Ok, that path is derived from a class name (that is in the classes dir of a project) or is a project root.

aloubyansky · 2020-07-24T11:45:06Z

...endent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/utils/BuildToolHelper.java

+
+    public static QuarkusModel enableGradleAppModel(Path projectRoot, String mode)
+            throws IOException, AppModelResolverException {
+        if (isMavenProject(projectRoot)) {


If projectRoot is actually the project root we shouldn't be walking up to the root of the FS looking for the pom.xml.

aloubyansky · 2020-07-24T11:47:47Z

...ap/gradle-resolver/src/main/java/io/quarkus/bootstrap/resolver/model/impl/WorkspaceImpl.java

+public class WorkspaceImpl implements Workspace, Serializable {
+
+    public ArtifactCoords mainModuleKey;
+    public Set<WorkspaceModule> modules;


WorkspaceModuleImpl does not implement hashCode/equals. It would probably be better to have a map instead here. Also given that there is getModule(key)

aloubyansky · 2020-07-24T11:53:19Z

...ts/bootstrap/gradle-resolver/src/main/java/io/quarkus/bootstrap/util/QuarkusModelHelper.java

+        Map<AppArtifactKey, AppDependency> versionMap = new HashMap<>();
+        model.getAppDependencies().stream().map(QuarkusModelHelper::toAppDependency).forEach(appDependency -> {
+            userDeps.add(appDependency);
+            versionMap.put(appArtifact.getKey(), appDependency);


you need appDependency.getArtifact().getKey() or something here.

aloubyansky · 2020-07-24T11:54:08Z

...ts/bootstrap/gradle-resolver/src/main/java/io/quarkus/bootstrap/util/QuarkusModelHelper.java

+                    }
+                }
+            }
+            if (!userDeps.contains(appDep)) {


This is not correct. You need to be comparing the keys instead. Otherwise, you are not aligning versions here.

This is actually done like this in the current model, should i update it to use the appDep.getArtifact() instead? (

quarkus/devtools/gradle/src/main/java/io/quarkus/gradle/AppModelGradleResolver.java

Line 189 in 23b3d64

if (!userDeps.contains(dependency)) {

)

Oh, that's clearly a bug. It should be appDep.getArtifact().getKey()

aloubyansky · 2020-07-24T11:59:24Z

...ation-tests/gradle/src/test/resources/test-resources-in-build-steps/application/build.gradle

@@ -4,6 +4,7 @@ plugins {

 dependencies {
    implementation project(":runtime")
+    implementation project(":deployment")


No, applications can't depend on the deployment artifact.

aloubyansky · 2020-07-24T12:07:25Z

test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java

@@ -168,6 +170,12 @@ private ExtensionState doJavaStart(ExtensionContext context, Class<? extends Qua
                rootBuilder.add(appResourcesLocation);
            }

+            Path root = Paths.get("").normalize().toAbsolutePath();
+            // If gradle project running directly with IDE
+            if (System.getProperty(BootstrapConstants.SERIALIZED_APP_MODEL) == null) {


Where is this property set? I see it in exportModel but has it been called by this point?

It could already have been set by the gradle plugin in the before test

We'll need a delegating impl of AppModelResolver. For now it's ok, I think.

aloubyansky

Looks good. We'll need to refactor some bootstrap API and setup later to have a cleaner Maven/Gradle bootstrap integration.

boring-cyborg bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/testing labels Jul 3, 2020

aloubyansky reviewed Jul 6, 2020

View reviewed changes

glefloch force-pushed the fix/gradle-model branch from 96dc2f2 to b4139b6 Compare July 6, 2020 15:30

aloubyansky reviewed Jul 9, 2020

View reviewed changes

boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file labels Jul 13, 2020

glefloch force-pushed the fix/gradle-model branch from 6048110 to 8258472 Compare July 15, 2020 18:58

glefloch changed the title ~~WIP - init custom gradle tooling model~~ init custom gradle tooling model Jul 15, 2020

glefloch marked this pull request as ready for review July 15, 2020 19:04

glefloch force-pushed the fix/gradle-model branch 8 times, most recently from 4503657 to 16632d2 Compare July 21, 2020 06:25

aloubyansky reviewed Jul 21, 2020

View reviewed changes

aloubyansky reviewed Jul 22, 2020

View reviewed changes

glefloch force-pushed the fix/gradle-model branch 3 times, most recently from 03fa3c6 to a58138d Compare July 24, 2020 07:51

aloubyansky reviewed Jul 24, 2020

View reviewed changes

init custom gradle tooling model

9735fad

glefloch force-pushed the fix/gradle-model branch from a58138d to 9735fad Compare July 24, 2020 13:31

aloubyansky approved these changes Jul 24, 2020

View reviewed changes

aloubyansky merged commit 47b78d2 into quarkusio:master Jul 24, 2020

aloubyansky added this to the 1.7.0 - master milestone Jul 24, 2020

aloubyansky mentioned this pull request Jul 24, 2020

Running a project with a main class from IntelliJ when project is built using gradle fails #10961

Open

glefloch deleted the fix/gradle-model branch March 28, 2021 18:16

		@@ -0,0 +1,13 @@
		package io.quarkus.bootstrap.model;

		public interface ArtifactKey {


		Collection<Dependency> getAppDependencies();

		Collection<String> getExtensions();


		WorkspaceModule getMainModule();

		Set<WorkspaceModule> getAllModule();

init custom gradle tooling model #10459

init custom gradle tooling model #10459

Conversation

glefloch commented Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefloch commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefloch commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloubyansky left a comment

Choose a reason for hiding this comment

glefloch commented Jul 3, 2020 •

edited

Loading