You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/11/29 07:23:31 UTC
[GitHub] [flink] alpinegizmo commented on a diff in pull request #21349: Support Maven 3.3+
alpinegizmo commented on code in PR #21349:
URL: https://github.com/apache/flink/pull/21349#discussion_r1034376565
##########
tools/ci/flink-ci-tools/src/main/java/org/apache/flink/tools/ci/optional/ShadeOptionalChecker.java:
##########
@@ -65,9 +65,23 @@ public static void main(String[] args) throws IOException {
if (!violations.isEmpty()) {
LOG.error(
- "{} modules bundle in total {} dependencies without them being marked as optional in the pom:",
+ "{} modules bundle in total {} dependencies without them being marked as optional in the pom.",
violations.keySet().size(),
violations.size());
+ LOG.error(
+ "\tIn order for shading to properly work within Flink we require all bundled dependencies to be marked as optional in the pom.");
+ LOG.error(
+ "\tFor verification purposes we require the dependency tree from the dependency-plugin to show the dependency as either:");
+ LOG.error("\t\ta) an optional dependency,");
+ LOG.error("\t\tb) a transitive dependency of another optional dependency.");
+ LOG.error(
+ "\tIn most cases adding '<optional>${flink.markBundledAsOptional}</optional>' to the the bundled dependency is sufficient.");
Review Comment:
```suggestion
"\tIn most cases adding '<optional>${flink.markBundledAsOptional}</optional>' to the bundled dependency is sufficient.");
```
##########
tools/ci/flink-ci-tools/src/main/java/org/apache/flink/tools/ci/optional/ShadeOptionalChecker.java:
##########
@@ -65,9 +65,23 @@ public static void main(String[] args) throws IOException {
if (!violations.isEmpty()) {
LOG.error(
- "{} modules bundle in total {} dependencies without them being marked as optional in the pom:",
+ "{} modules bundle in total {} dependencies without them being marked as optional in the pom.",
violations.keySet().size(),
violations.size());
+ LOG.error(
+ "\tIn order for shading to properly work within Flink we require all bundled dependencies to be marked as optional in the pom.");
+ LOG.error(
+ "\tFor verification purposes we require the dependency tree from the dependency-plugin to show the dependency as either:");
+ LOG.error("\t\ta) an optional dependency,");
+ LOG.error("\t\tb) a transitive dependency of another optional dependency.");
+ LOG.error(
+ "\tIn most cases adding '<optional>${flink.markBundledAsOptional}</optional>' to the the bundled dependency is sufficient.");
+ LOG.error(
+ "\tThere are some edge-cases where a transitive dependency might with be associated with the \"wrong\" dependency in the tree, for example if a test dependency also requires it.");
Review Comment:
```suggestion
"\tThere are some edge cases where a transitive dependency might be associated with the \"wrong\" dependency in the tree, for example if a test dependency also requires it.");
```
##########
tools/ci/flink-ci-tools/src/main/java/org/apache/flink/tools/ci/optional/ShadeOptionalChecker.java:
##########
@@ -65,9 +65,23 @@ public static void main(String[] args) throws IOException {
if (!violations.isEmpty()) {
LOG.error(
- "{} modules bundle in total {} dependencies without them being marked as optional in the pom:",
+ "{} modules bundle in total {} dependencies without them being marked as optional in the pom.",
violations.keySet().size(),
violations.size());
+ LOG.error(
+ "\tIn order for shading to properly work within Flink we require all bundled dependencies to be marked as optional in the pom.");
+ LOG.error(
+ "\tFor verification purposes we require the dependency tree from the dependency-plugin to show the dependency as either:");
+ LOG.error("\t\ta) an optional dependency,");
+ LOG.error("\t\tb) a transitive dependency of another optional dependency.");
+ LOG.error(
+ "\tIn most cases adding '<optional>${flink.markBundledAsOptional}</optional>' to the the bundled dependency is sufficient.");
+ LOG.error(
+ "\tThere are some edge-cases where a transitive dependency might with be associated with the \"wrong\" dependency in the tree, for example if a test dependency also requires it.");
+ LOG.error(
+ "\tIn such cases you need to adjust to poms such that the dependency shows up in the right spot; by it adding an explicit dependency(Management) entry, excluding dependencies or at times even reordering dependencies in the pom.");
Review Comment:
```suggestion
"\tIn such cases you need to adjust the poms so that the dependency shows up in the right spot. This may require adding an explicit dependency (Management) entry, excluding dependencies, or at times even reordering dependencies in the pom.");
```
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org