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 2021/05/19 12:29:11 UTC
[GitHub] [maven-shade-plugin] cstamas opened a new pull request #91: [MSHADE-389] Get rid of old baggage
cstamas opened a new pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91
https://issues.apache.org/jira/browse/MSHADE-389
Changes:
* get rid of Plexus Container et al (container, annotations, AbstractLogEnabled, etc)
* get rid of use of Guava (only Multimap was used from it, use commons-collections4 instead)
* up to Maven 3.1.x
* Note: UTs are still using PlexusTestCase as Maven 3.1.x has no SISU index
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r635603002
##########
File path: pom.xml
##########
@@ -244,21 +302,33 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <forceJavacCompilerUse>true</forceJavacCompilerUse>
Review comment:
removed as it is unneeded, but still, NPE in m-compiler-p?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r635554941
##########
File path: pom.xml
##########
@@ -151,6 +200,12 @@
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-dependency-tree</artifactId>
<version>3.0.1</version>
+ <exclusions>
Review comment:
remove these, mediate these to test scope
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r636311556
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -48,35 +49,49 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipException;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.commons.collections4.MultiValuedMap;
+import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
import org.apache.commons.lang3.StringUtils;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.shade.filter.Filter;
import org.apache.maven.plugins.shade.relocation.Relocator;
import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer;
import org.apache.maven.plugins.shade.resource.ReproducibleResourceTransformer;
import org.apache.maven.plugins.shade.resource.ResourceTransformer;
-import org.codehaus.plexus.component.annotations.Component;
-import org.codehaus.plexus.logging.AbstractLogEnabled;
import org.codehaus.plexus.util.IOUtil;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.ClassWriter;
import org.objectweb.asm.commons.ClassRemapper;
import org.objectweb.asm.commons.Remapper;
-
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.Multimap;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* @author Jason van Zyl
*/
-@Component( role = Shader.class, hint = "default" )
+@Singleton
+@Named
public class DefaultShader
- extends AbstractLogEnabled
implements Shader
{
private static final int BUFFER_SIZE = 32 * 1024;
+ private final Logger logger;
+
+ public DefaultShader()
+ {
+ this( LoggerFactory.getLogger( DefaultShader.class ) );
+ }
+
+ public DefaultShader( final Logger logger )
+ {
+ this.logger = Objects.requireNonNull( logger );
Review comment:
UTs are mocking logger and use it to verify output, (did not modify UT more than I had). In plugin (runtime) the default ctor is 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] michael-o commented on pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#issuecomment-845167457
All tests pass here. Only few issues to resolve and we are good to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r635555199
##########
File path: pom.xml
##########
@@ -244,21 +302,33 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <forceJavacCompilerUse>true</forceJavacCompilerUse>
Review comment:
Needed when built on Jana11. Why?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r635555199
##########
File path: pom.xml
##########
@@ -244,21 +302,33 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <forceJavacCompilerUse>true</forceJavacCompilerUse>
Review comment:
Needed when built on Java11 (NPE in compiler plugin otherwise). Why?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#issuecomment-845729828
merged as fa5e40dc59355163d89cd904fcf4c4724ba3d3d6
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas closed pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas closed pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] michael-o commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r636145640
##########
File path: pom.xml
##########
@@ -125,12 +150,32 @@
<version>3.3.0</version>
</dependency>
+ <!-- DI -->
+ <dependency>
+ <groupId>javax.inject</groupId>
+ <artifactId>javax.inject</artifactId>
+ <version>1</version>
+ <scope>provided</scope>
+ </dependency>
+
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-artifact-transfer</artifactId>
<version>0.13.1</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.sonatype.sisu</groupId>
+ <artifactId>sisu-inject-plexus</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
+
<!-- Others -->
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ <version>1.7.25</version>
Review comment:
1.7.30?
##########
File path: src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
##########
@@ -651,14 +641,13 @@ private void setupHintedShader()
{
if ( shaderHint != null )
{
- try
- {
- shader = (Shader) plexusContainer.lookup( Shader.ROLE, shaderHint );
- }
- catch ( ComponentLookupException e )
+ shader = shaders.get( shaderHint );
+
+ if ( shader == null )
{
- throw new MojoExecutionException( "unable to lookup own Shader implementation with hint:'" + shaderHint
- + "'", e );
+ throw new MojoExecutionException(
+ "unable to lookup own Shader implementation with hint:'" + shaderHint + "'"
Review comment:
add a space after the colon
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -48,35 +49,49 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipException;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.commons.collections4.MultiValuedMap;
+import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
import org.apache.commons.lang3.StringUtils;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.shade.filter.Filter;
import org.apache.maven.plugins.shade.relocation.Relocator;
import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer;
import org.apache.maven.plugins.shade.resource.ReproducibleResourceTransformer;
import org.apache.maven.plugins.shade.resource.ResourceTransformer;
-import org.codehaus.plexus.component.annotations.Component;
-import org.codehaus.plexus.logging.AbstractLogEnabled;
import org.codehaus.plexus.util.IOUtil;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.ClassWriter;
import org.objectweb.asm.commons.ClassRemapper;
import org.objectweb.asm.commons.Remapper;
-
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.Multimap;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* @author Jason van Zyl
*/
-@Component( role = Shader.class, hint = "default" )
+@Singleton
+@Named
public class DefaultShader
- extends AbstractLogEnabled
implements Shader
{
private static final int BUFFER_SIZE = 32 * 1024;
+ private final Logger logger;
+
+ public DefaultShader()
+ {
+ this( LoggerFactory.getLogger( DefaultShader.class ) );
+ }
+
+ public DefaultShader( final Logger logger )
+ {
+ this.logger = Objects.requireNonNull( logger );
Review comment:
Why so complicated instead of `...logger = LoggerFactory...`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r636313518
##########
File path: src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
##########
@@ -651,14 +641,13 @@ private void setupHintedShader()
{
if ( shaderHint != null )
{
- try
- {
- shader = (Shader) plexusContainer.lookup( Shader.ROLE, shaderHint );
- }
- catch ( ComponentLookupException e )
+ shader = shaders.get( shaderHint );
+
+ if ( shader == null )
{
- throw new MojoExecutionException( "unable to lookup own Shader implementation with hint:'" + shaderHint
- + "'", e );
+ throw new MojoExecutionException(
+ "unable to lookup own Shader implementation with hint:'" + shaderHint + "'"
Review comment:
fixed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r635556331
##########
File path: src/test/java/org/apache/maven/plugins/shade/DefaultShaderTest.java
##########
@@ -364,55 +359,162 @@ private static DefaultShader newShader()
{
DefaultShader s = new DefaultShader();
- s.enableLogging( new ConsoleLogger( Logger.LEVEL_INFO, "TEST" ) );
-
return s;
}
- private static class MockLogger extends AbstractLogger
+ private static class MockLogger extends MarkerIgnoringBase
Review comment:
Is there simpler way to achieve this? SLF4J logger that collects debug and warn messages...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] cstamas commented on a change in pull request #91: [MSHADE-389] Get rid of old baggage
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #91:
URL: https://github.com/apache/maven-shade-plugin/pull/91#discussion_r636316160
##########
File path: pom.xml
##########
@@ -125,12 +150,32 @@
<version>3.3.0</version>
</dependency>
+ <!-- DI -->
+ <dependency>
+ <groupId>javax.inject</groupId>
+ <artifactId>javax.inject</artifactId>
+ <version>1</version>
+ <scope>provided</scope>
+ </dependency>
+
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-artifact-transfer</artifactId>
<version>0.13.1</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.sonatype.sisu</groupId>
+ <artifactId>sisu-inject-plexus</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
+
<!-- Others -->
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ <version>1.7.25</version>
Review comment:
fixed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org