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-7974] Update Resolver 2.0.0-alpha-5 #1337

Merged
merged 25 commits into from
Dec 18, 2023

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Dec 7, 2023

Update to Resolver alpha-5 and apply some cleanups.

Notable changes:

  • update resolver to 2.0.0-alpha-5
  • detach model-builder from maven-artifact (this is important)
  • introduce model builder own VersionParser iface (implemented in resolver-provider)
  • API VersionParser implementation reuses VersionParser from resolver-provider to implement the service
  • various other cleanups, removal of old plexus, etc

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

(to be in alpha-4)

In short, this opens the gate to use different version schemes
per Maven session.

---

https://issues.apache.org/jira/browse/MNG-7950
https://issues.apache.org/jira/browse/MNG-7951
@cstamas cstamas self-assigned this Dec 7, 2023
@cstamas cstamas added this to the 4.0.0-alpha-10 milestone Dec 8, 2023
@cstamas cstamas marked this pull request as ready for review December 14, 2023 19:43
@gnodet
Copy link
Contributor

gnodet commented Dec 15, 2023

What about all the usages of org.apache.maven.artifact.versioning.DefaultArtifactVersion ?
This is currently the one used in the Maven 4 api and also in areas such as parent selection in model building.

@cstamas
Copy link
Member Author

cstamas commented Dec 15, 2023

@gnodet see da737af

@cstamas cstamas changed the title [MNG-7950][MNG-7951] Use VersionSchemeSelector from Resolver 2.0 [MNG-7950][MNG-7951] Allow use of custom VersionScheme Dec 15, 2023
@gnodet
Copy link
Contributor

gnodet commented Dec 15, 2023

I'm not sure I'm buying the use of the additional indirection for the VersionSchemeSelector. I wonder if it would be just as simple to be injected with a VersionScheme. If users want to use a different scheme, they just need to register one with a higher @Priority. And if we want pluggability, we could also define a @SessionSCoped component that would delegate to the VersionSchemeSelector underneath.
But at least, components would simply be injected with a single VersionScheme.

@cstamas
Copy link
Member Author

cstamas commented Dec 15, 2023

The goal of this PR is to allow transparently per-session choice of VersionScheme. The "transparently" means that even the constructed object graph can be retained, as all the selection happens at session runtime, hence this would work also in mvnd without any extra effort as well. Finally, yes, this opens ability for users to really shoot themselves in foot (which maven config does not do that so far?), but I think the benefit of this -- ie. to have some custom version scheme with project specific (fixed) version scheme outweighs that.

Also, don't forget that this allows now for a Mojo to customize session (and configure version scheme for use case) and then perform different queries against resolver...

VersionScheme is a resolver interface, and while Resolver does not instantiate it anywhere directly, it is always client code that needs to set it (ie. see util filters etc), but as this PR shows, there are some resolver component implementations (am looking at maven-resolver-provider) that use it. Hence, this PR makes this feature available for Maven but also for any Resolver 3rd party integration as well.

Reason why would you use different VersionScheme is usually due comparisons between versions, ordering. And remember, Maven does not do such thing, as plugins in POM, dependencies are usually "plain versions" where simple string equality (or translation via layout and on retrieval equality) is applied, are mostly unaffected by scheme. Where scheme comes into play is version sorting, range handling and such...

Finally, don't forget that currently regarding versions there are two "parallel universes" (both with own bugs), the maven-artifact Version and resolver Generic version scheme. This could also open a gate to drop one of them, and have two implementations of Scheme like "generic" and "legacy", that implements old one with all of it's bugs, hence users could choose "modus operandi", "current" or "legacy".

@cstamas
Copy link
Member Author

cstamas commented Dec 16, 2023

Reworked PR: stepped off selector, it was reverted (see https://issues.apache.org/jira/browse/MRESOLVER-457 and read why it was bad idea). All simpler now.

@cstamas cstamas changed the title [MNG-7950][MNG-7951] Allow use of custom VersionScheme [MNG-7951] Make all component accept Provider<VersionScheme> Dec 16, 2023
@cstamas cstamas changed the title [MNG-7951] Make all component accept Provider<VersionScheme> [MNG-7951][MNG-7974] Update Resolver 2.0.0-alpha-5 and make all component accept version scheme provider Dec 17, 2023
@cstamas
Copy link
Member Author

cstamas commented Dec 18, 2023

I agree with 49cec29 commit: if component changed to session scope, no need for provider.
But I don't understand 2nd commit 5ce5279 that left components singletons, but you removed the provider.

Can you explain that? As your 2nd commit makes mandatory to rebuild component graph in case of schema change, while with provider it was possible without doing that.

@gnodet
Copy link
Contributor

gnodet commented Dec 18, 2023

I agree with 49cec29 commit: if component changed to session scope, no need for provider. But I don't understand 2nd commit 5ce5279 that left components singletons, but you removed the provider.

Can you explain that? As your 2nd commit makes mandatory to rebuild component graph in case of schema change, while with provider it was possible without doing that.

So what kind of schema change do you have in mind exactly ?

I'm not buying the idea to use Provider at the injection point to control the lifecycle of dependent beans. If we suppose a plugin can register any kind of component and that it should override the default component set, then all injection points have to use Provider. Such components are supposed to be provided by extensions, which do not change. Fwiw, I think we do overuse the @Singleton and most components are defined as singletons but this is unnecessary because they are stateless. @Singleton should be used for classes that actually need to be singletons.

@cstamas
Copy link
Member Author

cstamas commented Dec 18, 2023

Ok, let's postpone this discussion, am completely fine if this PR merely upgrades Resolver to alpha-5.

My ultimate goal is to kill off "parallel universe" (two version implementations, resolver and maven), but for sure this PR is not fixing that. And agree with overuse of singletons as well.

Let's move on with this if you are fine with it.

@cstamas cstamas changed the title [MNG-7951][MNG-7974] Update Resolver 2.0.0-alpha-5 and make all component accept version scheme provider [MNG-7974] Update Resolver 2.0.0-alpha-5 Dec 18, 2023
Not needed anymore
@cstamas
Copy link
Member Author

cstamas commented Dec 18, 2023

@gnodet this PR became "just" update to resolver-5 w/ cleanup. So please review it as such.

@cstamas cstamas merged commit a1fdd89 into apache:master Dec 18, 2023
17 of 18 checks passed
@cstamas cstamas deleted the version-scheme-selector branch December 18, 2023 11:03
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.

3 participants