[MNG-8081] interpolate available properties during default profile selection (Maven 4.x) #1446

mbenson · 2024-03-18T22:37:08Z

Evaluate profile activations after having interpolated embedded property constructs.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue MNG-8081 filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] SUMMARY,
where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue.
Also format the first line of the commit message like [MNG-XXX] SUMMARY.
Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

ICLA on file

maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java

...n-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java

maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java

mbenson · 2024-03-19T19:41:03Z

Second iteration:

test case fails without updated code
using MavenTransformer for interpolation

gnodet · 2024-03-20T07:07:57Z

The code looks good.

However, what's the real use case for that ? It seems actually unrelated to MNG-5235, in a sense that the behaviour on this project is not modified by this PR. My understanding is that the use case for MNG-5235 is to activate profile based on project properties, while this PR is about interpolating activation property values (but not based on project properties). Also note that properties defined from activated external profiles are already taken into account, see MNG-2276.
Should this require a different JIRA issue ? I'm not really sure to understand the use case, specifically why you'd want the activation part to possibly change from a maven run to another ?

mbenson · 2024-03-20T11:57:20Z

My use case is to be able to compare environment variables. Specifically, with GitLab CI, if CI_COMMIT_REF_NAME has the same value as CI_DEFAULT_BRANCH, then I want to enable stricter enforcer checks. For my purposes I'd rather the project make assumptions about the CI than vice versa.

gnodet · 2024-03-20T14:04:47Z

My use case is to be able to compare environment variables. Specifically, with GitLab CI, if CI_COMMIT_REF_NAME has the same value as CI_DEFAULT_BRANCH, then I want to enable stricter enforcer checks. For my purposes I'd rather the project make assumptions about the CI than vice versa.

Not sure how you'll model that with the limited support we have right now, do you ?
Maybe we should extend the model activation to support a simple language which provide basic operations such as = and !=. Something like:

<property>
  <expression>${env. CI_COMMIT_REF_NAME} = ${env. CI_DEFAULT_BRANCH}</expression>
</property>

At some point, it may be easier to provide a custom Maven extension that would compute a system property based on those two variables so you can easily activate a profile based on the computed property. Maven 4 added org.apache.maven.api.spi.PropertyContributor for such use case (to have a cleaner api, but it's possible in Maven 3 using the AbstractMavenLifecycleParticipant. See https://github.com/trustin/os-maven-plugin/tree/master and trustin/os-maven-plugin#74
An example

mbenson · 2024-03-20T14:11:20Z

Hmm, my approach was, e.g.:

<activation>
  <property>
    <name>env.CI_COMMIT_REF_NAME</name>
    <value>${env.CI_DEFAULT_BRANCH}</value>
  </property>
</activation>

...which seems fine to me 🤷

rmannibucau · 2024-03-20T14:13:24Z

personally i do it in the sh part of the yaml:

case ${DEPLOY_ENV:-dev} in
  dev)
    export SCRIPTS_DEPLOY_DEPLOYMENT_PROFILES="-Pshared-dev -Dmonitoring.enabled=true"
    export SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS="minikube"
    ;;
  *)
    export SCRIPTS_DEPLOY_DEPLOYMENT_PROFILES="-P$DEPLOY_ENV"
    export SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS="${SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS:-all-in-one}"
    ;;
esac

# this shared script just runs mvn with the SCRIPTS_DEPLOY_... options but the magic literally happens in previous case and is open bar on the CI
/usr/local/bin/deploy.sh

No assumption needed all is detected in the build pipeline (where it belongs) and project itself just enables to activate it or not.

mbenson · 2024-03-20T14:21:01Z

@rmannibucau but your pipeline has to know/hope that the project has such a shared-dev profile. For my purposes my pipeline wouldn't know what such a profile might be named, but my org's projects do know what CI they run on (in the worst case, migrating to a different CI--which won't actually happen, but if it did--it would be pretty easy to make XXX CI emulate GitLab CI by setting the same environment variables).

gnodet · 2024-03-20T14:30:29Z

Hmm, my approach was, e.g.:

<activation>
  <property>
    <name>env.CI_COMMIT_REF_NAME</name>
    <value>${env.CI_DEFAULT_BRANCH}</value>
  </property>
</activation>

...which seems fine to me 🤷

Yes, obviously that'll work 😊

gnodet · 2024-03-20T15:07:09Z

maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java

@@ -76,16 +85,24 @@ public List<org.apache.maven.api.model.Profile> getActiveProfilesV4(
    @Override
    public List<Profile> getActiveProfiles(
            Collection<Profile> profiles, ProfileActivationContext context, ModelProblemCollector problems) {
+
+        if (profiles.stream().map(Profile::getId).distinct().count() < profiles.size()) {


What is this test about ? That seems unrelated to this PR. Should this be rather validated (with a warning printed) earlier in the process ? During settings validation and project validation ?

What I found during testing was that deliberately failing test cases where the same id was specified on multiple profiles seemed to reach this code, and break my collect-to-map, prior to failing the whole build due to the illegal profile duplication. So my thinking was that since we know at this point that we're in an illegal situation, we can just assume no profiles may be activated and the usual mechanisms will fail the build in due course (of course I am approaching all this as a superficial contributor with an incomplete understanding of Maven's guts).

rmannibucau · 2024-03-20T15:42:12Z

@mbenson see it the other way, if (magic) -DdoItSlow then you can use this new fake variable accordingly. You can migrate but the easiest is a custom base image IMHO which does this for you and abstracts this logic.

mbenson · 2024-03-20T18:03:17Z

@rmannibucau I think it's okay if different users want to interact with Maven in different ways. Your preference does not invalidate mine.

mbenson · 2024-03-20T18:04:24Z

@gnodet I agree that MNG-5235 has a different ask than this PR addresses. I will open a new issue and rewrite this commit message accordingly (pushed to same branch name to keep using this PR).

rmannibucau · 2024-03-20T18:06:19Z

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

gnodet · 2024-03-20T20:28:02Z

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

It seems it can be solved really easily though:
https://github.com/kpiwko/el-profile-activator-extension/blob/master/src/main/java/com/redhat/jboss/maven/elprofile/ElProfileActivator.java

gnodet · 2024-03-20T20:36:22Z

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

It seems it can be solved really easily though: https://github.com/kpiwko/el-profile-activator-extension/blob/master/src/main/java/com/redhat/jboss/maven/elprofile/ElProfileActivator.java

That makes we wonder if this PR targets the wrong location. Should we modify the PropertyProfileActivator instead ?

mbenson · 2024-03-20T20:38:33Z

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

gnodet · 2024-03-20T20:57:53Z

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

How could that (solving similar use cases) be considered a bad thing ?

mbenson · 2024-03-21T01:28:08Z

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

How could that (solving similar use cases) be considered a bad thing ?

That's what I wanted to know 😮
i.e., I think this approach takes care of them in one place before everybody comes asking for similar changes to be made everywhere.

mbenson · 2024-03-21T14:17:51Z

From email thread:

Guillaume Nodet gnodet@apache.org wrote:

I thought you were referring to having a small language in the
property activation...

I hadn't noted the discussion when it took place before, but fwiw I also lean in the direction that an embedded language might be going too far. At the same time EL, as in the extension you mentioned, seems a fine choice, but feels fine to leave that as an extension.

Matt

gnodet · 2024-03-22T10:44:49Z

From email thread:

Guillaume Nodet gnodet@apache.org wrote:

I thought you were referring to having a small language in the
property activation...

I hadn't noted the discussion when it took place before, but fwiw I also lean in the direction that an embedded language might be going too far. At the same time EL, as in the extension you mentioned, seems a fine choice, but feels fine to leave that as an extension.

Matt

@mbenson I just spotted that the model builder performs some interpolation for file base activation in

maven/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java

Lines 896 to 943 in 5029cc2

    
           private List<org.apache.maven.api.model.Profile> interpolateActivations( 
        
                   List<org.apache.maven.api.model.Profile> profiles, 
        
                   DefaultProfileActivationContext context, 
        
                   DefaultModelProblemCollector problems) { 
        
               List<org.apache.maven.api.model.Profile> newProfiles = null; 
        
               for (int index = 0; index < profiles.size(); index++) { 
        
                   org.apache.maven.api.model.Profile profile = profiles.get(index); 
        
                   org.apache.maven.api.model.Activation activation = profile.getActivation(); 
        
                   if (activation != null) { 
        
                       org.apache.maven.api.model.ActivationFile file = activation.getFile(); 
        
                       if (file != null) { 
        
                           String oldExists = file.getExists(); 
        
                           if (isNotEmpty(oldExists)) { 
        
                               try { 
        
                                   String newExists = interpolate(oldExists, context); 
        
                                   if (!Objects.equals(oldExists, newExists)) { 
        
                                       if (newProfiles == null) { 
        
                                           newProfiles = new ArrayList<>(profiles); 
        
                                       } 
        
                                       newProfiles.set( 
        
                                               index, profile.withActivation(activation.withFile(file.withExists(newExists)))); 
        
                                   } 
        
                               } catch (InterpolationException e) { 
        
                                   addInterpolationProblem(problems, file, oldExists, e, "exists"); 
        
                               } 
        
                           } else { 
        
                               String oldMissing = file.getMissing(); 
        
                               if (isNotEmpty(oldMissing)) { 
        
                                   try { 
        
                                       String newMissing = interpolate(oldMissing, context); 
        
                                       if (!Objects.equals(oldMissing, newMissing)) { 
        
                                           if (newProfiles == null) { 
        
                                               newProfiles = new ArrayList<>(profiles); 
        
                                           } 
        
                                           newProfiles.set( 
        
                                                   index, 
        
                                                   profile.withActivation(activation.withFile(file.withMissing(newMissing)))); 
        
                                       } 
        
                                   } catch (InterpolationException e) { 
        
                                       addInterpolationProblem(problems, file, oldMissing, e, "missing"); 
        
                                   } 
        
                               } 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
               return newProfiles != null ? newProfiles : profiles; 
        
           }

Would it make sense to move this code in this block to interpolate ? It seems more intuitive that the two interpolations would actually be performed at the same time...

mbenson · 2024-03-22T12:04:02Z

That is possible, sure. I'll take a look in a bit.

UPDATE: Yes, if there is already an "interpolate activations" activity taking place in the model builder, I agree it makes sense to add the expanded functionality there. In an early iteration of this work I encountered test failures relating to file paths when I allowed the interpolations to remain in the model, but reviewing this section of the code I see that that probably related to the relative path computation that's being done for file activations, so hopefully by taking that into consideration I can avoid such issues this time around.

mbenson · 2024-03-22T18:35:12Z

Here is the version that enhances DefaultModelBuilder. Can anyone offer ideas on the likely shape of unit tests for this approach?

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java

mbenson · 2024-03-26T23:17:32Z

By way of [some] testing, I went ahead and expanded the profile activation model validation to consider the whole activation, similarly to how we are now interpolating the whole activation.

Here I could be construed as misusing the MavenTransformer to walk the activation; if this is deemed offensive we could consider generating a similar MavenWalker class.

mbenson · 2024-03-27T14:18:37Z

Thanks @gnodet and @rmannibucau for the approvals! What is the next step?

gnodet · 2024-04-11T15:38:15Z

Thanks @gnodet and @rmannibucau for the approvals! What is the next step?

I'm nearly done with http://github.com/apache/maven/pull/1457, so as soon as that one is merged, we'll be able to port this PR to the new model builder easily.

mbenson · 2024-04-12T16:11:51Z

@gnodet do you have any thoughts on the best way to backport this feature to Maven 3? #1447 is my original PR, but a wildly divergent implementation may cause headaches in future. I wonder if it would make sense [from a future merge perspective] to stabilize the v3 impl and include the same commit in this branch prior to being replaced by the actual v4 implementation. 🤔

gnodet · 2024-04-23T20:50:21Z

@mbenson I've rebased the PR and ported it to the new v4 model builder, please have a look

mbenson · 2024-04-26T16:02:41Z

hi @gnodet ... so with the implementation having been largely duplicated from model-builder into api-impl, would the similar code remain in both places indefinitely, or will the model-builder code eventually point directly to api-impl?

gnodet · 2024-04-26T17:54:56Z

hi @gnodet ... so with the implementation having been largely duplicated from model-builder into api-impl, would the similar code remain in both places indefinitely, or will the model-builder code eventually point directly to api-impl?

The code in model-builder should be deprecated, it will be kept for compatibility with 3.x plugins for some time, and then removed. Not sure about the exact dates though. The point is that model-builder is not really used anymore in beta-1.

maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java

…ivations

mbenson · 2024-04-27T17:14:46Z

Incorporated noneMatch as suggested. IDK how I missed that pair of Stream methods all this time. 😕

mbenson · 2024-05-02T14:27:53Z

@gnodet again, thanks!

mbenson force-pushed the mng-5235@4x branch 4 times, most recently from 6f3dfe9 to 739cba8 Compare March 19, 2024 00:45

gnodet requested changes Mar 19, 2024

View reviewed changes

maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java Outdated Show resolved Hide resolved

...n-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java Outdated Show resolved Hide resolved

gnodet reviewed Mar 19, 2024

View reviewed changes

maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java Outdated Show resolved Hide resolved

mbenson force-pushed the mng-5235@4x branch from 739cba8 to d42c7e7 Compare March 19, 2024 19:38

mbenson requested a review from gnodet March 19, 2024 19:39

gnodet reviewed Mar 20, 2024

View reviewed changes

mbenson force-pushed the mng-5235@4x branch from d42c7e7 to 4285c57 Compare March 20, 2024 18:13

mbenson changed the title ~~[MNG-5235] interpolate available properties during default profile selection (Maven 4.x)~~ [MNG-8081] interpolate available properties during default profile selection (Maven 4.x) Mar 20, 2024

mbenson force-pushed the mng-5235@4x branch 2 times, most recently from ffe2dd6 to 2982758 Compare March 22, 2024 17:47

hboutemy force-pushed the master branch from a075afa to 6e50886 Compare March 24, 2024 21:16

gnodet reviewed Mar 25, 2024

View reviewed changes

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java Outdated Show resolved Hide resolved

mbenson force-pushed the mng-5235@4x branch from 2982758 to 18a1b24 Compare March 26, 2024 01:42

gnodet approved these changes Mar 26, 2024

View reviewed changes

rmannibucau approved these changes Mar 27, 2024

View reviewed changes

cstamas approved these changes Apr 11, 2024

View reviewed changes

gnodet force-pushed the mng-5235@4x branch from 66bbdc6 to 8bc934b Compare April 23, 2024 20:49

gnodet added this to the 4.0.0-beta-1 milestone Apr 23, 2024

gnodet self-assigned this Apr 25, 2024

gnodet reviewed Apr 27, 2024

View reviewed changes

maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java Show resolved Hide resolved

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java Outdated Show resolved Hide resolved

mbenson and others added 4 commits April 27, 2024 12:04

interpolate profile activations in DefaultModelBuilder#interpolateAct…

37e5dce

…ivations

refactor

b0c48be

validate all profile activation config strings

3eb2705

Port to the new v4 builder

fe7b78d

mbenson force-pushed the mng-5235@4x branch from 8bc934b to fe7b78d Compare April 27, 2024 17:06

Simplify

6883eb4

gnodet merged commit 399f8b4 into apache:master May 2, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-8081] interpolate available properties during default profile selection (Maven 4.x) #1446

[MNG-8081] interpolate available properties during default profile selection (Maven 4.x) #1446

mbenson commented Mar 18, 2024 •

edited by hboutemy

Loading

mbenson commented Mar 19, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024 •

edited

Loading

rmannibucau commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024 •

edited

Loading

gnodet Mar 20, 2024

mbenson Mar 20, 2024 •

edited

Loading

rmannibucau commented Mar 20, 2024

mbenson commented Mar 20, 2024

mbenson commented Mar 20, 2024 •

edited

Loading

rmannibucau commented Mar 20, 2024

gnodet commented Mar 20, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024 •

edited

Loading

mbenson commented Mar 21, 2024

mbenson commented Mar 21, 2024

gnodet commented Mar 22, 2024

mbenson commented Mar 22, 2024 •

edited

Loading

mbenson commented Mar 22, 2024

mbenson commented Mar 26, 2024

mbenson commented Mar 27, 2024

gnodet commented Apr 11, 2024

mbenson commented Apr 12, 2024

gnodet commented Apr 23, 2024

mbenson commented Apr 26, 2024

gnodet commented Apr 26, 2024

mbenson commented Apr 27, 2024

mbenson commented May 2, 2024

[MNG-8081] interpolate available properties during default profile selection (Maven 4.x) #1446

[MNG-8081] interpolate available properties during default profile selection (Maven 4.x) #1446

Conversation

mbenson commented Mar 18, 2024 • edited by hboutemy Loading

mbenson commented Mar 19, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024 • edited Loading

rmannibucau commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024 • edited Loading

gnodet Mar 20, 2024

Choose a reason for hiding this comment

mbenson Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

rmannibucau commented Mar 20, 2024

mbenson commented Mar 20, 2024

mbenson commented Mar 20, 2024 • edited Loading

rmannibucau commented Mar 20, 2024

gnodet commented Mar 20, 2024

gnodet commented Mar 20, 2024

mbenson commented Mar 20, 2024

gnodet commented Mar 20, 2024 • edited Loading

mbenson commented Mar 21, 2024

mbenson commented Mar 21, 2024

gnodet commented Mar 22, 2024

mbenson commented Mar 22, 2024 • edited Loading

mbenson commented Mar 22, 2024

mbenson commented Mar 26, 2024

mbenson commented Mar 27, 2024

gnodet commented Apr 11, 2024

mbenson commented Apr 12, 2024

gnodet commented Apr 23, 2024

mbenson commented Apr 26, 2024

gnodet commented Apr 26, 2024

mbenson commented Apr 27, 2024

mbenson commented May 2, 2024

mbenson commented Mar 18, 2024 •

edited by hboutemy

Loading

mbenson commented Mar 20, 2024 •

edited

Loading

gnodet commented Mar 20, 2024 •

edited

Loading

mbenson Mar 20, 2024 •

edited

Loading

mbenson commented Mar 20, 2024 •

edited

Loading

gnodet commented Mar 20, 2024 •

edited

Loading

mbenson commented Mar 22, 2024 •

edited

Loading