Use existing Maven BootstrapSessionListener to hint Surefire about required JVM parameters

[Sanne]

As I make progress on #49920 , it's becoming clear that we'll need all JVM modes to start with some new JVM flags.
In some cases we can automate this transparently, in others it's harder - it seems likely that we'll end up with needing people to apply some minor migration steps to set necessary parameters.

Among all "modes" which I'm trying to cover - Surefire is particularly problematic.

This particular PR is an attempt to mitigate some such needs, by setting the required parameters to Surefire automatically.

N.B.

This is only effective if people use <extensions>true</extensions> in their pom.xml (Quarkus plugin config)

But at least those who do would have a better experience

I'm fairly annoyed by this limitation, but couldn't find a way to make it work universally and transparently. An alternative would be for people to add the required settings in .mvn/maven.config - but I'm inclined to pursue all such venues, so that at least those having Maven extensions enabled have some additional benefits deriving from it.

For these reasons this PR is independent from #49920 : it's not requiring it, and the other patch can't rely on it.
For those people not having extensions enabled, I'm afraid they'll have to add some JVM flags expicitly in their pom (to be defined, and documented, as I finish #49920).

Finally, since it might be detabable if we want to apply this - I'd rather debate this approach separately and in isolation from the larger work, so I'd start by merging this if there are no objections: should be harmless when it's not useful.

WDYT, @aloubyansky , @gsmet , @geoand ?

[aloubyansky]

I haven't tried this but reading How do I use properties set by other plugins in argLine?, i was wondering whether adding some generic quarkus-surefire-args property in the surefire-plugin configuration following @{} format and setting the value of quarkus-surefire-args in a quarkus-maven-plugin goal would work.

[geoand]

This is only effective if people use true in their pom.xml (Quarkus plugin config)

If we do end up going down this route, we should at least log a message that we were not able to apply the proper flags automatically.

[Sanne]

This is only effective if people use true in their pom.xml (Quarkus plugin config)

If we do end up going down this route, we should at least log a message that we were not able to apply the proper flags automatically.

Yes of course, I plan warnings and proper deprecations.

I do have alternative ways to set the flags - it's just that I like them less (they imply the agent approach with self-attach, discussed on the other PR).
So I'm inclined to at least try to patch the majority of our runners to pre-set the flag correctly, and fallback to the agent only when they aren't set as required.

Essentially this gives us the opportunity to let people migrate to better approaches by nudging them rather than having a strong requirement, and gradually nudge them to better approaches.

In practice I think I'd like to get people to add <extensions>true</extensions> in their project because it's future proof: should we need to make further changes to the JVM flags, we can make them w/o people needing to adapt again.

[Sanne]

I haven't tried this but reading How do I use properties set by other plugins in argLine?, i was wondering whether adding some generic quarkus-surefire-args property in the surefire-plugin configuration following @{} format and setting the value of quarkus-surefire-args in a quarkus-maven-plugin goal would work.

I'm not following @aloubyansky , could you elaborate? Do you mean people should be adding this to their project? My goal was to figure out ways to set what people need w/o them having to change their build scripts.

[aloubyansky]

For a newly generated project, the surefire config looks like this currently:

            <plugin>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>${surefire-plugin.version}</version>
                <configuration>
                    <argLine>--add-opens java.base/java.lang=ALL-UNNAMED</argLine>
                    <systemPropertyVariables>
                        <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager>
                        <maven.home>${maven.home}</maven.home>
                    </systemPropertyVariables>
                </configuration>
            </plugin>

What we could consider doing is introducing a project property, e.g. quarkus.surefire.argLine:

    <properties>
        <quarkus.surefire.argLine>--add-opens java.base/java.lang=ALL-UNNAMED</quarkus.surefire.argLine>
    </properties>
...
            <plugin>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>${surefire-plugin.version}</version>
                <configuration>
                    <argLine>@{quarkus.surefire.argLine}</argLine>
                    <systemPropertyVariables>
                        <java.util.logging.manager>org.jboss.logmanager.LogManager</java.util.logging.manager>
                        <maven.home>${maven.home}</maven.home>
                    </systemPropertyVariables>
                </configuration>
            </plugin>

So far, this would be equivalent to what we have. But @{} format allows us to set a new value for quarkus.surefire.argLine in a quarkus goal. For example,

$ git diff
diff --git a/devtools/maven/src/main/java/io/quarkus/maven/GenerateCodeMojo.java b/devtools/maven/src/main/java/io/quarkus/maven/GenerateCodeMojo.java
index d95e82c8902..4f8e0c7f0bc 100644
--- a/devtools/maven/src/main/java/io/quarkus/maven/GenerateCodeMojo.java
+++ b/devtools/maven/src/main/java/io/quarkus/maven/GenerateCodeMojo.java
@@ -118,6 +118,9 @@ void generateCode(PathCollection sourceParents, Consumer<Path> sourceRegistrar,
                         }
                     }
                 }
+
+                mavenProject().getProperties().setProperty("quarkus.surefire.argLine",
+                        "--add-opens java.base/java.lang=ALL-UNNAMED -Xmx1g");
             }
         }
     }

