zabetak commented on code in PR #5172:
URL: https://github.com/apache/hive/pull/5172#discussion_r1792012881
##########
standalone-metastore/metastore-server/pom.xml:
##########
@@ -720,6 +720,14 @@
<groupId>org.datanucleus</groupId>
<artifactId>datanucleus-maven-plugin</artifactId>
<version>5.2.1</version>
+ <!-- The plexus-contianer-default can be removed once we move to
datanucleus-maven-plugin version >= 6.0.0-release-->
+ <dependencies>
+ <dependency>
+ <groupId>org.codehaus.plexus</groupId>
+ <artifactId>plexus-container-default</artifactId>
+ <version>${plexus.version}</version>
+ </dependency>
+ </dependencies>
Review Comment:
The same argument that holds for the shade plugin and `log4j-core:2.1` also
applies here. The fact that datanucleus plugin has a dependency to log4j is not
very problematic since it is not a Hive product/module dependency. log4j will
only be used during the build and this usage is rather fine.
Nevertheless, if you are confident that upgrading the plexus `1.5.6` version
is fully compatible with `5.2.1` I am OK to make this change although I don't
find it necessary. I checked the enhanced classes before and after the change
and they seem identical so I am ok to leave this as is.
##########
pom.xml:
##########
@@ -1617,6 +1616,24 @@
<fail>true</fail>
</configuration>
</execution>
+ <execution>
+ <id>enforce-banned-transitive-dependencies-logging</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <bannedDependencies>
+ <excludes>
+ <exclude>log4j:log4j</exclude>
+ </excludes>
+ <searchTransitive>true</searchTransitive>
+ <message>A banned transitive logging dependency was
found!</message>
+ </bannedDependencies>
Review Comment:
This code snippet is already a rule so you don't need to copy-paste the
entire execution block. You can just put this snippet in the existing
`enforce-banned-dependencies-logging` execution. You could also use
https://maven.apache.org/enforcer/enforcer-rules/banTransitiveDependencies.html
rule to make it slightly less verbose.
##########
beeline/pom.xml:
##########
@@ -276,6 +276,11 @@
<artifactId>maven-shade-plugin.log4j2-cachefile-transformer</artifactId>
<version>2.1</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
Review Comment:
The `log4j-core:2.1` version is a dependency of
https://github.com/edwgiz/maven-shaded-log4j-transformer plugin. There seems to
be a 1-to-1 mapping between the plugin and the log4j-core version thus it
doesn't make sense to declare an explicit dependency to log4j-core. If we want
to avoid the usage of the old log4j version we should consider changing the
version of the transformer. Nevertheless this is out of the scope of the
current PR so let's track it under another JIRA if needed.
The `log4j-core:2.1` version is used by the shade plugin during the Hive
build but it is not a Hive dependency thus its not used by the product. Since
it is not used by the product it is not captured by the enforcer rule. I don't
know of a good way to ban plugin dependencies and not sure if it is necessary
or worth the effort investing in such tooling. If a plugin is deemed unsafe
then we should either ban the plugin usage or bump its version.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
