Skip to content

Conversation

@famod
Copy link
Member

@famod famod commented Jan 21, 2021

So this is what the IMO "least painful" solution would look like to pull up and unify the native Maven profile in integration-tests/pom.xml.
I'd lie by saying I like flag files but standard Maven profile activation features are a bit limited.
More details can be found here: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Moving.20native.20profile.20to.20IT.20parent.3F

Do we want to continue that path? @aloubyansky @geoand @gsmet

PS: mongodb-rest-data-panache is a good example that shows the need for unification:

  • it's missing native.surefire.skip (just like ~8 other modules)
  • it's using the long deprecated goal native-image (just like 103 other modules)

and in general we don't need to repeat things like ${project.build.directory}/${project.build.finalName}-runner over and over again anymore.

@ghost ghost added the area/vault label Jan 21, 2021
@famod
Copy link
Member Author

famod commented Jan 21, 2021

I've added an example (vault-app) of how to add custom systemPropertyVariables. This requires a partial repetition of the profile but only a rather small part of it. Yes, the property activation is still required!

@famod famod added area/housekeeping Issue type for generalized tasks not related to bugs or enhancements and removed area/vault labels Jan 21, 2021
@ghost ghost added the area/vault label Jan 21, 2021
@geoand
Copy link
Contributor

geoand commented Jan 22, 2021

I like this idea! We definitely need some standardization...

@aloubyansky
Copy link
Member

Looks good to me. Thanks @famod!

@famod
Copy link
Member Author

famod commented Jan 22, 2021

Ok, unless @gsmet throws in a timely veto, I'll finalize this and see what CI in my fork is saying (the fun part 😉).

@famod
Copy link
Member Author

famod commented Jan 22, 2021

@galderz There are commented out failsafe execution entries in logging-min-level-set & logging-min-level-unset. Do you want me to keep these?

@geoand
Copy link
Contributor

geoand commented Jan 22, 2021

Yes, that is old (legacy) uber-jar config which can and should be replaced

@ghost ghost added area/core area/dependencies Pull requests that update a dependency file labels Jan 22, 2021
@famod
Copy link
Member Author

famod commented Jan 22, 2021

Yes, that is old (legacy) uber-jar config which can and should be replaced

Ok. After my change there will be a few usages left:

grep -l -R --include=pom.xml --exclude-dir=target '<uberJar>'
extensions/azure-functions-http/maven-archetype/src/main/resources/archetype-resources/pom.xml
integration-tests/maven/src/test/resources/projects/ignore-entries-uber-jar/pom.xml
integration-tests/maven/src/test/resources/projects/uberjar-maven-plugin-config/pom.xml
integration-tests/maven/src/test/resources/projects/command-mode-app-args-plugin-config/pom.xml

This should be cleaned up in a subsequent PR.

@aloubyansky
Copy link
Member

@famod
Copy link
Member Author

famod commented Jan 22, 2021

@aloubyansky

if you update the code using https://github.com/quarkusio/quarkus/blob/master/devtools/maven/src/main/java/io/quarkus/maven/BuildMojo.java#L33 that'll help.

You mean

<quarkus.package.uber-jar>true</quarkus.package.uber-jar>

instead of

<quarkus.package.type>uber-jar</quarkus.package.type>

?
How is this better (honest question)? It seems it is not even documented...?

@aloubyansky
Copy link
Member

Just look for the uses of that constant. QuarkusBootstrapProvider is actually setting that deprecated property. Which results in the error you posted above.

@ghost ghost added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Jan 22, 2021
@famod
Copy link
Member Author

famod commented Jan 22, 2021

The next CI run should hopefully be all green! The problem was how I was setting quarkus.package.type:
Since "parent profiles" cannot override submodule properties, some native tests were actually running with uber-jar instead of native.
I had to resort to a few !native profiles and left some very prominent comments why this is required, especially in the parent native profile.

@famod
Copy link
Member Author

famod commented Jan 23, 2021

@geoand it took me a while to figure out what's going on in the last failure I'm having but turns out it is "caused" by #14387.
spring-data-jpa does not compile a native image anymore because the native profile is now active in the parent, not in the concrete submodule and so the fast-jar setting from application.properties always wins.
So, I think I'll have to add a parent check here? https://github.com/quarkusio/quarkus/blob/master/devtools/maven/src/main/java/io/quarkus/maven/BuildMojo.java#L137

@geoand
Copy link
Contributor

geoand commented Jan 24, 2021

Oh, wow!

Indeed, that will be needed if this is to work.

+1 from me

@aloubyansky
Copy link
Member

@famod you mean that in this case the native profile will be present in the parent but not the current project?

@famod
Copy link
Member Author

famod commented Jan 24, 2021

@aloubyansky yes, that's the thing with Maven profiles in the parent: they are not "inherited"! Maven applies the profile in/for the parent when building the model for it which is then merged with the child model.
So you "inherit" the effects of the profile activation, but not the profile itself. That's also why you cannot override a property in the child via a parent profile.

@ghost ghost added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Jan 24, 2021
@famod famod marked this pull request as ready for review January 24, 2021 10:04
@famod famod requested review from aloubyansky and geoand January 24, 2021 10:04
@famod
Copy link
Member Author

famod commented Jan 24, 2021

So I think I'm done with this. I double checked everything. I did squash but I think those three distinct commits make sense since they address different things.

run: tar -xzf maven-repo.tgz -C ~
- name: Build with Maven
shell: bash
run: mvn -B --settings .github/mvn-settings.xml -DskipDocs -Dno-native -Dformat.skip -pl !integration-tests/gradle -pl !integration-tests/maven -pl !integration-tests/devtools install
Copy link
Member Author

Choose a reason for hiding this comment

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

-Dno-native must stem from the early times of Quarkus as I couldn't find any reference to it.
Since I had to introduce a few new no-native profiles, I wanted to avoid confusion and so I removed this system property.

private boolean isNativeProfileEnabled(MavenProject mavenProject) {
// gotcha: mavenProject.getActiveProfiles() does not always contain all active profiles (sic!),
// but getInjectedProfileIds() does (which has to be "flattened" first)
Stream<String> activeProfileIds = mavenProject.getInjectedProfileIds().values().stream().flatMap(List<String>::stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to know!

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

I like this a lot. Big +1 from me, but let's hear from @aloubyansky as well

Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

Good one @famod Thanks!

@famod
Copy link
Member Author

famod commented Jan 25, 2021

I had a quick look around and it seems no critical open PR will be affected by this (in terms of merge conflicts etc.) so I'll go ahead and merge this.
Thanks guys for your promptly review!

@famod famod merged commit 4868172 into quarkusio:master Jan 25, 2021
@ghost ghost added this to the 1.12 - master milestone Jan 25, 2021
@famod famod deleted the IT-native-parent branch January 25, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants