[MNG-6716] Test and fix erroneous compileRoots

[mickaelistria]

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.

[michael-o]

I am trying to understand the fix, but I don't. Although if I look at the bug report the OP add the same path twice in different representations. How does this fix address this?

[mickaelistria]

The comment in the code tries to explain it, but probably not that well.
I'll try again here and once we found the right words, or a better fix, I'll update the PR to makes things clearer.

So the point is that there are 2 passes done to build a MavenProject. It was the case before the changes I made first. Before my change, the project built on 1st pass was build with "raw" values (no resolution as parent wasn't set) was discarded and a new project was then build on 2nd pass (with some resolved values) using the result of the 1st pass, but really creating new objects.
That had some flows identified in other tickets, so I changed it so the project of 1st pass is reused in 2nd pass.
However, the issue it caused for MNG-6716 is that for some "fields" of the MavenProject -like the testCompileSourceRoots-, the 2nd pass wasn't replacing the values of the 1st one but was instead appending its values to the result of 1st pass. However, for most of the fields, the result of 1st pass are irrelevant because values are not resolved (read in this case path are not resolved to absolute ones) and shouldn't be set on the project because they're useless at that time.
That's what the patch does: when the value seem incomplete, we don't set them and they will be set on the 2nd pass.

Now I'm writing that, I think the fix is a bit weak. In general, we should instead identify what needs to be set on 1st pass and only set that, nothing more; and then on 2nd pass, set the other things.
However, changing that seems like a huge and risky work compared to the current patch.

[mickaelistria]

@michael-o I gave a try to another approach that's hopefully cleaner and easier to understand. The commit was updated and is ready for a new review.

[michael-o]

@rfscholte Can you drop some lines? I guess don't know that part of code to apply proper judgement here.

[rfscholte]

@michael-o your guess is as good as mine. It seems like these sourceRoots already have a special meaning, which might look like a design flaw in the MavenProject class (something to look at with Maven4 / Maven-API), however that doesn't help with the regression.
This fix does reflect that only some parts of the pom are having issues since 3.6.1. So ensure the unittest and ITs still work and let the reporter verify it on his project.

[michael-o]

OK, I will push a branch and we'll see how far we get.

[michael-o]

ITs are running...

[michael-o]

It passes locally for me, but does not show up on Jekins. Any idea?

[rfscholte]

I had that issue too. Trick is to hit "Scan Multibranch Pipeline Now" in Jenkins to force it manually. Need to investigate the cause

[michael-o]

So all ITs have passed on Jenkins. @mickaelistria Would you be so kind and test this out with the sample project of the reporter since he's offline for some while. If all is well, I will merge. If @rfscholte does not mind.

[mickaelistria]

I already tested it before submittinh the patch and the probable cause is fixed.bit I was never able to reproduce the actual symptom on Linux.

[rfscholte]

It looks like we're still suffering from issues from last week, https://builds.apache.org/job/maven-box/job/maven/job/MNG-6716/ is unstable. I would prefer a stable Maven before starting to merge new fixes.

[hboutemy]

1. I was able to reproduce the issue (on a Mac),
2. I was able to check that the PR fixes the issue,
3. I re-launched the build; and it now passes: https://builds.apache.org/job/maven-box/job/maven/job/MNG-6716/

everything looks ok to merge to master (and be able to launch 3.6.2 release process), isn't it?

[eolivelli]

#shipit

[michael-o]

@hboutemy, I trust your judgement. If you bless, I'll do it too.

[hboutemy]

@michael-o you asked (on the Jira issue) the user to check that the PR fixed the issue: I did it
Robert wanted to see a green build: it is there
I personally don't get more the content of the fix than you and Robert: I'm ok with you on "merge when ev erything is green"

[michael-o]

@hboutemy, let's go for it!

[Tibor17]

It looks like we're still suffering from issues from last week, https://builds.apache.org/job/maven-box/job/maven/job/MNG-6716/ is unstable. I would prefer a stable Maven before starting to merge new fixes.

It should be a good rule in every project that we would push PR before and after green Jenkins. Even some companies have a semaphore in the rooms and they stop the development if the build fails and they fix the build together. So there's only single failure to investigate at most.

[eolivelli]

committed to master branch, thank you @mickaelistria
I feel we are ready to cut 3.6.2

[Tibor17]

Would be a new issue important in Resolver for the Maven release? apache/maven-resolver@3e6eb78#commitcomment-34748393

…

On Mon, Aug 19, 2019 at 11:45 PM Enrico Olivelli ***@***.***> wrote: committed to master branch, thank you @mickaelistria <https://github.com/mickaelistria> I feel we are ready to cut 3.6.2 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#274?email_source=notifications&email_token=AAH7ER4FEF3PBCGGBEUG35LQFMH6FA5CNFSM4IISF3W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4UMPXA#issuecomment-522766300>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH7ER2FS7Q2MXJ7YFNF2NTQFMH6FANCNFSM4IISF3WQ> .