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/12/13 15:18:24 UTC
[GitHub] [maven] slawekjaranowski opened a new pull request, #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
slawekjaranowski opened a new pull request, #909:
URL: https://github.com/apache/maven/pull/909
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) 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.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MNG-XXX] SUMMARY`, where you replace `MNG-XXX`
and `SUMMARY` 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.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [x] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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 closed pull request #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
URL: https://github.com/apache/maven/pull/909
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1047350107
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
I think it should be the same than the method above:
```new MavenSettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), recessiveSourceLevel)``` instead.
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1048656875
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
The only non immutable bit in the m4 `Settings` is the `sourceLevel` which I also have no idea where/why/how/if it's used.
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1047360970
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
I described in issue why I use this methods.
`MavenSettingsMerger().merge()` will not work with m-invoker-p because change state of `recessive` - which is reused ...
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #909:
URL: https://github.com/apache/maven/pull/909#issuecomment-1348858555
Also related to https://github.com/apache/maven/pull/874, which contains a similar fix.
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1048744963
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
> > The only non immutable bit in the m4 `Settings` is the `sourceLevel` which I also have no idea where/why/how/if it's used.
>
> Exactly, and `sourceLevel` can be set once - it is a reason why I used `SettingsMerger`.
Ah, I see.
> m-invoker-p has a hack for it: https://github.com/apache/maven-invoker-plugin/blob/bfb75f9e52e93478dab710bb7243978c06f48d1a/src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java#L1580-L1618
> but only works with Maven3 - 4 has session in delegate field
Ok, I think that's not a real issue, right ? As exception is caught and forgot. But this would allow to get rid of this code in the future.
> We also can relax of settings `sourceLevel`.
I would be in favour of getting rid of it in the v4 api if it's not used, and worse, actually worked around.
> By the way we have double implementation `MavenSettingsMerger` and `SettingsMerger` IMHO we should use one.
I wasn't sure if the results would be the same, given the first one is hand-crafted and the second one generated.
If that's the case, the easier would be to deprecate `MavenSettingsMerger` and have it delegated to `SettingsMerger`. The `MavenSettingsMerger` is also the one used by default in the `DefaultSettingsBuilder`, but we can try and see if the ITs raise any issues.
Also, if the goal is to restore compatibility, maybe a cleaner way would be similar to https://github.com/apache/maven/pull/874, i.e. restore the methods as they were in m3, but delegating to a new v4 similar class. Both PRs are conflicting but #874 also has additional value and should have been merged earlier imho.
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1048712224
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
> The only non immutable bit in the m4 `Settings` is the `sourceLevel` which I also have no idea where/why/how/if it's used.
Exactly, and `sourceLevel` can be set once - it is a reason why I used `SettingsMerger`.
m-invoker-p has a hack for it:
https://github.com/apache/maven-invoker-plugin/blob/bfb75f9e52e93478dab710bb7243978c06f48d1a/src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java#L1580-L1618
but only works with Maven3 - 4 has session in delegate field
We also can relax of settings `sourceLevel`.
By the way we have double implementation `MavenSettingsMerger` and `SettingsMerger` IMHO we should use one.
--
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 #909: [MNG-7625] Restore compatibility with Maven 3 - SettingsUtils#merge
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1048812974
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive, recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(), recessive.getDelegate(), true, null);
Review Comment:
We can suppres this PR - and work on #874
Requirements that `sourceLevel` can be set once still will be a problem, event if in invoker method which override this filed by reflection can pass by ignore exception.
Second call on `merge` method will throw exception because of set `sourceLevel` again.
--
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