Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on PR #136: URL: https://github.com/apache/flink-shaded/pull/136#issuecomment-1916671742 Thanks for your feedback @zentol I checked locally with 1.2.7 flatten plugin and confirm that it resolves the issue So I bumped the issue and currently reverted previous changes regarding shading... May be one question which is still not clear to me (not directly related to perf regression) Could you please clarify: In confluence's creating flink-shaded release page it is mentioned that we need to use maven 3.2.5 to release. Playing locally I was not able to find a difference between the result of usage 3.2.5 and 3.8.6. Do we need still apply ShadeOptionalChecker's approach like in Flink main repo to safely use 3.8.6 during release? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1470407309 ## pom.xml: ## @@ -65,6 +65,7 @@ under the License. 4.1.100.Final 2.15.3 32.1.3-jre +true Review Comment: yep, sure FLINK-34269 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1469437739 ## pom.xml: ## @@ -65,6 +65,7 @@ under the License. 4.1.100.Final 2.15.3 32.1.3-jre +true Review Comment: fair point :+1: a follow-up Jira task is fine as well. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1468176508 ## pom.xml: ## @@ -65,6 +65,7 @@ under the License. 4.1.100.Final 2.15.3 32.1.3-jre +true Review Comment: that was the plan the only thing stopped me from making it here is that I was thinking about making ShadeOptionalChecker working which then could be backported to 18.x and then as a separate task sync maven-shade-plugin for all the modules Or do you think it worth making it here? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467969251
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
May be I was not clear enough, sorry
By saying
>it does not make sense to add it for depedencies inside dependencyManagement
I ment not in general. Just for the case here
Here there is already existing dependency on zookeeper for every zookeeper
module in dependencies which overrides the one from dependency management
(optional tag).
Netty here comes only as a dependency from zookeeper and once zookeeper is
marked optional maven started to think that netty is also becoming optional
I don't think this is the case for poms you've mentioned
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467969251
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
May be I was not clear enough, sorry
By saying
>it does not make sense to add it for depedencies inside dependencyManagement
I ment not in general.
Here there is already existing dependency on zookeeper for every zookeeper
module in dependencies which overrides the one from dependency management
(optional tag).
Netty here comes only as a dependency from zookeeper and once zookeeper is
marked optional maven started to think that netty is also becoming optional
I don't think this is the case for poms you've mentioned
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467771500
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
btw. the `exec-maven-plugin` allows to set System properties (see
[docs](https://www.mojohaus.org/exec-maven-plugin/usage.html#pom-configuration-1))
which could be used to enable profiles implicitly (see
[docs](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#property)).
:innocent:
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467765454
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
```
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467762581
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
may be we need to rename the profile itself... :thinking:
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467761466
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
In fact it is...
The reason is that version suffix of several modules like guava, zookeeper
is added only with this profile e.g. [1] and we need to know module name
together with version suffix while parsing output of `dependency:tree`
https://github.com/apache/flink-shaded/blob/b4f9ed2dc85b3dcbb4bfc3bdb6866505bfae6893/flink-shaded-guava-32/pom.xml#L52-L59
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467749847
##
pom.xml:
##
@@ -65,6 +65,7 @@ under the License.
4.1.100.Final
2.15.3
32.1.3-jre
+true
Review Comment:
I see, those were different issue. But that still leaves the question open
whether we want to enable the more recent shade plugin version for all modules.
:thinking:
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
Sorry for being not clear on that one. I didn't mean to move it from the
`Build` step into the `Check shade optional dependencies`. Instead, you could
add a new step `Create Dependency Tree`.
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
Can you elaborate a bit here? It doesn't make sense because dependencies
from the `` section would need to be explicitly added to
a module pom to make the dependency available and only then would we have to
add the `` tag?!
Looks like we have 4 locations in `flink-dist/pom.xml` and
`flink-filesystems/flink-s3-fs-presto/pom.xml` where this also applies.
:thinking:
##
.github/workflows/ci.yml:
##
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
--Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+-Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+run: |
+mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
```suggestion
mvn ${MVN_COMMON_OPTIONS} dependency:tree | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
```
The profile is not necessary for the `dependency:tree` IIUC.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467672006 ## pom.xml: ## @@ -341,21 +342,41 @@ under the License. java + + + org.apache.flink.tools.ci.licensecheck.LicenseChecker + + + +check-shade + +none + +java + + + + org.apache.flink.tools.ci.optional.ShadeOptionalChecker + - org.apache.flink.tools.ci.licensecheck.LicenseChecker true false org.apache.flink flink-ci-tools -1.17-SNAPSHOT +1.18-SNAPSHOT Review Comment: I think so. I guess it could also go with its own hotfix commit -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467669315 ## pom.xml: ## @@ -341,21 +342,41 @@ under the License. java + + + org.apache.flink.tools.ci.licensecheck.LicenseChecker + + + +check-shade + +none + +java + + + + org.apache.flink.tools.ci.optional.ShadeOptionalChecker + - org.apache.flink.tools.ci.licensecheck.LicenseChecker true false org.apache.flink flink-ci-tools -1.17-SNAPSHOT +1.18-SNAPSHOT Review Comment: I'm wondering whether we could go for the released version as well to have a clearer reproducibility in case of errors. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467507163
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
That's not about BOMs
in general it does not make sense to add it for depedencies inside
dependencyManagement
I removed it for others in dependencyManagement, thanks for noticing this
:+1:
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467507163
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
That's not about BOMs
in general it does not make sense to add it for depedencies inside
dependencyManagement
I removed it for others in dependencyManagement, thanks
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467476574
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
Review Comment:
moved `mvn dependency:tree` to check shade optional step
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on PR #136: URL: https://github.com/apache/flink-shaded/pull/136#issuecomment-1911714197 rebased since have to extract license related update into a separate commit -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467389710
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
Review Comment:
IIUC it is already in a separate step...
Or do you mean a separate job?
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467385245
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
Review Comment:
yep, i was thinking about that however had some doubts...
Since you've also mentioned this I think we should do that
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
Review Comment:
extracted into a separate commit
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466495286
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
frankly speaking I don't know
I will see whether it will work without this (together with Flink tests)
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466384524 ## pom.xml: ## @@ -65,6 +65,7 @@ under the License. 4.1.100.Final 2.15.3 32.1.3-jre +true Review Comment: If I understand things around [MNG-5899](https://issues.apache.org/jira/browse/MNG-5899) correctly in maven they stopped reuse dependency reduced pom produced by shade plugin however they didn't forbid manipulation on api level. At the same time on api level it is still possible to get some info e.g. list of dependencies from maven and then make some manipulation in code with this list of dependencies, this is how it was in maven-shade-plugin [1]. After [MSHADE-413](https://issues.apache.org/jira/browse/MSHADE-413) instead of manipulation on collection of objects received from maven they started to clone it first and then perform same manipulation on cloned version [1] https://github.com/apache/maven-shade-plugin/pull/124/files#diff-083ef87367e79a2ea6b333846149588f4ce284ed930db1d87712fdbf9111e381L1066-L1112 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466332128 ## pom.xml: ## @@ -341,21 +341,41 @@ under the License. java + + + org.apache.flink.tools.ci.licensecheck.LicenseChecker + + + +check-shade + +none + +java + + + + org.apache.flink.tools.ci.optional.ShadeOptionalChecker + - org.apache.flink.tools.ci.licensecheck.LicenseChecker true false org.apache.flink flink-ci-tools -1.17-SNAPSHOT +1.18-SNAPSHOT + +org.apache.maven.plugins Review Comment: that's the requirement to make `ShadeOptionalChecker` working previous versions of `maven-dependency-plugin` do not mark dependencies `optional` in output of `mvn dependency:tree` thus these lines align the version with version from Flink main repo -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466332128 ## pom.xml: ## @@ -341,21 +341,41 @@ under the License. java + + + org.apache.flink.tools.ci.licensecheck.LicenseChecker + + + +check-shade + +none + +java + + + + org.apache.flink.tools.ci.optional.ShadeOptionalChecker + - org.apache.flink.tools.ci.licensecheck.LicenseChecker true false org.apache.flink flink-ci-tools -1.17-SNAPSHOT +1.18-SNAPSHOT + +org.apache.maven.plugins Review Comment: that's the requirement to make `ShadeOptionalChecker` working previous versions of `maven-dependency-plugin` do not mark dependencies `optional` in output of `mvn dependency:tree` -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
XComp commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466310753
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
Review Comment:
nit: can we move it into its own step rather than having it included in the
Build step? ...to have a clearer separation and easier check what went wrong if
something went wrong
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
Review Comment:
That might deserve it's own hotfix commit
##
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##
@@ -128,6 +131,7 @@ under the License.
org.apache.zookeeper
zookeeper
${zookeeper.version}
+${flink.markBundledAsOptional}
io.netty
Review Comment:
we don't need that for BOMs?!
##
pom.xml:
##
@@ -65,6 +65,7 @@ under the License.
4.1.100.Final
2.15.3
32.1.3-jre
+true
Review Comment:
doesn't it make sense to upgrade the shade plugin to `3.4.1` here as well
instead of having it only being upgraded for the jackson dependencies?
my understanding is (according to [your comment in
FLINK-34148](https://issues.apache.org/jira/browse/FLINK-34148?focusedCommentId=17809796=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17809796))
that the shade plugin stopped editing the dependency tree in 3.4.1.
Maven 3.3+ didn't allow editing the dependency tree anymore (based on the
description in FLINK-28016). Shouldn't we have run into issues already earlier
because we upgraded Maven in CI to 3.8.2 even though we were still using an
older version of the shade plugin? It seems that I'm missing something here.
:thinking:
##
pom.xml:
##
@@ -341,21 +341,41 @@ under the License.
java
+
+
+
org.apache.flink.tools.ci.licensecheck.LicenseChecker
+
+
+
+check-shade
+
+none
+
+java
+
+
+
+
org.apache.flink.tools.ci.optional.ShadeOptionalChecker
+
-
org.apache.flink.tools.ci.licensecheck.LicenseChecker
true
false
org.apache.flink
flink-ci-tools
-1.17-SNAPSHOT
+1.18-SNAPSHOT
+
+org.apache.maven.plugins
Review Comment:
why is this needed?
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136: URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466289100 ## pom.xml: ## @@ -341,21 +342,41 @@ under the License. java + + + org.apache.flink.tools.ci.licensecheck.LicenseChecker + + + +check-shade + +none + +java + + + + org.apache.flink.tools.ci.optional.ShadeOptionalChecker + - org.apache.flink.tools.ci.licensecheck.LicenseChecker true false org.apache.flink flink-ci-tools -1.17-SNAPSHOT +1.18-SNAPSHOT Review Comment: `ShadeOptionalChecker` was introduced in 1.18.x -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466288220
##
flink-shaded-asm-9/pom.xml:
##
@@ -43,24 +43,28 @@ under the License.
org.ow2.asm
asm
${asm.version}
+${flink.markBundledAsOptional}
Review Comment:
This naming is used in error logs in cases there is an issue
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded [flink-shaded]
snuyanzin commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1466287619
##
.github/workflows/ci.yml:
##
@@ -49,8 +50,18 @@ jobs:
-Plicense-check \
| tee ${{ env.MVN_BUILD_OUTPUT_FILE }}
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
+
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
Review Comment:
align with naming in 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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
