Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSHADE-307] adding useDefaultConfiguration flag in ShadeMojo #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[MSHADE-307] adding useDefaultConfiguration flag in ShadeMojo #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/main/java/org/apache/maven/plugins/shade/mojo/ArchiveFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ public class ArchiveFilter

private boolean excludeDefaults = true;

public ArchiveFilter()
{
// no-op
}

public ArchiveFilter( Set<String> includes, Set<String> excludes )
{
this.includes = includes;
this.excludes = excludes;
}

public String getArtifact()
{
return artifact;
Expand Down
149 changes: 136 additions & 13 deletions src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
import org.apache.maven.plugins.shade.pom.PomWriter;
import org.apache.maven.plugins.shade.relocation.Relocator;
import org.apache.maven.plugins.shade.relocation.SimpleRelocator;
import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer;
import org.apache.maven.plugins.shade.resource.ResourceTransformer;
import org.apache.maven.plugins.shade.resource.ServicesResourceTransformer;
import org.apache.maven.project.DefaultProjectBuildingRequest;
import org.apache.maven.project.MavenProject;
import org.apache.maven.project.MavenProjectHelper;
Expand Down Expand Up @@ -76,8 +78,10 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
import java.util.Set;

/**
Expand Down Expand Up @@ -490,7 +494,7 @@ public void execute()
replaceFile( finalFile, testJar );
testJar = finalFile;
}

renamed = true;
}

Expand Down Expand Up @@ -752,30 +756,66 @@ private List<Relocator> getRelocators()

private List<ResourceTransformer> getResourceTransformers()
{
final List<ResourceTransformer> resourceTransformers = new LinkedList<>( getDefaultResourceTransformers() );
if ( transformers == null )
{
return Collections.emptyList();
return resourceTransformers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still merging. Here you should replace Collections.emptyList() with getDefaultResourceTransformers()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the case, resourceTransformers is initialized with getDefaultResourceTransformers. If the else skips the default transformers the feature is pointless since in 80% of the case you need to add a custom transformer or customize a config (like mainClass for the manifest)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's agree to disagree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to disagree but I'd like to solve the issue which aims to reduce the entry cost of the plugin and save hours of debug time due to a bad packaging which is hard to identify - and even more with jpms since module-info.java drops SPI registration :s.

any alternative proposal to have a trivial to enable good auto configuration compatible with user customizations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of a clean, clear and simple solution yet. So let's split it into 2 issues: 1. have good default transformers; 2. combine default+custom transfomers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's split. Just to ensure I get it and not do work for nothing, it means 3 PR - no order:

  1. defaults (transformers and filters)
  2. SPI
  3. combine
    ?

Copy link
Contributor Author

@rmannibucau rmannibucau Dec 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 done at #12
2 done at #13

will let 3 to a bit later to see if we find a good idea

}
resourceTransformers.addAll( Arrays.asList( transformers ) );
return resourceTransformers;
}

return Arrays.asList( transformers );
private List<ResourceTransformer> getDefaultResourceTransformers()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not merging, so you can return a list with these 2 transformers.
Also document this on the transfomers-parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess it is addressed by previous comment, we want to merge ;)

final List<ResourceTransformer> transformers = new LinkedList<>();
if ( missTransformer( ServicesResourceTransformer.class ) )
{
getLog().debug( "Adding ServicesResourceTransformer transformer" );
transformers.add( new ServicesResourceTransformer() );
}
if ( missTransformer( ManifestResourceTransformer.class ) )
{
getLog().debug( "Adding ManifestResourceTransformer transformer" );
transformers.add( new ManifestResourceTransformer() );
}
for ( final ResourceTransformer transformer : ServiceLoader.load( ResourceTransformer.class ) )
{
if ( !missTransformer( transformer.getClass() ) )
{
continue;
}
getLog().debug( "Adding " + transformer.getClass().getName() + " transformer" );
transformers.add( transformer );
}
return transformers;
}

private boolean missTransformer( final Class<?> type )
{
if ( transformers == null )
{
return true;
}
for ( final ResourceTransformer transformer : transformers )
{
if ( type.isInstance( transformer ) )
{
return false;
}
}
return true;
}

private List<Filter> getFilters()
throws MojoExecutionException
{
List<Filter> filters = new ArrayList<>();
List<SimpleFilter> simpleFilters = new ArrayList<>();
List<Filter> filters = new LinkedList<>();
List<SimpleFilter> simpleFilters = new LinkedList<>();
Map<Artifact, ArtifactId> artifacts = null;

if ( this.filters != null && this.filters.length > 0 )
{
Map<Artifact, ArtifactId> artifacts = new HashMap<>();

artifacts.put( project.getArtifact(), new ArtifactId( project.getArtifact() ) );

for ( Artifact artifact : project.getArtifacts() )
{
artifacts.put( artifact, new ArtifactId( artifact ) );
}
artifacts = getArtifactIds();

for ( ArchiveFilter filter : this.filters )
{
Expand Down Expand Up @@ -813,6 +853,51 @@ private List<Filter> getFilters()
}
}

// first check for backward compatibility this is not already configured explicitly
boolean addExclusion = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a similar trick here:

public Collection<Filter> getFilters()
{
  if ( this.filters == null )
  {
    return defaultFilters();
  }
  else
  {
    return this.filters;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely the same

if ( this.filters != null )
{
for ( final ArchiveFilter filter : this.filters )
{
if ( filter.getExcludes() != null
&& filter.getExcludes().contains( "META-INF/*.SF" )
&& filter.getExcludes().contains( "META-INF/*.DSA" )
&& filter.getExcludes().contains( "META-INF/*.RSA" )
&& "*:*".equals( filter.getArtifact() ) )
{
addExclusion = false;
break;
}
}
}
if ( addExclusion )
{
getLog().debug( "Adding META-INF/*.SF, META-INF/*.DSA and META-INF/*.RSA exclusion" );

if ( artifacts == null )
{
artifacts = getArtifactIds();
}
simpleFilters.add( new SimpleFilter(
getMatchingJars( artifacts , new ArtifactId( "*:*" ) ),
new ArchiveFilter(
Collections.<String>emptySet(),
new HashSet<>( Arrays.asList( "META-INF/*.SF", "META-INF/*.DSA", "META-INF/*.RSA" ) ) ) ) );
}

for ( final Filter filter : ServiceLoader.load( Filter.class ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ServiceLoader requires a separate JIRA issue, and I wonder if this is going to work. See https://issues.apache.org/jira/browse/MNG-6275

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must work if the extension is added in plugin dependencies as commonly done for transformers - http://openwebbeans.apache.org/meecrowave/meecrowave-maven/index.html#_shading for example. About the other issue: it can be the case but the issue is to make the configuration easier, if we don't want of a toggle then the SPI is the answer so it is the same issue, no?

{
getLog().debug( "Adding " + filter.getClass().getName() + " filter" );
if ( SimpleFilter.class.isInstance( filter ) )
{
simpleFilters.add( SimpleFilter.class.cast( filter ) );
}
else
{
filters.add( filter );
}
}

filters.addAll( simpleFilters );

if ( minimizeJar )
Expand All @@ -832,6 +917,44 @@ private List<Filter> getFilters()
return filters;
}

private Set<File> getMatchingJars( final Map<Artifact, ArtifactId> artifacts, final ArtifactId pattern )
{
final Set<File> jars = new HashSet<File>();

for ( final Map.Entry<Artifact, ArtifactId> entry : artifacts.entrySet() )
{
if ( entry.getValue().matches( pattern ) )
{
final Artifact artifact = entry.getKey();

jars.add( artifact.getFile() );

if ( createSourcesJar )
{
final File file = resolveArtifactSources( artifact );
if ( file != null )
{
jars.add( file );
}
}
}
}
return jars;
}

private Map<Artifact, ArtifactId> getArtifactIds()
{
final Map<Artifact, ArtifactId> artifacts = new HashMap<Artifact, ArtifactId>();

artifacts.put( project.getArtifact(), new ArtifactId( project.getArtifact() ) );

for ( final Artifact artifact : project.getArtifacts() )
{
artifacts.put( artifact, new ArtifactId( artifact ) );
}
return artifacts;
}

private File shadedArtifactFileWithClassifier()
{
Artifact artifact = project.getArtifact();
Expand Down
111 changes: 82 additions & 29 deletions src/test/java/org/apache/maven/plugins/shade/mojo/ShadeMojoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.net.URLClassLoader;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -41,10 +42,13 @@
import org.apache.maven.plugins.shade.ShadeRequest;
import org.apache.maven.plugins.shade.Shader;
import org.apache.maven.plugins.shade.filter.Filter;
import org.apache.maven.plugins.shade.filter.SimpleFilter;
import org.apache.maven.plugins.shade.relocation.Relocator;
import org.apache.maven.plugins.shade.relocation.SimpleRelocator;
import org.apache.maven.plugins.shade.resource.ComponentsXmlResourceTransformer;
import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer;
import org.apache.maven.plugins.shade.resource.ResourceTransformer;
import org.apache.maven.plugins.shade.resource.ServicesResourceTransformer;
import org.apache.maven.project.MavenProject;
import org.apache.maven.project.ProjectBuildingRequest;
import org.apache.maven.shared.transfer.artifact.ArtifactCoordinate;
Expand All @@ -60,6 +64,49 @@
public class ShadeMojoTest
extends PlexusTestCase
{

public void testDefaultConfiguration() throws Exception
{
final ShadeMojo shadeMojo = new ShadeMojo();
setProject(shadeMojo);

// default transformers are present
final Method getResourceTransformers = ShadeMojo.class.getDeclaredMethod("getResourceTransformers");
getResourceTransformers.setAccessible(true);
final List<ResourceTransformer> transformers =
List.class.cast(getResourceTransformers.invoke(shadeMojo));
assertEquals(2, transformers.size());
assertTrue(ServicesResourceTransformer.class.isInstance(transformers.get(0)));
assertTrue(ManifestResourceTransformer.class.isInstance(transformers.get(1)));

// default exclusion is present
final Method getFilters = ShadeMojo.class.getDeclaredMethod("getFilters");
getFilters.setAccessible(true);
final List<Filter> filters =
List.class.cast(getFilters.invoke(shadeMojo));
assertEquals(1, filters.size());

final Filter filter = filters.iterator().next();
assertTrue(SimpleFilter.class.isInstance(filter));

final Field jars = filter.getClass().getDeclaredField("jars");
jars.setAccessible(true);
assertEquals(1, Collection.class.cast(jars.get(filter)).size());

final Field excludes = filter.getClass().getDeclaredField("excludes");
excludes.setAccessible(true);
final Collection<String> excludesValues = Collection.class.cast(excludes.get(filter));
assertEquals(3, excludesValues.size());
for ( final String exclude : Arrays.asList( "META-INF/*.SF", "META-INF/*.DSA", "META-INF/*.RSA" ) )
{
assertTrue(exclude, excludesValues.contains(exclude) );
}

final Field includes = filter.getClass().getDeclaredField("includes");
includes.setAccessible(true);
assertTrue(Collection.class.cast(includes.get(filter)).isEmpty());
}

public void testShaderWithDefaultShadedPattern()
throws Exception
{
Expand Down Expand Up @@ -124,11 +171,45 @@ public void testShadeWithFilter()
createSourcesJar.setAccessible( true );
createSourcesJar.set( mojo, Boolean.TRUE );

// setup a project
setProject(mojo);

// create and configure the ArchiveFilter
ArchiveFilter archiveFilter = new ArchiveFilter();
Field archiveFilterArtifact = ArchiveFilter.class.getDeclaredField( "artifact" );
archiveFilterArtifact.setAccessible( true );
archiveFilterArtifact.set( archiveFilter, "org.apache.myfaces.core:myfaces-impl" );

// add ArchiveFilter to mojo
Field filtersField = ShadeMojo.class.getDeclaredField( "filters" );
filtersField.setAccessible( true );
filtersField.set( mojo, new ArchiveFilter[]{ archiveFilter } );

Field sessionField = ShadeMojo.class.getDeclaredField( "session" );
sessionField.setAccessible( true );
sessionField.set( mojo, mock( MavenSession.class ) );

// invoke getFilters()
Method getFilters = ShadeMojo.class.getDeclaredMethod( "getFilters", new Class[0] );
getFilters.setAccessible( true );
List<Filter> filters = (List<Filter>) getFilters.invoke( mojo);

// assertions - there must be one filter
assertEquals( 2, filters.size() );

// the filter must be able to filter the binary and the sources jar
Filter filter = filters.get( 1 ); // 0 is the built-in META-INF/*
assertTrue( filter.canFilter( new File( "myfaces-impl-2.0.1-SNAPSHOT.jar" ) ) ); // binary jar
assertTrue( filter.canFilter( new File( "myfaces-impl-2.0.1-SNAPSHOT-sources.jar" ) ) ); // sources jar
}

private void setProject(final ShadeMojo mojo) throws Exception
{
// configure artifactResolver (mocked) for mojo
ArtifactResolver mockArtifactResolver = new ArtifactResolver()
{
@Override
public ArtifactResult resolveArtifact( ProjectBuildingRequest req, final Artifact art )
public ArtifactResult resolveArtifact(ProjectBuildingRequest req, final Artifact art )
throws ArtifactResolverException
{
return new ArtifactResult()
Expand Down Expand Up @@ -185,34 +266,6 @@ public Artifact getArtifact()
Field projectField = ShadeMojo.class.getDeclaredField( "project" );
projectField.setAccessible( true );
projectField.set( mojo, project );

// create and configure the ArchiveFilter
ArchiveFilter archiveFilter = new ArchiveFilter();
Field archiveFilterArtifact = ArchiveFilter.class.getDeclaredField( "artifact" );
archiveFilterArtifact.setAccessible( true );
archiveFilterArtifact.set( archiveFilter, "org.apache.myfaces.core:myfaces-impl" );

// add ArchiveFilter to mojo
Field filtersField = ShadeMojo.class.getDeclaredField( "filters" );
filtersField.setAccessible( true );
filtersField.set( mojo, new ArchiveFilter[]{ archiveFilter } );

Field sessionField = ShadeMojo.class.getDeclaredField( "session" );
sessionField.setAccessible( true );
sessionField.set( mojo, mock( MavenSession.class ) );

// invoke getFilters()
Method getFilters = ShadeMojo.class.getDeclaredMethod( "getFilters" );
getFilters.setAccessible( true );
List<Filter> filters = (List<Filter>) getFilters.invoke( mojo);

// assertions - there must be one filter
assertEquals( 1, filters.size() );

// the filter must be able to filter the binary and the sources jar
Filter filter = filters.get( 0 );
assertTrue( filter.canFilter( new File( "myfaces-impl-2.0.1-SNAPSHOT.jar" ) ) ); // binary jar
assertTrue( filter.canFilter( new File( "myfaces-impl-2.0.1-SNAPSHOT-sources.jar" ) ) ); // sources jar
}

public void shaderWithPattern( String shadedPattern, File jar )
Expand Down