[MNG-6802] Fix bug in FileProfileActivator

[dehasi]

FileProfileActivator changes FileProfileActivator.exists
which lets flattened resolveCiFriendliesOnly depending
fail activating profile.

Problem

The root of the problem that FileProfileActivator
interpolates path value and rawModel is updated here
and becomes not raw anymore.

It affects i.e. flatten-maven-plugin

Solution

FileProfileActivator has to have an absolute path to check
if a file exists but shouldn't update Profile.

On the other side DefaultModelBuilder
needs to have interpolated profiles to build an effective model.
To interpolate profiles, DefaultModelBuilder also needs absolute paths.

The interpolation logic is needed in two places.
That's why I extracted the common logic to ProfileActivationFilePathInterpolator.

I use it in both places. In FileProfileActivator I only check if the file exists.
In DefaultModelBuilder I interpolate profiles.

I added DefaultModelBuilderFactoryTest#test_pom_changes()
to check that getRawModel() is still raw and getEffectiveModel() is interpolated.

Also, I added FileProfileActivatorTest to show that the activator doesn't update profiles anymore.

Discussion

Please, let me know if I can do something else to make this PR accepted.

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

• Make sure there is a JIRA issue 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] - Fixes bug in ApproximateQuantiles,
  where you replace MNG-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in 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.

[slannez]

@dehasi Hi, is there anything we can do to help? This bug is also preventing us from using flatten plugin on our projects.

[glimmerveen]

Coincidentally I ran into the same issue, and did not notice your issue/PR when working on a solution as well.
Interestingly I took a different route at addressing this behaviour, you can have a look at my PR here: #348 .

[dehasi]

@glimmerveen I like your solution! It's short and solves the problem.
But:) I still think that FileProfileActivator shouldn't update method parameters.
Anyway, hope at least one of PRs will be accepted the issue will be resolved.

@slannez I also submitted a fix #152 to flatten itself. Probably it also can help you.

[glimmerveen]

My solution looks more like a workaround I guess; and I am not sure if it has any unforeseen consequences :).

I agree, let's hope the underlying problem is quickly solved with either solution!

[slachiewicz]

@elharo commons-lang see #308 and https://issues.apache.org/jira/browse/MNG-6829

[elharo]

see my comment at the end of that issue: plexus-utils and maven-shared-utils are not as well maintained or reliable as commons-lang. I'd pick commons-lang over either of those, given a choice.

[rfscholte]

IIRC the original reason for our own utils is to prevent class collissions. There have been Maven releases where plugins picked up the incompatible version from Maven core. Unsure if this is still an issue, but for that reason I would stay with plexus-utils.

[dehasi]

Dear @elharo @rfscholte @slachiewicz

Correct me if I'm wrong, It looks like my PR raised an important topic,
but you have not agreed about it yet.

But the discussion is slightly out of the scope of the problem, which I'm trying to solve.

Could we focus on the issue with the FileProfileActivator itself?
Can I do anything else to help you fix the bug?

For now, I reverted lang3 to plexus-utils,
not because I support one of the sides but because it was there from the beginning.
So, let's try to make fewer changes.

Once, when you agree, I'll be happy to help you refactor the code.
Seriously, I'm ready to make the tool, that I use on my daily basis, better.
My username in Apache Jira is rgaleyev, so you can just assign a ticket to me.

[glimmerveen]

Are there any updates on the proposed changes? Can these be merged or are there still issues to be resolved?

[rfscholte]

I've assigned MNG-6802 to me, not sure when I'll pick it up as everybody is waiting for a response. Be aware there's a conflict right now.

[dehasi]

@rfscholte I resolved conflicts.

[basvandenbrink]

I would also love to see this fix or fix #348 merged. Quite some people are facing the issues that it aims to solve, because this is the root cause of this issue: e-gineering/gitflow-helper-maven-plugin#105.

@elharo, I see that you were assigned as reviewer. Do you have some time to take a look?

[elharo]

conflicts need to be resolved before this can be reviewed.

[mthmulders]

However those are weird failure messages I don't understand so let's be careful about checking this in. They don't look like a typical temporary maven central failure. Maybe Github actions uses some sort of local mirror that's misbehaving?

@mthmulders created a thread on the dev mailing list about this about 20 hours ago.

That mailing list thread resulted in a potential fix in the integration test suite. That fix has been merged, and the last four runs of the GitHub actions have run successfully. That is of course no guarantee that it'll never fail again, but you could at least give it another go.

[elharo]

rerunning jenkins: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven/job/MNG-6802/3/

[dehasi]

@elharo It looks the build was successful. Can we proceed with the review? I answered your comments.

[elharo]

running through jenkins: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven/job/MNG-6802/

[elharo]

There are a lot of test failures I don't immediately understand. I don't usually work on the core, so you might need to find someone else to help analyze these.

[dehasi]

@elharo I've pulled the last changes from the master. Does it make sense to try to restart the build before I ask for help?

[MartinKanters]

@elharo I've pulled the last changes from the master. Does it make sense to try to restart the build before I ask for help?

Yes, that would make sense. The ITs that failed are one of the newest, so it makes sense that they fail if the latest changes from maven (core) are not merged in this branch. @elharo could you please test the Jenkins build again with the most recent version of @dehasi 's changes? The Github Actions workflow went fine, so I'm assuming Jenkins should run fine as well. (they target the same ITs)

[elharo]

jenkins failed again. Trying one more time:

https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven/job/MNG-6802/6/

[MartinKanters]

jenkins failed again. Trying one more time:

https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven/job/MNG-6802/6/

I believe you are running Jenkins against an older version of his work, the one were master wasn't merged in yet (which @dehasi did yesterday). I pushed his most recent changes branch to asf and the Jenkins build succeeded.

[basvandenbrink]

Great to see it has been merged! Question: will this fix be included in the next 3.x.x release?

[MartinKanters]

@basvandenbrink The next release will be Maven 4.0.0-alpha-1, which is coming "soon" (can't give any real ETA since we are all doing this in spare time).

[michael-o]

@elharo Why didn't you modify the commit message before merging? It is unreable and therefore pointless. Intermediate work should not land when merged.

[basvandenbrink]

@MartinKanters Thanks for your answer. I think it would be great to also have a stable 3.x.x version including the fix. However, I could also understand that you want to spend your limited amount of time on the v4 work.

[michael-o]

@MartinKanters Thanks for your answer. I think it would be great to also have a stable 3.x.x version including the fix. However, I could also understand that you want to spend your limited amount of time on the v4 work.

Could a backport have any side effects? If not, feel free to provide a PR. @rfscholte Would you object a backport?

[rfscholte]

I am kind of surprised that a bug in FileProfileActivator is discovered after so many years. That makes we wonder if people have found workarounds, which now would break their builds.
Let's keep it for Maven 4, which already contains changes in the same cetagory.

[mpepping]

Would a PR for a backport be considered? It would be good to have this fixed in Maven 3 as well.

[michael-o]

@mpepping Likely for at least 3.9.0x. Can you provide a PR rebased on top of 3.8.x?