[BUG] Maven Dependencies are not (always) correctly parsed

[ascheman]

When analyzing Maven output, the project has to cope with different formatting.

Coloured output

When run interactively (even when started from a local analyser execution in the background),
Maven may use color highlighting (Terminal specific escape sequences) which will result in parsing problems.

Maven Version Problem

Depending on the Maven version, the output of dependency:tree is different, e.g.,

• Maven 3.6.3 outputs each lifecycle execution with maven-dependency-plugin:<version>:tree, while
• Maven 3.9.5 outputs dependency:<version>:tree at the same position.

Maven dependency string splitting problem

Depending on the type of dependency, the dependency plugin prints differently composed strings, e.g.,

• <groupId>:<artifactId>:<packaging>:<version> vs.
• <groupId>:<artifactId>:<packaging>:<classifier>:<version>.

[jmle]

@pranavgaikwad @ascheman I think these issues are probably solved now, right?

[ascheman]

Hmm, I would appreciate if you could either cherry-pick my changes or at least refer to the respective issue (this one) in your commit messages.
I was not aware my reported bugs are (partially) already resolved by your changes, e.g. d295615.
I didn't find much time to work on them myself in the last days.
But I have them on my agenda and spent some time on them today, only to find them partially fixed already in your code by now (without any hint to the respective issue).

Currently, I cannot see how you handled different dependency results (split by ':') correctly, as I suggest it here: qaware@541a2d0?
I wanted to provide some tests for these cases, but I am unsure whether you are working in parallel on this? We should avoid such situations (as I guess we both have enough to do).

Additionally I tried to provide a smaller fix of some logging under the shield of this issue, i.e., qaware@9465c3a.
Will you pick this up also?

[jmle]

@ascheman the color output has been fixed here. The dependency plugin version differences have been addressed here (see the latest commits on that file). The dependency splitting is also addressed there.

[ascheman]

@ascheman the color output has been fixed here. The dependency plugin version differences have been addressed here (see the latest commits on that file). The dependency splitting is also addressed there.

For both it would have been great to refer to #392 in the commit message. Then it would have been easier to correlate the fixes (and to spare me some time analyzing an already fixed problem).

Additionally it would be great to provide unit test cases. I am not sure wether all valid combinations of split-parts are correctly addressed? For example, if their number differs from 5, further analysis is stopped here. Are you sure, dependendies with, e.g., classifiers do not need further analysis?

[jmle]

@ascheman the color output has been fixed here. The dependency plugin version differences have been addressed here (see the latest commits on that file). The dependency splitting is also addressed there.

For both it would have been great to refer to #392 in the commit message. Then it would have been easier to correlate the fixes (and to spare me some time analyzing an already fixed problem).

Additionally it would be great to provide unit test cases. I am not sure wether all valid combinations of split-parts are correctly addressed? For example, if their number differs from 5, further analysis is stopped here. Are you sure, dependendies with, e.g., classifiers do not need further analysis?

We don't have the habit of making references to the issues in commits, we usually refer to them in the PRs themselves, but I think it is a good practice, maybe the rest of the team wouldn't mind implementing this. I had forgotten though about this particular issue, that's why I didn't refer to it.

As far as I could see, all possible combinations of splitting the artifacts' strings were covered (there were two that I could see in version 3.9.5 of the plugin)

[ascheman]

In my opinion, the PR should only be a collection/aggregation of the respective commits.
If you work for days or weeks on a branch (which you should avoid for other reasons, but it can happen), how would you know which bugs where affected by your changes after so much time?
Mentioning them in the respective commit makes it much easier to correlate.

[shawn-hurley]

All the commits for a PR are squashed. You should have a PR per bug fix. We sometimes need to revise this and add two fixes together. We usually attach PRs to Issues by adding fixes #<number>. in the description. This automatically closes them when they are merged.

I know that we don't have the best documentation here, and sorry about that we were heads down building the thing. @savitharaghunathan do we have any contributor docs around this, or can I work with you?

