Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: parent dependencies missing in flattened pom since 1.4.0 #377

Open
hohwille opened this issue Oct 5, 2023 · 26 comments
Open
Labels

Comments

@hohwille
Copy link
Member

hohwille commented Oct 5, 2023

When I have a parent POM that has a compile time dependency X and I do flatten a child POM of it, then flatten-maven-plugin version 1.5.0 does not have X as dependency in the flattened POM.
This is working until version 1.3.0.
Some change broke this - maybe 5b757ae

I am slightly confused why the IT inherit-parent-dependency did not fail (maybe the version of the parent dependency has to be omitted and defined in dependency management).

Here some links to prove the error in my project:

Related issues:

@hohwille hohwille added the bug label Oct 18, 2023
@leonarta
Copy link

leonarta commented Feb 2, 2024

Was it caused by #329?

@hkampbjorn
Copy link

Was it caused by #329?

No, it was #220 that introduced this regression.

Revision 2b1a1d3 works and next revision (aab4485) fails :-/

Here is a patch to reproduce the issue
reproduce-issue-377.patch

@hkampbjorn
Copy link

It turned out that

  • inherit-parent-dependency
  • parent-with-version-range

were configured default to flattenDependencyMode direct by default.

They also failed after implementing the bug fix for #220

Originally posted by @KemalSoysal in #220 (comment)

That's the regression, dependencies in parent are direct dependencies and not transitive dependencies. The inherit-parent-dependency IT did fail, and the "fix" was to change the IT and add flattenDependencyMode: all (instead of direct which is the default)

@KemalSoysal
Copy link
Contributor

KemalSoysal commented May 30, 2024

What exactly is the desired behavior?
I am happy to contribute changes accordingly, but I do not understand the requirement
@hohwille , @leonarta and @hkampbjorn.
Maybe we could layout the tests accordingly to improve.
I am unhappy to have missed, that there was an issue about i#220's implementation and tests.
Maybe we should complete the coverage of the integration tests on requirements first to find out other issues as a first step...

@filip26
Copy link

filip26 commented May 30, 2024

@KemalSoysal try

  • run mvn flatten:flatten on a project using pom_parent.xml, e.g. https://github.com/filip26/titanium-json-ld It uses flatten plugin version 1.3.0 that works as expected.
  • then save the generated pom and upgrade on the latest flatten plugin and run it again.
  • Compare the generated poms. The one generated with newer version would not contain dependencies declared in pom_parent.xml

@KemalSoysal
Copy link
Contributor

KemalSoysal commented May 30, 2024

Hi @filip26,
thank You very much for your quick response:
As outlined in the description the flatten dependency mode is defaulted to direct

The parent_pom.xml configures the flatten plugin with only ossrh flatten-mode (did I miss a configuration somewhere else?).
There is no code or documentation about ossrh-FlattenMode including the all-flatten-dependency-mode.
I think this needs to be configured in the parent_pom.xml accordingly to your desired flatten-dependency-mode all <flattenDependencyMode>all</flattenDependencyMode>

@filip26 , @leonarta , @hohwille, @filip26, @hkampbjorn Please advise how to proceed

If the requirement is, that the flattenDependencyMode is defaulted to all with the flattenModel ossrh then we should locate the integration tests, enhance if needed and implement the desired behavior.
I only found

src/it/projects/optional-elements-modeOssrh/pom.xml
src/it/projects/flatten-shaded-drp/pom.xml

@filip26
Copy link

filip26 commented May 30, 2024

I guess the issue could be in terminology, transitive vs direct dependency. You suggest that a dependency declared outside main pom.xml is transitive, that the difference is in a location of dependency declaration.

Convenient interpretation is that a direct dependency is the one that an artifact requires to compile/run, no matter where the dependency is declared. Transitive dependency is a dependency of a direct dependency or any other transitive dependency.

@KemalSoysal
Copy link
Contributor

KemalSoysal commented May 30, 2024

I tested the pom_parent.xml after adding
<flattenDependencyMode>all</flattenDependencyMode>
with the version 1.3.0 and 1.6.0:

I get a difference: <optional>false</optional>. Is this ok for You, @filip26 ?

@filip26
Copy link

filip26 commented May 30, 2024

