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-323] make shade plugin compatible with the flatten plugin #129

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[MSHADE-323] make shade plugin compatible with the flatten plugin #129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3ad36d
[MSHADE-323] compatible with the flatten plugin
gzm55 Apr 6, 2022
a0f8848
fix description and style
gzm55 Apr 6, 2022
221c760
set ModelReader to parse pom file
gzm55 Apr 6, 2022
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
84 changes: 84 additions & 0 deletions src/it/projects/MSHADE-323-compatible-with-flatten-pom/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?xml version="1.0" encoding="UTF-8"?>

<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->

<project>
<modelVersion>4.0.0</modelVersion>

<groupId>org.apache.maven.its.shade.flatten</groupId>
<artifactId>test</artifactId>
<version>1.0</version>

<name>MSHADE-323</name>
<description>
Test that integration of shade and flatten plugins.
gzm55 marked this conversation as resolved.
Show resolved Hide resolved
</description>

<properties>
<ver>0.1</ver>
<maven.jar.forceCreation>true</maven.jar.forceCreation>
</properties>

<dependencies>
<dependency>
<groupId>org.apache.maven.its.shade.drp</groupId>
<artifactId>a</artifactId>
<version>${ver}</version>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<version>1.2.5</version>
<executions>
<execution>
<goals>
<goal>flatten</goal>
</goals>
<phase>prepare-package</phase>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<version>3.2.2</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
<version>@project.version@</version>
<executions>
<execution>
<goals>
<goal>shade</goal>
</goals>
<configuration>
<createDependencyReducedPom>true</createDependencyReducedPom>
<dependencyReducedPomLocation>target/dependency-reduced-pom.xml</dependencyReducedPomLocation>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
20 changes: 20 additions & 0 deletions src/it/projects/MSHADE-323-compatible-with-flatten-pom/src/main/java/A.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

class A {}
26 changes: 26 additions & 0 deletions src/it/projects/MSHADE-323-compatible-with-flatten-pom/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

File pomFile = new File( basedir, "target/dependency-reduced-pom.xml" );
assert pomFile.isFile()

def ns = new groovy.xml.Namespace("http://maven.apache.org/POM/4.0.0")
gzm55 marked this conversation as resolved.
Show resolved Hide resolved
def pom = new XmlParser().parse( pomFile )

assert pom.properties.size() == 0
9 changes: 7 additions & 2 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 @@ -25,6 +25,7 @@
import org.apache.maven.model.Dependency;
import org.apache.maven.model.Exclusion;
import org.apache.maven.model.Model;
import org.apache.maven.model.io.xpp3.MavenXpp3Reader;
import org.apache.maven.plugin.AbstractMojo;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.annotations.Component;
Expand Down Expand Up @@ -57,6 +58,7 @@
import org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolverException;
import org.codehaus.plexus.util.IOUtil;
import org.codehaus.plexus.util.WriterFactory;
import org.codehaus.plexus.util.xml.pull.XmlPullParserException;

import java.io.File;
import java.io.FileInputStream;
Expand Down Expand Up @@ -1059,7 +1061,7 @@ private File shadedTestArtifactFile()
// We need to find the direct dependencies that have been included in the uber JAR so that we can modify the
// POM accordingly.
private void createDependencyReducedPom( Set<String> artifactsToRemove )
throws IOException, DependencyGraphBuilderException, ProjectBuildingException
throws IOException, DependencyGraphBuilderException, ProjectBuildingException, XmlPullParserException
{
List<Dependency> dependencies = new ArrayList<>();

Expand Down Expand Up @@ -1090,7 +1092,10 @@ private void createDependencyReducedPom( Set<String> artifactsToRemove )
origDeps = transitiveDeps;
}

Model model = project.getOriginalModel();
final Model model = project.getFile() == null
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like an issue any plugin consuming the pom will get so it should likely be fixed in flatten plugin using setOriginalModel to be a global fix and not a local one.

Copy link
Author

@gzm55 gzm55 Apr 6, 2022 •
edited by gnodet
Loading

Choose a reason for hiding this comment

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

hi @rmannibucau , the in Maven, what is originalModel defined to be, a model of the original pom or a model of the current pom file? If defined as the latter one, the global solution of updating originalModel could be done in MavenProject::setPomFile or MavenProject::setFile.

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that it was the really original file but since maven is a chain (per module) it is in practise the current pom file - otherwise it can't really be used by any plugins and some do.
So guess the choice is:

  1. keep original untouched but means no plugin should use it except help/debug ones (so fix is to always read getFile())
  2. ensure flatten plugin updates original model to let downstream plugins consumes the post processing model

Personally I think the option 2 is what makes the most sense for the ecosystem (inter-plugin operability).

Copy link
Author

Choose a reason for hiding this comment

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

ok, this pr is the first choice, which prefer a practice single source to get current raw model.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gzm55 this pr does not impl 1 since it should never use original model in case 1 but always file to enable to chain plugins.

Copy link
Author

Choose a reason for hiding this comment

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

it is ok to not fall back to original model when getFile() is null, throwing an exception instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I just checked flatten code and it only works if updatePomFile is set to true no? So long story short, if you want it to work reliably you must ensure originalModel is set before shade plugin is executed. From a design point of view it is the responsability of flatten plugin to do that since it is the one hacking the user pom but an intermediary plugin setting it from a file when flatten plugin dumped its value would work too but I don't think there is any bug in shade there.

Copy link
Author

Choose a reason for hiding this comment

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

it seems better to have a convention or rule that the original model should be equal to the pom file. the shade plugin also breaking this convention, it reverts the parent.relativePath after write the model to the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

shade plugins reset it cause it just needs to change it while dumping the pom (that said good point it breaks thread safety in concurrent builds).
agree shade plugin breaks this rules and happy to get it fixed, it is just not that important cause shade plugin is generally in the end of the chain (so dowstream consumers list is empty) and it also handles itself the pom rewriting but you are right.
long story short: I don't think we should re-read from the filesystem any pom model anytime in maven build otherwise the design itself is broken so I'm in favor of trying to not use the reader more than once per file.

hope it makes sense.

? project.getOriginalModel().clone()
: new MavenXpp3Reader().read( new FileInputStream( project.getFile() ) );
gzm55 marked this conversation as resolved.
Show resolved Hide resolved

// MSHADE-185: We will remove all system scoped dependencies which usually
// have some kind of property usage. At this time the properties within
// such things are already evaluated.
Expand Down