That way we have a specific Quarkus property to override, which has its pros and cons. It doesn't require enabling extensions but it does require running a quarkus:xxx goal.

We already do some optimization steps in generate-code-tests, such as resolve and persist the test ApplicationModel that would be loaded when bootstrapping tests. It's optional though.

[Sanne]

I see, thanks for the details! However I was really hoping for a solution in which we could control the Surefire arguments w/o the user needing to make changes to existing projects - it seems like that's not possible :-/

Now, having concluded that our users will need to make some changes: shouldn't we rather suggest users to enable pugin extensions? I'd much rather ask to add a single one-liner, which we can easily validate.

My hope is that it's more future proof: should we need further evolutions, including in other areas, we can make them transparently without having to ask users to make changes yet again.
Apparently we can also better verify / validate things, as we can read the whole structure, while with the double property above we need to pray people are doing it right.

This makes me think, actually, that I could already patch the extension today to check if extensions are enabled and suggest to do it when not.

[aloubyansky]

I'm not against enabling extensions. In fact, I'd say it has been recommended, given this has been the default for years now.

I wouldn't say Quarkus should fully control a surefire config though. I'd say Quarkus should provide what it needs but the user should remain in control. Generally, so far, this has been our approach integrating into other build systems.
With that in mind, we should make sure users can still add other options or possibly customize them.

Having looked more closely at the other PR that lead to this one. I think for test mode we should consider an equivalent approach we used for dev mode - let extension developers provide their jvm options in extension metadata and expose them through the ApplicationModel API. That would mean executing a quarkus goal though prior to surefire.

[geoand]

I think for test mode we should consider an equivalent approach we used for dev mode - let extension developers provide their jvm options in extension metadata and expose them through the ApplicationModel API

I believe that is a very reasonable approach

[aloubyansky]

Another issue is that, if we want to set extension-specific jvm options, doing it at this stage would be too early, since in a multi module project, the modules haven't been built yet, so we won't be able to resolve dependencies to see which extensions will be used in a test.

[Sanne]

I think for test mode we should consider an equivalent approach we used for dev mode - let extension developers provide their jvm options in extension metadata and expose them through the ApplicationModel API

I believe that is a very reasonable approach

I agree with the goal as well - I'm just not 100% sure on consuming exclusibely such static metadata (I believe we have some cases needing more flexibility - but the simple approach is surely tempting).
I'll need figure out how to reconcile this with all the other requirements and the recommendations by @dmlloyd . Might be useful to have a meeting.

Another issue is that, if we want to set extension-specific jvm options, doing it at this stage would be too early, since in a multi module project, the modules haven't been built yet, so we won't be able to resolve dependencies to see which extensions will be used in a test.

This particular aspect should not be a problem: with the solution drafted in #49920 we can reconfigure the modules at any point in time as needed.

In general I'm inclined to prefer build items for the specific JVM flags regarding module configurations, while using the more general purpose approach of static metadata within the extensions for all other JVM flags.

My rationale, for this so far:

• the POC in POC for a buildItem defining the add-opens needs #49920 is highly specific for modules and can't be extended to all JVM flags (would most definitely be impossible). For example, I a won't be able to use such an approach to set --sun-misc-unsafe-memory-access=allow as required by Netty - so that would be a good use case for the extensions metadata.
• the build items give us more flexibility specifically in regards to modules configuration, which is going to be a very active area in the near future, so I think we'll need the extra flexibility

I'm not hell bent on either way though - the build-items introduced in #49920 are the most trivial aspect and I suppose we could take this particular aspect out from my POC for now, and see how far we can get with just static metadata as input for the same kind of solution.

[aloubyansky]

Let's consider the overall picture in terms of use-cases and configuration. If neither approach works for all the use-cases, somehow we should be clear which use-case belongs to which approach.

Also, as mentioned previously, metadata could be the source of data for build items. For example, we do that with capabilities already. Capability requirements are configured in metadata so dev tools can be aware when verifying compatibility of extensions but we also produce CapabilityBuildItems during the build from that metadata.

[Sanne]

Let's consider the overall picture in terms of use-cases and configuration. If neither approach works for all the use-cases, somehow we should be clear which use-case belongs to which approach.

Right. The approach I'm working on focuses on --add-opens, --add-exports and --enable-native-access (for this last one, I still have has some caveats to resolve). These can now be fully controlled in: tests, dev-mode, production native image and production jars.
I'm quite happy with being able to address all these modes consistently - however any other JVM flag can't be covered.

Also, as mentioned previously, metadata could be the source of data for build items. For example, we do that with capabilities already. Capability requirements are configured in metadata so dev tools can be aware when verifying compatibility of extensions but we also produce CapabilityBuildItems during the build from that metadata.

Yes that sounds nice to me!

In the meantime, I've updated this particular PR - if applied as-is it would resolve a good amount of warnings people see during testing, IFF people follow the hints being logged: to enable <extensions>true</extensions>.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 16376a8.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.