@KemalSoysal Well, I don't mind adding the config line, generally, the issue is that this plugin changed behavior unexpectedly - a minor version change. I've released a few artifacts with broken pom because of this, so the real issue is to establish a trust again. For now, I'm sticking with 1.3.0 ;)

@KemalSoysal
Copy link
Contributor

KemalSoysal commented May 30, 2024

I guess the issue could be in terminology, transitive vs direct dependency. You suggest that a dependency declared outside main pom.xml is transitive, that the difference is in a location of dependency declaration.

Communication is always context related and prone to misunderstandings....
the maven way of direct dependency understanding is described here: https://maven.apache.org/pom.html#pom-relationships

@KemalSoysal
Copy link
Contributor

@KemalSoysal Well, I don't mind adding the config line, generally, the issue is that this plugin changed behavior unexpectedly - a minor version change. I've released a few artifacts with broken pom because of this, so the real issue is to establish a trust again. For now, I'm sticking with 1.3.0 ;)

I think there are still a lot of integration tests missing for flatten-maven-plugin....
So we should maybe check the expectations with the documentation....
@hohwille , @leonarta , @hkampbjorn

@KemalSoysal
Copy link
Contributor

KemalSoysal commented May 30, 2024

@filip26 I guess it was hard to test a <scope>provided</scope> dependency....

@hkampbjorn
Copy link

The problem is that 1.4.0 change the behaviour of "direct" from the effective dependencies including those inherited from parent, to exclude those inherited from parent. Without any clarifying updated documentation. To have those inherited dependencies included in versions 1.4.0 up to 1.6.0 we need to change to flatten dependency mode "all" but this will also includes all transitive dependencies. We cannot get the default behaviour in 1.3.0 or before

@KemalSoysal yes there are many small variations that are missing from integration tests, like having dependencies in parent module that have transitive dependencies.

I have a patch that introduces a new "effective" flatten dependency mode, which behaves like 1.2.0 and before, and as "direct" in 1.3.0. And I would also like to make this the default from 1.7.0 on.

I'd like to make "effective" the new default too

PS Personally I consider having compile scope dependencies in parent modules an anti-pattern, but it's allowed in maven, and internally we do have such projects :-(

@KemalSoysal
Copy link
Contributor

Hi @hkampbjorn,

I need your help to really understand the scenario, that you describe.

Could you point to repositories eligible to showcase the flattened-pom problem when changed

  • from --- to direct ?
  • from --- to all?
    where we can compare the outcome with versions 1.3, 1.4,.. 1.6 ?

Or can you provide a simplistic maybe mermaid diagram to define the the dependencies scenario which fail to be flattened correctly like:

---
title pom dependency diagram
---
graph LR;
    B[module B] -->  A[parent A];
    C[module C] -->A;
    D[module D] --> B;
    E[module E]--> C;
    E--> B;
Loading

The problem is that 1.4.0 change the behaviour of "direct" from the effective dependencies including those inherited from parent, to exclude those inherited from parent. Without any clarifying updated documentation. To have those inherited dependencies included in versions 1.4.0 up to 1.6.0 we need to change to flatten dependency mode "all" but this will also includes all transitive dependencies. We cannot get the default behaviour in 1.3.0 or before

  • The default behavior was to get all in that case, as far as I remember.
  • When you mention effective, do you mean the help:effective-pom result?
  • Are you observing, when you make an effective pom from the flattened, then you get different results?

I have a patch that introduces a new "effective" flatten dependency mode, which behaves like 1.2.0 and before, and as "direct" in 1.3.0. And I would also like to make this the default from 1.7.0 on.

What is the difference in the flattened-pom ?

@hkampbjorn
Copy link

In a project like this

graph LR;
 A --> |parent| P;
 P --> B;
 A --> C;
 A --> D;
 D --> F;
 B --> E;
Loading

where A has B, C, and D as direct dependencies and E and F as transitive dependencies. The changes in #220 excludes B from direct dependencies.

Default behaviour in 1.3.0 and before was to include B, C and D. And flatten dependency mode "all" includes B, C, D, E and F

This default behaviour changed in 1.4.0 to only include C and D without B. And "all" hasn't change, there is no way to only include B, C and D, changing mode to "all" will also get E and F

