Add multi-build support in a single module #16054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think it'd be better instead of setting system properties to turn them into the build properties, i.e. add the properties parameter to https://github.com/quarkusio/quarkus/blob/main/devtools/maven/src/main/java/io/quarkus/maven/QuarkusBootstrapMojo.java and set them like https://github.com/quarkusio/quarkus/blob/main/devtools/maven/src/main/java/io/quarkus/maven/QuarkusBootstrapProvider.java#L139 WDYT? Thanks for the great contribution, btw! Just in case, fyi, @famod |
|
Yeah, great contribution! |
I see, it makes sense but do we agree that those properties will be set as system properties at the end? Indeed, otherwise a parameter like the Quarkus profile won't be taken into account and setting the Quarkus profile is necessary to be able to fulfill the requirements of this feature request.
Good point, I'm afraid that indeed this approach won't help much in your case since the dependencies will be the same, as you mentioned (maybe a little in native mode? if it removes dead code, not sure and not tested) however it could be interesting as the next step of this improvement, WDYT? |
Besides the Quarkus profile one, would the rest work as project properties for you? Or perhaps in that case you could model all that with Maven profiles instead? I haven't checked, would setting system properties break in case of parallel builds? |
We will test it asap with @bcournaud
Good question, not tested either but I guess it should not work, however AFAIK custom Quarkus profiles can only be set thanks to a System property, an environment variable or a static field, in other words there isn't any easy way to isolate the value unless we use a dedicated ClassLoader per build, or we make |
|
Right, which is why I am hesitating to set system properties by default. Quarkus profile is a known exception for now. |
quarkus-bot
bot
added
area/devtools
bcournaud
force-pushed
the
multi-build-artifact
branch
from
1ff96c9 to
8e851d0
Compare
aloubyansky
reviewed
Apr 1, 2021
devtools/maven/src/main/java/io/quarkus/maven/QuarkusBootstrapProvider.java
Outdated
Show resolved
Hide resolved
|
Thanks for the suggestions @aloubyansky. |
|
@bcournaud Thanks! Could you please squash the commits and un-draft it please? |
essobedo
force-pushed
the
multi-build-artifact
branch
from
017a15b to
6fd0efc
Compare
essobedo
reviewed
Apr 6, 2021
| @@ -30,7 +34,7 @@ | |||
| /** | |||
| * Builds the Quarkus application. | |||
| */ | |||
| @Mojo(name = "build", defaultPhase = LifecyclePhase.PACKAGE, requiresDependencyResolution = ResolutionScope.COMPILE_PLUS_RUNTIME, threadSafe = true) | |||
There was a problem hiding this comment.
We mark it as non thread safe as long as the problem with the quarkus profile (set as a System property) is not fixed
There was a problem hiding this comment.
Just curious, have you actually verified the system property set in one Mojo is visible in another one? You haven't I will check that.
There was a problem hiding this comment.
You can certainly re-check this but in the past I have filed multiple issues for various maven plugins that fell in the same trap, e.g. NPEs during iterating the properties while another Maven thread added or removed a property.
There was a problem hiding this comment.
Was that project properties of system properties? Thanks for the info @famod
There was a problem hiding this comment.
@aloubyansky yes I confirm that I have tested with a project with two modules, in each module I have 2 executions, in the first module, the first execution sets the Quarkus profile to foo and the second one sets the Quarkus profile to bar and in the second module, it does the opposite (first bar and second foo). I launched mvn -T 1C clean install and ended up with a build success but incorrect Quarkus profile set at runtime (Quarkus profile set to bar instead of foo and vice versa).
In other words, I confirm that system properties make the plugin non thread safe unless you use the same system properties across all your modules.
There was a problem hiding this comment.
@aloubyansky If you want to keep it thread safe, we can try to investigate more to see how we can isolate the Quarkus profile (by relying on the ClassLoaders for example) but it won't be easy
Test Failures⚙️ jvm-linux-jdk11📦 integration-tests/kafka# Tests: 10
+ Success: 8
- Failures: 0
- Errors: 2
! Skipped: 0❌
|
Failing Jobs
Test Failures⚙️ jvm-linux-jdk15📦 integration-tests/kafka# Tests: 10
+ Success: 8
- Failures: 0
- Errors: 2
! Skipped: 0❌
|
|
The CI failures don't seem to be related. |
aloubyansky
approved these changes
Apr 7, 2021
|
Thank you @bcournaud and @essobedo for another great contribution! |
|
Is there any follow up issue tracking the |
|
@famod We did not create any ticket for this topic. If you create one, please let me know, I can have a look asap. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #16053
Motivations
Be able to build the same module multiple times with different profiles, generating different artifacts.
Modifications
A new parameter has been added to the
BuildMojothat contains the system properties of the plugin.These properties are added to the global system properties if they are not already defined.
Once the build is done, these plugin properties are removed from the system properties so that they are not taken into account by another build which may have different properties.
All the plugin system properties are now used during the execution
Note that if multiple executions are defined, the
quarkus.package.output-directoryproperty must be defined and different for each execution in order to store the resulting artifact in a specific folder for each execution (instead of overriding the artifacts at each build).