[savitharaghunathan]

All the commits for a PR are squashed. You should have a PR per bug fix. We sometimes need to revise this and add two fixes together. We usually attach PRs to Issues by adding fixes #<number>. in the description. This automatically closes them when they are merged.

I am a +1000000 on this. I also feel there should be a 1 -> 1 or 1 -> many association between issues and PRs. plus sqush commits. 1 commit per PR unless we need more than one commits. there could be exceptions for sure.

I know that we don't have the best documentation here, and sorry about that we were heads down building the thing. @savitharaghunathan do we have any contributor docs around this, or can I work with you?

Not that I know of. If we get a consensus, I'd be happy to add these information.

[savitharaghunathan]

All the commits for a PR are squashed. You should have a PR per bug fix. We sometimes need to revise this and add two fixes together. We usually attach PRs to Issues by adding fixes #<number>. in the description. This automatically closes them when they are merged.

I am a +1000000 on this. I also feel there should be a 1 -> 1 or 1 -> many association between issues and PRs. plus sqush commits. 1 commit per PR unless we need more than one commits. there could be exceptions for sure.

If there is no associated issue for a PR, I think we should create one. This is one thing I am advocating for.

I know that we don't have the best documentation here, and sorry about that we were heads down building the thing. @savitharaghunathan do we have any contributor docs around this, or can I work with you?

Not that I know of. If we get a consensus, I'd be happy to add these information.

[ascheman]

All the commits for a PR are squashed. You should have a PR per bug fix. We sometimes need to revise this and add two fixes together. We usually attach PRs to Issues by adding fixes #. in the description. This automatically closes them when they are merged.

Hmm, why do you squash them (in general)? I think they should tell a story to other developers. I always try to provide a nice commit history to a PR which shows important steps on my way to the solution. Of course, in some cases, squashing helps to streamline the (hi)story of a solution a little bit. But in general it sounds like unnecessarily compressing knowledge and understanding to me.

Having said that, even when you end up squashing the commits of a PR, the related issues should be mentioned at least in the PR itself (and the merge/squash commit). And what would help better to preserve their IDs than referring to the PR(s) in each commit?

[shawn-hurley]

When we merge, we squash. The PR description and title become the commit. Please reference them there.

For a PR, it can make sense to break up the commits, but note that we will squash them, and the PR becomes the commit.

[ascheman]

Policys like we squash can be re-defined.

I kindly ask for a possibility not to destroy the work of a contributor who tries to provide a well-thought commit history. Perhaps, a comment in the PR could prohibit the merging person to squash. In Gitlab you can tell with the PR whether the commits should be squashed or not. This seems to be impossible with Github. But respective comments should do.

[djzager]

Policys like we squash can be re-defined.

I kindly ask for a possibility not to destroy the work of a contributor who tries to provide a well-thought commit history. Perhaps, a comment in the PR could prohibit the merging person to squash. In Gitlab you can tell with the PR whether the commits should be squashed or not. This seems to be impossible with Github. But respective comments should do.

Nothing of what you have stated is false. Well thought out commit histories are very useful.

The reason that we squash commits is that, in combination with enforcing rules on PR titles, we can construct meaningful release notes without manual intervention. The costs incurred to us in supporting multiple merge methods, enforcing commit conventions, and the necessary updates in release note generation far outweigh the potential benefits of more granular commit histories in my opinion.

[ascheman]

@jmle to come back to our original question: I provided a change, which first leads to this error and then subsequently causes #390 to occur (therefore I started #420). As of today this can be seen here.

[jmle]

@ascheman thanks for the failing test. I think the ideal thing would be to close this issue and create a separate PR to fix the dependency parsing, and refer to #390 in the PR.

[ascheman]

Perhaps I need to be more fine granular with each problem I see. I was trying to subsume multiple Maven problems under one issue. Then it's hard to track which of those are fixed and which are still open.

[ascheman]

Closed from my POV by the merge of #437.