From 046916d0e56499a953b5486ebd110ae87940fc1b Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Tue, 20 Jul 2021 12:12:16 +0200 Subject: [PATCH 1/2] basic dir handling in default shader --- .../maven/plugins/shade/DefaultShader.java | 177 +++++++++++++----- .../plugins/shade/DefaultShaderTest.java | 114 ++++++++--- 2 files changed, 219 insertions(+), 72 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/shade/DefaultShader.java b/src/main/java/org/apache/maven/plugins/shade/DefaultShader.java index 9226e3b8..aa69e237 100644 --- a/src/main/java/org/apache/maven/plugins/shade/DefaultShader.java +++ b/src/main/java/org/apache/maven/plugins/shade/DefaultShader.java @@ -21,6 +21,7 @@ import java.io.BufferedOutputStream; import java.io.File; +import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -40,6 +41,7 @@ import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.concurrent.Callable; import java.util.jar.JarEntry; import java.util.jar.JarFile; import java.util.jar.JarOutputStream; @@ -231,60 +233,140 @@ private void shadeJars( ShadeRequest shadeRequest, Set resources, List jarFilters = getFilters( jar, shadeRequest.getFilters() ); - - try ( JarFile jarFile = newJarFile( jar ) ) + if ( jar.isDirectory() ) + { + shadeDir( shadeRequest, resources, transformers, remapper, jos, duplicates, jar, jar, "", jarFilters ); + } + else { + shadeJar( shadeRequest, resources, transformers, remapper, jos, duplicates, jar, jarFilters ); + } + } + } - for ( Enumeration j = jarFile.entries(); j.hasMoreElements(); ) + private void shadeDir( ShadeRequest shadeRequest, Set resources, + List transformers, RelocatorRemapper remapper, + JarOutputStream jos, MultiValuedMap duplicates, + File jar, File current, String prefix, List jarFilters ) throws IOException + { + final File[] children = current.listFiles(); + if ( children == null ) + { + return; + } + for ( final File file : children ) + { + final String name = prefix + file.getName(); + if ( file.isDirectory() ) + { + try + { + shadeDir( + shadeRequest, resources, transformers, remapper, jos, + duplicates, jar, file, + prefix + file.getName() + '/', jarFilters ); + continue; + } + catch ( Exception e ) { - JarEntry entry = j.nextElement(); + throw new IOException( + String.format( "Problem shading JAR %s entry %s: %s", current, name, e ), e ); + } + } + if ( isFiltered( jarFilters, name ) || isExcludedEntry( name ) ) + { + continue; + } - String name = entry.getName(); - - if ( entry.isDirectory() || isFiltered( jarFilters, name ) ) - { - continue; - } + try + { + shadeSingleJar( + shadeRequest, resources, transformers, remapper, jos, duplicates, jar, + new Callable() + { + @Override + public InputStream call() throws Exception + { + return new FileInputStream( file ); + } + }, name, file.lastModified(), -1 /*ignore*/ ); + } + catch ( Exception e ) + { + throw new IOException( String.format( "Problem shading JAR %s entry %s: %s", current, name, e ), + e ); + } + } + } + private void shadeJar( ShadeRequest shadeRequest, Set resources, + List transformers, RelocatorRemapper remapper, + JarOutputStream jos, MultiValuedMap duplicates, + File jar, List jarFilters ) throws IOException + { + try ( JarFile jarFile = newJarFile( jar ) ) + { - if ( "META-INF/INDEX.LIST".equals( name ) ) - { - // we cannot allow the jar indexes to be copied over or the - // jar is useless. Ideally, we could create a new one - // later - continue; - } + for ( Enumeration j = jarFile.entries(); j.hasMoreElements(); ) + { + final JarEntry entry = j.nextElement(); - if ( "module-info.class".equals( name ) ) - { - logger.warn( "Discovered module-info.class. " - + "Shading will break its strong encapsulation." ); - continue; - } + String name = entry.getName(); - try - { - shadeSingleJar( shadeRequest, resources, transformers, remapper, jos, duplicates, jar, - jarFile, entry, name ); - } - catch ( Exception e ) - { - throw new IOException( String.format( "Problem shading JAR %s entry %s: %s", jar, name, e ), - e ); - } + if ( entry.isDirectory() || isFiltered( jarFilters, name ) || isExcludedEntry( name ) ) + { + continue; } + try + { + shadeSingleJar( + shadeRequest, resources, transformers, remapper, jos, duplicates, jar, + new Callable() + { + @Override + public InputStream call() throws Exception + { + return jarFile.getInputStream( entry ); + } + }, name, entry.getTime(), entry.getMethod() ); + } + catch ( Exception e ) + { + throw new IOException( String.format( "Problem shading JAR %s entry %s: %s", jar, name, e ), + e ); + } } + } } + private boolean isExcludedEntry( final String name ) + { + if ( "META-INF/INDEX.LIST".equals( name ) ) + { + // we cannot allow the jar indexes to be copied over or the + // jar is useless. Ideally, we could create a new one + // later + return true; + } + + if ( "module-info.class".equals( name ) ) + { + logger.warn( "Discovered module-info.class. " + + "Shading will break its strong encapsulation." ); + return true; + } + return false; + } + private void shadeSingleJar( ShadeRequest shadeRequest, Set resources, List transformers, RelocatorRemapper remapper, JarOutputStream jos, MultiValuedMap duplicates, File jar, - JarFile jarFile, JarEntry entry, String name ) - throws IOException, MojoExecutionException + Callable inputProvider, String name, long time, int method ) + throws Exception { - try ( InputStream in = jarFile.getInputStream( entry ) ) + try ( InputStream in = inputProvider.call() ) { String mappedName = remapper.map( name ); @@ -295,14 +377,14 @@ private void shadeSingleJar( ShadeRequest shadeRequest, Set resources, String dir = mappedName.substring( 0, idx ); if ( !resources.contains( dir ) ) { - addDirectory( resources, jos, dir, entry.getTime() ); + addDirectory( resources, jos, dir, time ); } } duplicates.put( name, jar ); if ( name.endsWith( ".class" ) ) { - addRemappedClass( remapper, jos, jar, name, entry.getTime(), in ); + addRemappedClass( remapper, jos, jar, name, time, in ); } else if ( shadeRequest.isShadeSourcesContent() && name.endsWith( ".java" ) ) { @@ -312,12 +394,12 @@ else if ( shadeRequest.isShadeSourcesContent() && name.endsWith( ".java" ) ) return; } - addJavaSource( resources, jos, mappedName, entry.getTime(), in, shadeRequest.getRelocators() ); + addJavaSource( resources, jos, mappedName, time, in, shadeRequest.getRelocators() ); } else { if ( !resourceTransformed( transformers, mappedName, in, shadeRequest.getRelocators(), - entry.getTime() ) ) + time ) ) { // Avoid duplicates that aren't accounted for by the resource transformers if ( resources.contains( mappedName ) ) @@ -326,7 +408,7 @@ else if ( shadeRequest.isShadeSourcesContent() && name.endsWith( ".java" ) ) return; } - addResource( resources, jos, mappedName, entry, jarFile ); + addResource( resources, jos, mappedName, inputProvider, time, method ); } else { @@ -655,24 +737,24 @@ private void addJavaSource( Set resources, JarOutputStream jos, String n resources.add( name ); } - private void addResource( Set resources, JarOutputStream jos, String name, JarEntry originalEntry, - JarFile jarFile ) throws IOException + private void addResource( Set resources, JarOutputStream jos, String name, Callable input, + long time, int method ) throws Exception { - ZipHeaderPeekInputStream inputStream = new ZipHeaderPeekInputStream( jarFile.getInputStream( originalEntry ) ); + ZipHeaderPeekInputStream inputStream = new ZipHeaderPeekInputStream( input.call() ); try { final JarEntry entry = new JarEntry( name ); // We should not change compressed level of uncompressed entries, otherwise JVM can't load these nested jars - if ( inputStream.hasZipHeader() && originalEntry.getMethod() == ZipEntry.STORED ) + if ( inputStream.hasZipHeader() && method == ZipEntry.STORED ) { new CrcAndSize( inputStream ).setupStoredEntry( entry ); inputStream.close(); - inputStream = new ZipHeaderPeekInputStream( jarFile.getInputStream( originalEntry ) ); + inputStream = new ZipHeaderPeekInputStream( input.call() ); } - entry.setTime( originalEntry.getTime() ); + entry.setTime( time ); jos.putNextEntry( entry ); @@ -770,5 +852,4 @@ public String map( String name ) } } - } diff --git a/src/test/java/org/apache/maven/plugins/shade/DefaultShaderTest.java b/src/test/java/org/apache/maven/plugins/shade/DefaultShaderTest.java index d48fd43d..de8b7777 100644 --- a/src/test/java/org/apache/maven/plugins/shade/DefaultShaderTest.java +++ b/src/test/java/org/apache/maven/plugins/shade/DefaultShaderTest.java @@ -19,10 +19,30 @@ * under the License. */ +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.relocation.SimpleRelocator; +import org.apache.maven.plugins.shade.resource.AppendingTransformer; +import org.apache.maven.plugins.shade.resource.ComponentsXmlResourceTransformer; +import org.apache.maven.plugins.shade.resource.ResourceTransformer; +import org.codehaus.plexus.util.IOUtil; +import org.codehaus.plexus.util.Os; +import org.junit.Assert; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.mockito.ArgumentCaptor; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.Opcodes; +import org.slf4j.Logger; + import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.lang.reflect.Field; import java.net.URL; import java.net.URLClassLoader; @@ -30,33 +50,20 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Enumeration; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; import java.util.jar.JarEntry; import java.util.jar.JarFile; +import java.util.jar.JarInputStream; import java.util.jar.JarOutputStream; import java.util.zip.CRC32; import java.util.zip.ZipEntry; -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.relocation.SimpleRelocator; -import org.apache.maven.plugins.shade.resource.AppendingTransformer; -import org.apache.maven.plugins.shade.resource.ComponentsXmlResourceTransformer; -import org.apache.maven.plugins.shade.resource.ResourceTransformer; -import org.codehaus.plexus.util.IOUtil; -import org.codehaus.plexus.util.Os; -import org.junit.Assert; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; -import org.mockito.ArgumentCaptor; -import org.objectweb.asm.ClassReader; -import org.objectweb.asm.ClassVisitor; -import org.objectweb.asm.Opcodes; -import org.slf4j.Logger; - +import static java.util.Arrays.asList; +import static java.util.Collections.singleton; +import static org.codehaus.plexus.util.FileUtils.forceMkdir; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.hasItems; @@ -76,6 +83,9 @@ public class DefaultShaderTest private static final String[] EXCLUDES = new String[] { "org/codehaus/plexus/util/xml/Xpp3Dom", "org/codehaus/plexus/util/xml/pull.*" }; + @ClassRule + public static final TemporaryFolder tmp = new TemporaryFolder(); + @Test public void testOverlappingResourcesAreLogged() throws IOException, MojoExecutionException { final DefaultShader shader = newShader(); @@ -220,6 +230,60 @@ public void testShaderWithoutExcludesShouldRemoveReferencesOfOriginalPattern() new String[] {} ); } + @Test + public void testHandleDirectory() + throws Exception + { + final File dir = tmp.getRoot(); + // explode src/test/jars/test-artifact-1.0-SNAPSHOT.jar in this temp dir + try ( final JarInputStream in = new JarInputStream( + new FileInputStream( "src/test/jars/test-artifact-1.0-SNAPSHOT.jar" ) ) ) + { + JarEntry nextJarEntry; + while ( (nextJarEntry = in.getNextJarEntry()) != null ) + { + if ( nextJarEntry.isDirectory() ) + { + continue; + } + final File out = new File( dir, nextJarEntry.getName() ); + forceMkdir( out.getParentFile() ); + try ( final OutputStream outputStream = new FileOutputStream( out ) ) + { + IOUtil.copy( in, outputStream, (int) Math.max( nextJarEntry.getSize(), 512 ) ); + } + } + } + + // do shade + final File shade = new File( "target/testHandleDirectory.jar" ); + shaderWithPattern( "org/shaded/plexus/util", shade, new String[0], singleton( dir ) ); + + // ensure directory was shaded properly + try ( final JarFile jar = new JarFile( shade ) ) + { + final List entries = new ArrayList<>(); + final Enumeration jarEntryEnumeration = jar.entries(); + while ( jarEntryEnumeration.hasMoreElements() ) + { + final JarEntry jarEntry = jarEntryEnumeration.nextElement(); + if ( jarEntry.isDirectory() ) + { + continue; + } + entries.add( jarEntry.getName() ); + } + Collections.sort( entries ); + assertEquals( + asList( + "META-INF/maven/org.apache.maven.plugins.shade/test-artifact/pom.properties", + "META-INF/maven/org.apache.maven.plugins.shade/test-artifact/pom.xml", + "org/apache/maven/plugins/shade/Lib.class" + ), + entries ); + } + } + @Test public void testShaderWithRelocatedClassname() throws Exception @@ -337,16 +401,18 @@ private void writeEntryWithoutCompression( String entryName, byte[] entryBytes, jos.closeEntry(); } - private void shaderWithPattern( String shadedPattern, File jar, String[] excludes ) - throws Exception + private void shaderWithPattern( String shadedPattern, File jar, String[] excludes ) throws Exception { - DefaultShader s = newShader(); - Set set = new LinkedHashSet<>(); - set.add( new File( "src/test/jars/test-project-1.0-SNAPSHOT.jar" ) ); - set.add( new File( "src/test/jars/plexus-utils-1.4.1.jar" ) ); + shaderWithPattern( shadedPattern, jar, excludes, set ); + } + + private void shaderWithPattern( String shadedPattern, File jar, String[] excludes, Set set ) + throws Exception + { + DefaultShader s = newShader(); List relocators = new ArrayList<>(); From 6c3d67a35908074ec7615d8980853b250d19ad96 Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Wed, 21 Jul 2021 08:56:36 +0200 Subject: [PATCH 2/2] @kriegaex feedback on the lack of minijarfilter handling for directories --- .../plugins/shade/filter/MinijarFilter.java | 110 ++++++++++++------ 1 file changed, 77 insertions(+), 33 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java index 818339ed..2133691f 100644 --- a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java +++ b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java @@ -32,6 +32,7 @@ import java.io.BufferedReader; import java.io.File; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -98,7 +99,7 @@ public MinijarFilter( MavenProject project, Log log, List simpleFi { Clazzpath cp = new Clazzpath(); - ClazzpathUnit artifactUnit = cp.addClazzpathUnit( new FileInputStream( artifactFile ), project.toString() ); + ClazzpathUnit artifactUnit = cp.addClazzpathUnit( artifactFile.toPath(), project.toString() ); for ( Artifact dependency : project.getArtifacts() ) { @@ -129,8 +130,42 @@ private void removeServices( final MavenProject project, final Clazzpath cp ) neededClasses.removeAll( removable ); try { + // IMPORTANT: runtime classpath is not consistent with previous analysis using getArtifacts + // -> this must be changed once analyzed more properly for ( final String fileName : project.getRuntimeClasspathElements() ) { + final File file = new File( fileName ); + + // likely target/classes from the project (getRuntimeClasspathElements) + // we visit it but since it is the shaded artifact we should be able to skip it in 90% of casesd + if ( file.isDirectory() ) + { + final File services = new File( fileName, "META-INF/services" ); + if ( !services.exists() || !services.isDirectory() ) + { + continue; + } + final File[] registeredServices = services.listFiles(); + if ( registeredServices == null ) + { + continue; + } + for ( final File entry : registeredServices ) + { + try + { + final String name = entry.getName(); + repeatScan = onServices( + new FileInputStream( entry ), neededClasses, name, cp ) || repeatScan; + } + catch ( final FileNotFoundException e ) + { + log.warn( e.getMessage() ); + } + } + continue; + } + try ( final JarFile jar = new JarFile( fileName ) ) { for ( final Enumeration entries = jar.entries(); entries.hasMoreElements(); ) @@ -143,39 +178,9 @@ private void removeServices( final MavenProject project, final Clazzpath cp ) final String serviceClassName = jarEntry.getName().substring( "META-INF/services/".length() ); - final boolean isNeededClass = neededClasses.contains( cp.getClazz( serviceClassName ) ); - if ( !isNeededClass ) - { - continue; - } + repeatScan = onServices( + jar.getInputStream( jarEntry ), neededClasses, serviceClassName, cp ) || repeatScan; - try ( final BufferedReader bufferedReader = - new BufferedReader( new InputStreamReader( jar.getInputStream( jarEntry ), UTF_8 ) ) ) - { - for ( String line = bufferedReader.readLine(); line != null; - line = bufferedReader.readLine() ) - { - final String className = line.split( "#", 2 )[0].trim(); - if ( className.isEmpty() ) - { - continue; - } - - final Clazz clazz = cp.getClazz( className ); - if ( clazz == null || !removable.contains( clazz ) ) - { - continue; - } - - log.debug( className + " was not removed because it is a service" ); - removeClass( clazz ); - repeatScan = true; // check whether the found classes use services in turn - } - } - catch ( final IOException e ) - { - log.warn( e.getMessage() ); - } } } catch ( final IOException e ) @@ -192,6 +197,45 @@ private void removeServices( final MavenProject project, final Clazzpath cp ) while ( repeatScan ); } + private boolean onServices( final InputStream in, final Set all, final String name, final Clazzpath cp ) + { + final boolean isNeededClass = all.contains( cp.getClazz( name ) ); + if ( !isNeededClass ) + { + return false; + } + + boolean rs = false; + try ( final BufferedReader bufferedReader = + new BufferedReader( new InputStreamReader( in, UTF_8 ) ) ) + { + for ( String line = bufferedReader.readLine(); line != null; + line = bufferedReader.readLine() ) + { + final String className = line.split( "#", 2 )[0].trim(); + if ( className.isEmpty() ) + { + continue; + } + + final Clazz clazz = cp.getClazz( className ); + if ( clazz == null || !removable.contains( clazz ) ) + { + continue; + } + + log.debug( className + " was not removed because it is a service" ); + removeClass( clazz ); + rs = true; + } + } + catch ( final IOException e ) + { + log.warn( e.getMessage() ); + } + return rs; + } + private void removeClass( final Clazz clazz ) { removable.remove( clazz );