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.

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

Automatically execute plugin goals configured in phases preceding quarkus:dev #33617

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

aloubyansky
Copy link
Member

@aloubyansky aloubyansky commented May 25, 2023

Fix #32816
Fix #30166

This change enhances DevMojo autocompile feature by figuring out the necessary phases and plugin goals that need to be handled before launching the app in dev mode.

@famod would you mind reviewing this one? Thanks.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven labels May 25, 2023
@quarkus-bot

This comment has been minimized.

@geoand geoand requested a review from famod May 29, 2023 12:21
Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

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

Nice to see this long standing "gap" fixed!

I didn't have the time to test it, but I might have found two issues while running the algorithm through my brain. 😉

if (goal.equals(QUARKUS_GENERATE_CODE_GOAL)) {
var clone = e.clone();
clone.setGoals(List.of(QUARKUS_GENERATE_CODE_GOAL));
// this will schedule it before the compile phase goals
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that it will always be added before any compile phase goals?

What if someone configures default-compile plugin execution (of m-compiler-p) first, without a phase, and then quarkus-m-p?

Better use add(0, Map.entry(p, clone))?

} else {
executeIfConfigured(IO_SMALLRYE, JANDEX_MAVEN_PLUGIN, JANDEX, Map.of());
for (int phaseIndex = latestHandledPhaseIndex + 1; phaseIndex < PRE_DEV_MODE_PHASES.size(); ++phaseIndex) {
var executions = phaseExecutions.get(PRE_DEV_MODE_PHASES.get(phaseIndex));
Copy link
Member

@famod famod May 31, 2023

Choose a reason for hiding this comment

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

If I'm not mistaken, when no phase has been specified (e.g. just mvn quarkus:dev), then phaseIndex would start at 1 (because latestHandledPhaseIndex stayed at 0), hence skipping validate and jumping right to initialize, no?

@aloubyansky aloubyansky force-pushed the dev-mode-execute-phases branch from 75ff382 to c763507 Compare June 1, 2023 14:53
@aloubyansky
Copy link
Member Author

Thanks @famod both were spot on, fixed.

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

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

Nice!

I was thinking about a way to exclude some executions from being auto-run like that, but let's see if someone complains.

@quarkus-bot

This comment has been minimized.

@aloubyansky aloubyansky force-pushed the dev-mode-execute-phases branch from c763507 to 0e27aaf Compare June 2, 2023 09:16
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 2, 2023

Failing Jobs - Building 0e27aaf

Status Name Step Failures Logs Raw logs
Native Tests - Amazon Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Amazon #

- Failing: integration-tests/amazon-lambda integration-tests/amazon-lambda-http 

📦 integration-tests/amazon-lambda

io.quarkus.it.amazon.lambda.AmazonLambdaSimpleIT.testSimpleLambdaSuccess - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.amazon.lambda.deployment.DevServicesLambdaProcessor#startEventServer threw an exception: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.net.BindException: Address already in use

📦 integration-tests/amazon-lambda-http

io.quarkus.it.amazon.lambda.AmazonLambdaSimpleIT.testJaxrsCognitoJWTSecurityContext - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.amazon.lambda.deployment.DevServicesLambdaProcessor#startEventServer threw an exception: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.net.BindException: Address already in use

@aloubyansky aloubyansky merged commit 10e0d21 into quarkusio:main Jun 2, 2023
@quarkus-bot quarkus-bot bot added kind/bugfix kind/enhancement New feature or request labels Jun 2, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 2, 2023
@melloware
Copy link
Contributor

Nice work @aloubyansky looking forward to it!

@rsvoboda
Copy link
Member

rsvoboda commented Jun 6, 2023

Hi @aloubyansky.

Quarkus QE framework daily runs started to fail on daily runs after this PR. E.g. https://github.com/quarkus-qe/quarkus-test-framework/actions/runs/5182751224/jobs/9340270864

The failure can be seen using mvn clean verify -pl examples/greetings -Dquarkus.platform.version=999-SNAPSHOT -Dit.test=RemoteDevGreetingResourceIT command. You need to install FW into local cahe before executing (e.g. mvn clean install -DskipTests -DskipITs)

The main cause is:

23:57:49,244 INFO  [app] Caused by: org.apache.maven.plugins.checkstyle.exec.CheckstyleExecutorException: Unable to find configuration file at location: checkstyle.xml

Checkstyle xml file is in the root of the https://github.com/quarkus-qe/quarkus-test-framework repository and examples/greetings depends transitively on the root pom.xml where the checkstyle is defined.

I can workaround this by adding <checkstyle.skip>true</checkstyle.skip> into examples/greetings/pom.xml, but I rather highlight this to you. There could be others affected by this change, especially with bigger projects.

@aloubyansky
Copy link
Member Author

Thanks @rsvoboda, I'll have a look.

@aloubyansky
Copy link
Member Author

Interestingly it does not fail for me locally.

@rsvoboda
Copy link
Member

rsvoboda commented Jun 7, 2023

@aloubyansky you need to revert quarkus-qe/quarkus-test-framework@08f9220 to see that

@aloubyansky
Copy link
Member Author

I suspect it's because of the app module being moved to a new dir out of the project (although still in the dir hierarchy but out of the Maven project module hierarchy), so it ends being an orphaned project referencing the parent POM that is resolved from the local Maven repo, which configures the checkstyle plugin that is looking for the config file that is now not found in the new (orhaned) workspace.

@aloubyansky
Copy link
Member Author

Basically, the project ends up in quarkus-test-framework/examples/greetings/target/RemoteDevGreetingResourceIT/app if you cd in there and simply try mvn compile it will fail in the same way, which means the project isn't actually buildable.

@rsvoboda
Copy link
Member

rsvoboda commented Jun 7, 2023

Hi @aloubyansky, thanks for looking into this! You are correct, we see the trouble because we use just part of the repo structure in orphaned location.

@aloubyansky
Copy link
Member Author

aloubyansky commented Jun 7, 2023

As a workaround, you could probably adjust the parent location, i.e. the <relativePath> value to point to the proper location of the parent in the original workspace.

I didn't initially realize that we are copying just the module where the test is.

@aloubyansky
Copy link
Member Author

Here is what I meant, as an idea, in case you are curious

diff --git a/quarkus-test-core/src/main/java/io/quarkus/test/services/quarkus/RemoteDevModeQuarkusApplicationManagedResourceBuilder.java b/quarkus-test-core/src/main/java/io/quarkus/test/services/quarkus/RemoteDevModeQuarkusApplicationManagedResourceBuilder.java
index 96fa0a02..ec4606cf 100644
--- a/quarkus-test-core/src/main/java/io/quarkus/test/services/quarkus/RemoteDevModeQuarkusApplicationManagedResourceBuilder.java
+++ b/quarkus-test-core/src/main/java/io/quarkus/test/services/quarkus/RemoteDevModeQuarkusApplicationManagedResourceBuilder.java
@@ -16,6 +16,7 @@ import java.util.List;
 import java.util.Optional;
 import java.util.ServiceLoader;
 
+import io.quarkus.bootstrap.resolver.maven.workspace.ModelUtils;
 import io.quarkus.test.bootstrap.ManagedResource;
 import io.quarkus.test.bootstrap.Protocol;
 import io.quarkus.test.bootstrap.ServiceContext;
@@ -65,9 +66,19 @@ public class RemoteDevModeQuarkusApplicationManagedResourceBuilder extends Artif
     @Override
     protected void build() {
         try {
+
             FileUtils.copyCurrentDirectoryTo(getContext().getServiceFolder());
             copyResourcesToAppFolder();
 
+            var currentDir = Path.of("").normalize().toAbsolutePath();
+            var relativeParent = ModelUtils.readModel(currentDir.resolve("pom.xml")).getParent().getRelativePath();
+            var parentPom = currentDir.resolve(relativeParent).normalize().toAbsolutePath();
+            var targetAppDir = getContext().getServiceFolder().normalize().toAbsolutePath();
+            var targetAppPom = targetAppDir.resolve("pom.xml");
+            var targetAppModel = ModelUtils.readModel(targetAppPom);
+            targetAppModel.getParent().setRelativePath(targetAppDir.relativize(parentPom).toString());
+            ModelUtils.persistModel(targetAppPom, targetAppModel);

@rsvoboda
Copy link
Member

rsvoboda commented Jun 7, 2023

For now I just skip checkstyle execution when invoking remote dev command. Will make a followup note about relativePath.

@rsvoboda
Copy link
Member

rsvoboda commented Jun 7, 2023

Ah, you are quick! We would need something like for all the classes where we copy the module.

@aloubyansky
Copy link
Member Author

It'd be still more like a hack though. Because the parent POM is still referencing the original module in its <modules>. It'd be better to look for a cleaner alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven kind/bugfix kind/enhancement New feature or request
Projects
None yet
4 participants