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

[MNG-7982] Switch to enable transitivity in depMgr used by Maven #1357

Merged
merged 9 commits into from
Jan 11, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Dec 20, 2023

Resolver 2.0.0-alpha-6 introduced "transitive" switch to dependency Manager, make use of it.


https://issues.apache.org/jira/browse/MNG-7982

Make the "transitive" depMgr the default in Maven4, while
provide fallback to Maven3 used depMgr if needed.

---

https://issues.apache.org/jira/browse/MNG-7982
@cstamas cstamas requested a review from gnodet December 20, 2023 11:48
@cstamas cstamas self-assigned this Dec 20, 2023
@rmannibucau
Copy link
Contributor

Hi @cstamas , what is the plan to identify mvn 4 consumed artifacts to switch the impl, using the mvn4 specific pom metadata file? in current state it breaks probably too easily IMHO and I didn't find how we enable this behavior in the produced pom for the consumed artifact - I don't think it is good to be a config prop, it should belong to the pom only to avoid a global toggle which will work only under a closed ecosystem.

@cstamas cstamas changed the title [MNG-7982] Make transitive the default depMgr [MNG-7982] Make classicTransitive the default depMgr Dec 20, 2023
@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

@rmannibucau this PR is draft on purpose. Actually, I'd say "consider all resolver related Maven PRs draft" I do, as am just throwing ideas or missing Maven features, to improve use of (so far unused or newly made) Resolver features in Maven, but HOW are they gonna be consumed/integrated into Maven I'd leave to @gnodet and probably you and others 😄

This PR would fix for example these issues, reported by many, while providing "escape hatch" to go back to "classic" if needed:
https://issues.apache.org/jira/browse/MRESOLVER-235
https://issues.apache.org/jira/browse/MNG-7003

@cstamas cstamas added this to the 4.0.0-alpha-12 milestone Dec 20, 2023
@gnodet
Copy link
Contributor

gnodet commented Dec 20, 2023

Hi @cstamas , what is the plan to identify mvn 4 consumed artifacts to switch the impl, using the mvn4 specific pom metadata file? in current state it breaks probably too easily IMHO and I didn't find how we enable this behavior in the produced pom for the consumed artifact - I don't think it is good to be a config prop, it should belong to the pom only to avoid a global toggle which will work only under a closed ecosystem.

The consumer has no knowledge. I also kinda saw that as a problem...
I wonder if this should be automatically toggled by the modelVersion of the root project: if using a 4.1.0 model, then use the new depMgr, if using 4.0.0 model, use the Maven 3 compatible one....
This piece can be easily detected very early at boot time and could be used to configure the resolver.

Related to MNG-7984.

@rmannibucau
Copy link
Contributor

What I worry about it is the fact we are moving the current issues a step further but I don't understand yet how it does fix anything, this kind of configuration looks per dependency, so while I fully agree we can make it global or per version, it will keep blowing up as soon as you have > 2 dep there no?

Don't get me wrong, I think the transitive feature is rather a good thing if we can instantiate it without more side effects than we have today but alone it looks like mixing behaviors which all have pitfalls and therefore just make it more complex.
Ultimately it can mean we become stricter on the depMgt and forbid not resolved conflicts (a bit like what enforcer plugin enables), this way the resolution of a built project (not transitive deps alone) is always deterministic at a little user cost.

Still thinking out loud to try to get this feature onboard without realizing in some months we are back where we were in terms of issues with new ones due to the behavior change and "resolution merge" logic we would need from consumed poms.

@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

This setting cannot be per dependency, at least resolver does not support that. The depMgr is set per session. And what current PR changes is really only this: are depMgt sections obeyed only 2 level deep or to the end of the pit. The "2 level depth" comes from Maven2 times, and was artificial limit just to make Maven3 (resolver in it) work like Maven2 did. Unsure why Maven4 would need to work in this respect as Maven2 did.

@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

And as regarding "blowing", lets see what ITs has to say. Also we could maybe add IT for https://issues.apache.org/jira/browse/MRESOLVER-235

@rmannibucau
Copy link
Contributor

@cstamas I understand that and ultimately I hope we reach your proposal I do like, but I don't want to be at the cost of making builds more complex and user most lost, this is how I end on the "strict" mode for depMgt in maven 4 but I'm not 100% happy with that so any idea welcomed ;).

@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

IMO, this change -- if goes in -- will just make POMs simpler and not more complex. Essentially what this makes possible is that lib author (whos lib is used somewhere deep in your project) pass information to your project, instead to have those information ignored. Hence, your POM will need less and not more depMgt entries, that you had to add before to make up the missing/lost information.

@rmannibucau
Copy link
Contributor

@cstamas right, now consider 2 lib authors for your project (3rd maven build to make it clear) and also maven 3 consumers (let's ignore 2, agree), it becomes a mess for us :(

@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

I see no difference to what happens if you use two libs that declare different version on some artifact, really. And depMgt (unlike BOM imports) let it all to you to sort it out in "maven way".

@cstamas
Copy link
Member Author

cstamas commented Dec 20, 2023

ITs look good.

And thinking more about this, this should be maybe just a boolean flag? classic($transitive)? As I would leave out completely the "transitive" and the "default" as they do introduce unwanted behaviour for sure (fail ITs), rendering existing healthy build broken.

It is really just about transitivity. The other two depMgrs are
too "invasive".
@cstamas cstamas changed the title [MNG-7982] Make classicTransitive the default depMgr [MNG-7982] Switch to enable transitivity in depMgr used by Maven Dec 20, 2023
@rmannibucau
Copy link
Contributor

Hmm, still thinking a bit out loud but at some point it was discussed that the consumed pom would be "cleaned for central" (flatten maven plugin like etc) to ensure the consumers see it as the producer in terms of resolution, whatever version it uses, is it still in the scope? If so depMgt is maybe no more a thing transitively and then we just do what we want for maven 4, wdyt?

@gnodet
Copy link
Contributor

gnodet commented Dec 21, 2023

Hmm, still thinking a bit out loud but at some point it was discussed that the consumed pom would be "cleaned for central" (flatten maven plugin like etc) to ensure the consumers see it as the producer in terms of resolution, whatever version it uses, is it still in the scope? If so depMgt is maybe no more a thing transitively and then we just do what we want for maven 4, wdyt?

Keeping deployed artifacts consumable by maven 3 is still the plan to day, and yes, this kinda imply we can't really bring-in things that would require a change at consumption time in maven 3. However, in Maven 4, the 4.1.0 model has a flag preserveModelVersion which can be used to force the usage of 4.1.0 model at deployment time. This could be used to force the trigger of the new behaviour on the consumer side, at the cost of requiring maven 4.0.0.

However, I have no idea what the impact is if one builds a project with the transitive dependency manager, but uses it with the classic one in Maven 3. Could that break the consumer side ?

@cstamas
Copy link
Member Author

cstamas commented Jan 8, 2024

However, I have no idea what the impact is if one builds a project with the transitive dependency manager, but uses it with the classic one in Maven 3. Could that break the consumer side ?

I don't quite understand: so same project that is once built with Maven 4 and once with Maven 3? Or an artifact that is dependency once in Maven4 built project and once in Maven3 project? Or WDYM?

pom.xml Outdated Show resolved Hide resolved
@cstamas
Copy link
Member Author

cstamas commented Jan 11, 2024

Yes, we have separate issue for that #1373

@cstamas cstamas marked this pull request as ready for review January 11, 2024 10:45
@gnodet
Copy link
Contributor

gnodet commented Jan 11, 2024

However, I have no idea what the impact is if one builds a project with the transitive dependency manager, but uses it with the classic one in Maven 3. Could that break the consumer side ?

I don't quite understand: so same project that is once built with Maven 4 and once with Maven 3? Or an artifact that is dependency once in Maven4 built project and once in Maven3 project? Or WDYM?

I mean you build the project with maven 4, upload to central, and a maven 3 projects depends on it.

@gnodet gnodet self-requested a review January 11, 2024 12:33
@cstamas
Copy link
Member Author

cstamas commented Jan 11, 2024

I mean you build the project with maven 4, upload to central, and a maven 3 projects depends on it.

Yes, in that case they may be differences. BUT, since we talk about two distinct projects: your, that is built with mvn4, and theirs (consumer) built with mvn3. So they will anyway have more dependencies, not just your lib, and they will anyway have to handle their transitive hull, hence maybe will need to have more entries in their depMgt to make things work out for them. Just like things worked so far, no change here.

In general, I don't see this as an issue at all, here is why: consumer either suffered from issue like https://issues.apache.org/jira/browse/MNG-7003 and they had it fixed, or they do not suffer from this issue.

I see this just like making maven "more correct", "more convergent", am pretty much sure we do not break anything with this.

@cstamas cstamas merged commit 5261ba9 into apache:master Jan 11, 2024
18 checks passed
@cstamas cstamas deleted the MNG-7982 branch January 11, 2024 16:55
@DidierLoiseau
Copy link
Contributor

DidierLoiseau commented Oct 5, 2024

There is a strange quirk with the ClassicDependencyManager used in this way, due to these 3 lines, it will ignore the dependency management just for the first layer of dependencies, which makes it quite inconsistent. More details in my comment on MNG-5761 with a reproducible example.

Shouldn’t the DefaultDependencyManager or the TransitiveDependencyManager have been used instead? (The only difference between the two is the value of applyFrom, and the latter is actually the same as the classic but without those 3 lines)

It feels like the integration test from MNG-4720 is somewhat incomplete, but maybe it shouldn’t apply any more after this change?

@DidierLoiseau
Copy link
Contributor

FYI @cstamas and @gnodet, I have provided integration tests for this in apache/maven-integration-testing#379, which show the current inconsistency.

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

Successfully merging this pull request may close these issues.

4 participants