[MNG-7129] - Changes in maven core required to support incremental mave…

[AlexanderAshitkin]

Changes in maven core to inject caching extension

CC @ansip

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.

[gnodet]

This looks roughly similar to 0a6f1d3
The https://github.com/apache/maven/tree/MNG-7129-master already contains much more so if the goal is to merge into maven 4 only, I'd suggest looking at this branch instead.

[maximilian-novikov-db]

This looks roughly similar to 0a6f1d3 The https://github.com/apache/maven/tree/MNG-7129-master already contains much more so if the goal is to merge into maven 4 only, I'd suggest looking at this branch instead.

@gnodet our plan is the following:

1. merge the minimal changes to 3.x(@michael-o to 3.9 ?) and 4, as it's a very localized change it it will provide better supportability if the caching module will be moved to an extension
2. create new branch from MNG-7129-master and apply changes which we have done in our draft extension version (bug fixes, support to change projects versions like it is done by maven set plugin)
3. raise PR from our branch to MNG-7129-master

[michael-o]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

[andrey-petrenko-db]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement)
and now this PR reworked and there is used java 8 and maven 4 features

[michael-o]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

[andrey-petrenko-db]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

Ok but is there a separate branch for 3.9
Or 3.9 is planned to release from the master hence 3.9 equals 4.0?

[andrey-petrenko-db]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

Ok but is there a separate branch for 3.9 Or 3.9 is planned to release from the master hence 3.9 equals 4.0?

Just to clarify: we don't have a problem with a version itself. It does not matter 3.9 or 4.0. The problem with compatibility.
If we build maven from master with 4.0-SNAPSHOT and try to build our project then our build stuck. (Max checked this, however he did this long time ago)

• Currently we have 3.6 used for our prod
• We tried 3.8 - it works
• built snapshot from master and tried to build our project - fail, so it seems it requires migration to a new maven version - this is what we want to prevent to not block extension development till maven 4 stable version released

[michael-o]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

Ok but is there a separate branch for 3.9 Or 3.9 is planned to release from the master hence 3.9 equals 4.0?

Not yet. There is this which is the tentative 3.9.x branch. I will branch off a couple of weeks after 3.8.4 release. Something around 2021-12-10 or so.

[gnodet]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

Ok but is there a separate branch for 3.9 Or 3.9 is planned to release from the master hence 3.9 equals 4.0?

Not yet. There is this which is the tentative 3.9.x branch. I will branch off a couple of weeks after 3.8.4 release. Something around 2021-12-10 or so.

My understanding was that the goal was to focus on 4.0.x. The work has already stalled a bit and I'm not sure creating additional branches will help in any way.

[gnodet]

@maximilian-novikov-db Since you are using Java 8 features here, it cannot be merged to 3.8.x for sure. I need to go through these changes and try to understand them.

@michael-o We can raise PR to 3.8 with similar changes, initial changes here were made against 3.8 branch (Used java 1.7 features only, used annotations from plexus: org.codehaus.plexus.component.annotations.Component, org.codehaus.plexus.component.annotations.Requirement) and now this PR reworked and there is used java 8 and maven 4 features

Let's stick with Java 8 and 3.9. The code should not diverge. This will make life for us and you easier.

Ok but is there a separate branch for 3.9 Or 3.9 is planned to release from the master hence 3.9 equals 4.0?

Just to clarify: we don't have a problem with a version itself. It does not matter 3.9 or 4.0. The problem with compatibility. If we build maven from master with 4.0-SNAPSHOT and try to build our project then our build stuck. (Max checked this, however he did this long time ago)

• Currently we have 3.6 used for our prod
• We tried 3.8 - it works
• built snapshot from master and tried to build our project - fail, so it seems it requires migration to a new maven version - this is what we want to prevent to not block extension development till maven 4 stable version released

It would be nice to have reports of things that fail so that we can actually fix those.

[gnodet]

This looks roughly similar to 0a6f1d3 The https://github.com/apache/maven/tree/MNG-7129-master already contains much more so if the goal is to merge into maven 4 only, I'd suggest looking at this branch instead.

@gnodet our plan is the following:

1. merge the minimal changes to 3.x(@michael-o to 3.9 ?) and 4, as it's a very localized change it it will provide better supportability if the caching module will be moved to an extension

Yes, it's similar to the commit I pointed at but done in a different way (i.e. introducing a MojoExecutorStrategy interface instead of a IMojoExecutor one).

2. create new branch from MNG-7129-master and apply changes which we have done in our draft extension version (bug fixes, support to change projects versions like it is done by maven set plugin)
3. raise PR from our branch to MNG-7129-master

I guess you're referring to https://github.com/apache/maven/tree/MNG-7129-master in your two links ... ?

[andrey-petrenko-db]

Yes, it's similar to the commit I pointed at but done in a different way (i.e. introducing a MojoExecutorStrategy interface instead of a IMojoExecutor one).

not quite
in your PR there is no low coupling, no single extension point, you moved MojoExecutor to interface yes, but with this interface you transitively moved to public API ProjectIndex, PhaseRecorder, DependencyContext - do we really need them in public API, it is possible to keep consistency between all of them in future?
Our idea was to make change in core as small as possible to prevent breaking changes in future
Also because caching logic should not override all logic in default executor there is a code duplication in MojoExecutor and CachingMojoExecutor
Of course I think you better know how to implement this correctly than me
But when we started to think about how to move caching logic to extension we made similar changes that you have done in your PR and found out these drawbacks which I described above

All of the above is my opinion of course

I guess you're referring to https://github.com/apache/maven/tree/MNG-7129-master in your two links ... ?

yes

[gnodet]

Yes, it's similar to the commit I pointed at but done in a different way (i.e. introducing a MojoExecutorStrategy interface instead of a IMojoExecutor one).

not quite in your PR there is no low coupling, no single extension point, you moved MojoExecutor to interface yes, but with this interface you transitively moved to public API ProjectIndex, PhaseRecorder, DependencyContext - do we really need them in public API, it is possible to keep consistency between all of them in future? Our idea was to make change in core as small as possible to prevent breaking changes in future Also because caching logic should not override all logic in default executor there is a code duplication in MojoExecutor and CachingMojoExecutor Of course I think you better know how to implement this correctly than me But when we started to think about how to move caching logic to extension we made similar changes that you have done in your PR and found out these drawbacks which I described above

Fair point, I think you're right that your proposal requires a lower coupling and thus is better.

All of the above is my opinion of course

I guess you're referring to https://github.com/apache/maven/tree/MNG-7129-master in your two links ... ?

yes

It would be nice to have a first PR to merge this change into https://github.com/apache/maven/tree/MNG-7129-master, so that we'll have a better understanding on how this new interface will be used.

[andrey-petrenko-db]

Yes, it's similar to the commit I pointed at but done in a different way (i.e. introducing a MojoExecutorStrategy interface instead of a IMojoExecutor one).

not quite in your PR there is no low coupling, no single extension point, you moved MojoExecutor to interface yes, but with this interface you transitively moved to public API ProjectIndex, PhaseRecorder, DependencyContext - do we really need them in public API, it is possible to keep consistency between all of them in future? Our idea was to make change in core as small as possible to prevent breaking changes in future Also because caching logic should not override all logic in default executor there is a code duplication in MojoExecutor and CachingMojoExecutor Of course I think you better know how to implement this correctly than me But when we started to think about how to move caching logic to extension we made similar changes that you have done in your PR and found out these drawbacks which I described above

Fair point, I think you're right that your proposal requires a lower coupling and thus is better.

All of the above is my opinion of course

I guess you're referring to https://github.com/apache/maven/tree/MNG-7129-master in your two links ... ?

yes

It would be nice to have a first PR to merge this change into https://github.com/apache/maven/tree/MNG-7129-master, so that we'll have a better understanding on how this new interface will be used.

I see two options:

1. merge your PR to master then we make branch from master and reapply all changes we have done, raise PR
2. create branch from your branch reapply all changes and raise PR to it (not master) hence you can see difference

From my point of view changes from your branch should be taken anyway so the order of merge does not make much sense
@maximilian-novikov-db , @AlexanderAshitkin what do you think?

[gnodet]

Yes, it's similar to the commit I pointed at but done in a different way (i.e. introducing a MojoExecutorStrategy interface instead of a IMojoExecutor one).

not quite in your PR there is no low coupling, no single extension point, you moved MojoExecutor to interface yes, but with this interface you transitively moved to public API ProjectIndex, PhaseRecorder, DependencyContext - do we really need them in public API, it is possible to keep consistency between all of them in future? Our idea was to make change in core as small as possible to prevent breaking changes in future Also because caching logic should not override all logic in default executor there is a code duplication in MojoExecutor and CachingMojoExecutor Of course I think you better know how to implement this correctly than me But when we started to think about how to move caching logic to extension we made similar changes that you have done in your PR and found out these drawbacks which I described above

Fair point, I think you're right that your proposal requires a lower coupling and thus is better.

All of the above is my opinion of course

I guess you're referring to https://github.com/apache/maven/tree/MNG-7129-master in your two links ... ?

yes

It would be nice to have a first PR to merge this change into https://github.com/apache/maven/tree/MNG-7129-master, so that we'll have a better understanding on how this new interface will be used.

I see two options:

1. merge your PR to master then we make branch from master and reapply all changes we have done, raise PR
2. create branch from your branch reapply all changes and raise PR to it (not master) hence you can see difference

From my point of view changes from your branch should be taken anyway so the order of merge does not make much sense @maximilian-novikov-db , @AlexanderAshitkin what do you think?

I'd rather squash and merge into master the whole feature at once when it's ready, and let others review the full set of changes, so I'd rather go for #2.

[AlexanderAshitkin]

From my point of view changes from your branch should be taken anyway so the order of merge does not make much sense @maximilian-novikov-db , @AlexanderAshitkin what do you think?

I'd rather squash and merge into master the whole feature at once when it's ready, and let others review the full set of changes, so I'd rather go for #2.

Hi. My understanding from previous communication with Herve and Robert that minimal isolated change in master will be better perceived than one massive drop. It could be done today and will be a step forward already. Having that merged the following work will be focused solely on extension and likely will be easier to change after decoupling from the maven-core. As i understand the concern is integration itself - is this change enough or not. The answer to that is yes, it is enough. Andrey has integrated the change already in upcoming PR, so he likely can create preview branch to demonstrate it if necessary

In short, i think we should merge it and move forward

Thank you!

[gnodet]

From my point of view changes from your branch should be taken anyway so the order of merge does not make much sense @maximilian-novikov-db , @AlexanderAshitkin what do you think?

I'd rather squash and merge into master the whole feature at once when it's ready, and let others review the full set of changes, so I'd rather go for #2.

Hi. My understanding from previous communication with Herve and Robert that minimal isolated change in master will be better perceived than one massive drop. It could be done today and will be a step forward already. Having that merged the following work will be focused solely on extension and likely will be easier to change after decoupling from the maven-core. As i understand the concern is integration itself - is this change enough or not. The answer to that is yes, it is enough. Andrey has integrated the change already in upcoming PR, so he likely can create preview branch to demonstrate it if necessary

In short, i think we should merge it and move forward

Thank you!

It's quite hard to actually review this PR without any real usage for it. As this is kind of a backport, I think we'd need the PR for the caching system in order to understand how those new interfaces will be used.

[jvanzyl]

If this is ultimately going to be an extension, which I agree it should, can one commit be of the interfaces created that will allow the incremental capabilities in the core, and create a separate repository outside of Maven core with the extension?

This is historically how extensions are made available: as separate artifacts. From past experience I can tell you that the implementation will change at a much higher rate than the core. I don't think it makes sense for an a new, experimental feature to be bound to the core where a change in said feature necessitates a release of the core.

[andrey-petrenko-db]

If this is ultimately going to be an extension, which I agree it should, can one commit be of the interfaces created that will allow the incremental capabilities in the core, and create a separate repository outside of Maven core with the extension?

This is historically how extensions are made available: as separate artifacts. From past experience I can tell you that the implementation will change at a much higher rate than the core. I don't think it makes sense for an a new, experimental feature to be bound to the core where a change in said feature necessitates a release of the core.

Actually that was our plan, small as possible changes in core and then raise separate PR to extension repository
As I understood there is a concern regarding this PR because it is not clear whether it is enough or we require more changes in core
So we can create separate PR to extension repository to demonstrate how this API is used

[gnodet]

If this is ultimately going to be an extension, which I agree it should, can one commit be of the interfaces created that will allow the incremental capabilities in the core, and create a separate repository outside of Maven core with the extension?
This is historically how extensions are made available: as separate artifacts. From past experience I can tell you that the implementation will change at a much higher rate than the core. I don't think it makes sense for an a new, experimental feature to be bound to the core where a change in said feature necessitates a release of the core.

Actually that was our plan, small as possible changes in core and then raise separate PR to extension repository As I understood there is a concern regarding this PR because it is not clear whether it is enough or we require more changes in core So we can create separate PR to extension repository to demonstrate how this API is used

I propose the following plan:

• create a similar PR than this one on top of the MNG-7129-master branch which will modify the code to have a cleaner separation between maven-core and maven-caching and eventually become a real extension
• merge the PR in MNG-7129-master
• merge this PR (and any additional changes in core needed) into master
• create a back port for 3.x (if there's a consensus)
• extract the maven-caching extension to its own repo and drop MNG-7129-master if there's a consensus, else merge MNG-7129-master into master

[gnodet]

I've merge this branch to MNG-7129-master. We can continue working on MNG-7129-master and when the state is satisfying, we can come back to this PR to merge to master and extract the module to its final destination, wherever it will be...

[gnodet]

I've changed this PR to a draft until we can get #607 in a more mature state.

[gnodet]

Superseded by #607