You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/11/16 10:57:09 UTC
[GitHub] [maven] kwin opened a new pull request, #874: [MNG-7598] Enforce binary backwards-compatibility
kwin opened a new pull request, #874:
URL: https://github.com/apache/maven/pull/874
For now only enabled in core and plugin-api
Currently breaks build due to incompatible changes compared to 3.8.6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #874:
URL: https://github.com/apache/maven/pull/874#issuecomment-1321706362
I think it needs to be enabled on not only maven-core. It's missing stuff like https://github.com/apache/maven/pull/841/files#diff-a660bdaa2e580478c65a557fd7001c80e129c0e9d4766840b4072c3b72c1863f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] kwin commented on pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #874:
URL: https://github.com/apache/maven/pull/874#issuecomment-1317193067
@gnodet I fixed most of the issues in Maven Core. Can you look at the others and either fix or ignore with a comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] kwin merged pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
kwin merged PR #874:
URL: https://github.com/apache/maven/pull/874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #874:
URL: https://github.com/apache/maven/pull/874#discussion_r1049352600
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,9 +37,9 @@ private SettingsUtils()
* @param recessive
* @param recessiveSourceLevel
*/
- public static Settings merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
+ public static void merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
{
- return new MavenSettingsMerger().merge( dominant, recessive, recessiveSourceLevel );
+ SettingsUtilsV4.merge( dominant.getDelegate(), recessive.getDelegate(), recessiveSourceLevel );
Review Comment:
The method now reads:
```
public static void merge(Settings dominant, Settings recessive, String recessiveSourceLevel) {
if (dominant != null && recessive != null) {
dominant.delegate = SettingsUtilsV4.merge(dominant.getDelegate(), recessive.getDelegate());
}
}
```
As for the source level, I think it would be better to leverage the InputLocationTracker in the model, which is way more powerful, for both settings and toolchains. And get rid of the TrackableBase in the v4 models. But this is an enhancement for another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] slawekjaranowski commented on a diff in pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #874:
URL: https://github.com/apache/maven/pull/874#discussion_r1048805974
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,9 +37,9 @@ private SettingsUtils()
* @param recessive
* @param recessiveSourceLevel
*/
- public static Settings merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
+ public static void merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
{
- return new MavenSettingsMerger().merge( dominant, recessive, recessiveSourceLevel );
+ SettingsUtilsV4.merge( dominant.getDelegate(), recessive.getDelegate(), recessiveSourceLevel );
Review Comment:
`dominant` and `recessive` can be null in v3.
caller expect that `dominant` will have a result of merge , so we need
```
dominant.delegate = SettingsUtilsV4.merge( ...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] kwin commented on pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #874:
URL: https://github.com/apache/maven/pull/874#issuecomment-1321722446
This is just a start for sure to allow us to incrementally merge. Let us first fix Core and Plugin API and then follow-up with subsequent PRs for the other modules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] kwin commented on pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #874:
URL: https://github.com/apache/maven/pull/874#issuecomment-1337080224
@gnodet Any objection to merge this (after fixing the conflicts)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #874: [MNG-7598] Enforce binary backwards-compatibility
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #874:
URL: https://github.com/apache/maven/pull/874#discussion_r1049352600
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,9 +37,9 @@ private SettingsUtils()
* @param recessive
* @param recessiveSourceLevel
*/
- public static Settings merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
+ public static void merge( Settings dominant, Settings recessive, String recessiveSourceLevel )
{
- return new MavenSettingsMerger().merge( dominant, recessive, recessiveSourceLevel );
+ SettingsUtilsV4.merge( dominant.getDelegate(), recessive.getDelegate(), recessiveSourceLevel );
Review Comment:
The method now reads:
```
public static void merge(Settings dominant, Settings recessive, String recessiveSourceLevel) {
if (dominant != null && recessive != null) {
dominant.delegate = SettingsUtilsV4.merge(dominant.getDelegate(), recessive.getDelegate());
}
}
```
As for the source level, I think it would be better to leverage the InputLocationTracker in the model, which is way more powerful, for both settings and toolchains. And get rid of the TrackableBase in the v4 models.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org