You wanted to exclude the dependencies from P, but these are real dependencies, they are not transitive like E and F. We need a new mode that includes them (or revert #220)

In maven it's possible to declare things in parent modules and they are inherited in child modules. For dependencies there is no way to exclude dependencies declared in parent. It's possible to change version and scope.

I don't understand your need to exclude B from the direct dependencies. That is I don't understand the point of #220, for me it is introducing a regression instead of fixing any defect or introducing a new feature. Maybe it's an optimisation as most parents don't have any dependencies

About "effective" that's what the flatten mojo calls it. It's inside createEffectivePom method that the inheritance assembler is called from. But you are right, the output from help:effective-pom includes all the transitives dependencies, and that is what "all" does. So "effective" could be confusing

Maybe "inherited" is better than "effective"

@KemalSoysal
Copy link
Contributor

KemalSoysal commented Jun 3, 2024

Thank You for Your reply, @hkampbjorn ,

I am sorry for not confirming immediately, that I read your comment and was processing it.

Do I understand correctly, that only for the case of a parent pom, you would like to have B as a direct dependency for parent, because a parent pom's dependencies are inherited by its children as if they were their own dependencies.
If P had parent poms PP and PPP, then also their dependencies and all other inherited elements like pointed out here would be included?

Let us describe the requirement as precise as possible, and then create the integration tests for it.
We also need to review all other integration tests for consistency.

graph LR;
 A --> |parent| P:::pp;
 P -->  |parent| PP:::pp;
 PP -->  |parent| PPP:::pp;
 PP --> PPB;
 PPP --> PPPB;
 P --> B;
 A --> C;
 A --> D;
 D --> F;
 B --> E;
 classDef pp fill:antiquewhite;
Loading

Which artifacts would be included in the flattened pom?

@hkampbjorn
Copy link

Yes, we need a new flatten dependency mode that includes all the direct and inherited dependencies: B, PPB and PPPB, and C and D

@KemalSoysal
Copy link
Contributor

Yes, we need a new flatten dependency mode that includes all the direct and inherited dependencies: B, PPB and PPPB, and C and D

I was thinking, we could introduce the check for the parent pom relation to include inherited elements on direct mode. I thought that might solve the issue.... did I forget something?

For me, it looks like an additional constraint instead of something totally new.

I guess we need a broader participation in this discussion, @hohwille , @hkampbjorn , @leonarta , @filip26

@hkampbjorn
Copy link

@KemalSoysal can you explain the point of Issue #220 because it removed looking up inherited dependencies from parent modules for "direct" flatten dependency mode. If we don't need that, we can just revert it

@KemalSoysal
Copy link
Contributor

KemalSoysal commented Jun 3, 2024

@KemalSoysal can you explain the point of Issue #220 because it removed looking up inherited dependencies from parent modules for "direct" flatten dependency mode. If we don't need that, we can just revert it

We cannot just revert it, because it would mean all transients would be merged into flattened pom...
I understand, that one can expect, maven inheritance rules stay in place.
If we revert, we would get also E and F.

@hkampbjorn
Copy link

@KemalSoysal can you explain the point of Issue #220 because it removed looking up inherited dependencies from parent modules for "direct" flatten dependency mode. If we don't need that, we can just revert it

We cannot just revert it, because it would mean all transients would be merged into flattened pom... I understand, that one can expect, maven inheritance rules stay in place. If we revert, we would get also E and F.

That's not what I see in 1.3.0, I don't see it including transitive dependencies unless I ask for them explicitly. Not from the direct module or from parent modules

@KemalSoysal
Copy link
Contributor

Ok, I will check all of that again, and possibly make a specific branch to test all of the scenarios again and add the one you described, maybe there is something else wrong :-)

@slawekjaranowski
Copy link
Member

Thanks for such discussion and deep analysis
If you need a help with merging or finally release let mention me on PR

@hkampbjorn
Copy link

@KemalSoysal did you have a chance to look at this. Do you need a flatten mode that only includes direct dependencies without inherited dependencies?

@davgia
Copy link

davgia commented Dec 20, 2024

Hi everyone, any update on this? I've bumped from 1.1.0 to 1.6.0 and I've stumble upon this issue. I've now reverted to 1.3.0.

@KemalSoysal
Copy link
Contributor

KemalSoysal commented Dec 20, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants