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

Fix Profile Interpolation bug #114

Closed
wants to merge 5 commits into from
+100 −7
Closed

Fix Profile Interpolation bug #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b26719
introducing rawPom as replacement for originalPom as original pom has…
nruzic Jul 10, 2019
3292034
introducing test case for resolveCiFriendliesOnly interpolation bug
nruzic Jul 10, 2019
c6abd71
Fix whitespace
ciis0 Nov 12, 2019
3fa9e21
Use "version" interpolation mode in multi-module IT
ciis0 Nov 12, 2019
6819696
Merge branch 'master' into flatten-maven-plugin-1.1.x
hohwille Dec 17, 2019
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
22 changes: 20 additions & 2 deletions src/it/projects/complete-multimodule-parent-pom-cifriendly/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<configuration>
<flattenMode>resolveCiFriendliesOnly</flattenMode>
<flattenMode>version</flattenMode>
Copy link
Member

Choose a reason for hiding this comment

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

Please do not missuse existing ITs to test a new feature. You would need to create a new integration test instead.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should either "fix" the existing mode, or if we keep it create a new IT for version mode.

Copy link
Author

Choose a reason for hiding this comment

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

As per #114 (comment) resolveCiFriendlies should be fixed. :)

</configuration>
</plugin>
</plugins>
Expand Down Expand Up @@ -100,6 +100,8 @@
<revision>1.2.3.0</revision>

<key>value</key>
<repoHost>foo</repoHost>
<repoPath>bar</repoPath>
</properties>

<reporting>
Expand All @@ -114,4 +116,20 @@

<url>http://mojo.codehaus.org</url>

</project>
<profiles>
<profile>
<id>cifriendly-profile-bug</id>
<activation>
<activeByDefault>true</activeByDefault>
</activation>
<repositories>
<repository>
<id>myrepo</id>
<name>myrepo</name>
<url>https://${repoHost}/${repoPath}</url>
</repository>
</repositories>
</profile>
</profiles>

</project>
5 changes: 4 additions & 1 deletion src/it/projects/complete-multimodule-parent-pom-cifriendly/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,7 @@ assert '4.0.0' == flattendChildWithParentProject.modelVersion.text()
assert 'org.codehaus.mojo.flatten.its' == flattendChildWithParentProject.groupId.text()
assert 'multimodule-module-with-parent-cifriendly' == flattendChildWithParentProject.artifactId.text()
assert '1.2.3.4' == flattendChildWithParentProject.version.text()
assert '1.2.3.4' == flattendChildWithParentProject.parent.version.text()
assert '1.2.3.4' == flattendChildWithParentProject.parent.version.text()

// CiFriendly interpolation bug
assert 'https://${repoHost}/${repoPath}' == flattendProject.profiles.profile[0].repositories.repository[0].url.text()
5 changes: 4 additions & 1 deletion src/main/java/org/codehaus/mojo/flatten/ElementHandling.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ public enum ElementHandling
keep,

/** Remove the element entirely so it will not be present in flattened POM. */
remove
remove,

/** Take the element untouched from the original POM. Fix for {@link #keep} */
keepRaw
Comment on lines +52 to +53
Copy link
Member

Choose a reason for hiding this comment

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

interesting. So actually keep does not actually do what it should? Can you give an example?
And you invented a new handling to avoid breaking existing build configs?
That would make a lot of sense. So does keep somehow already resolve variables or what is it that it does too much?

Copy link
Author

Choose a reason for hiding this comment

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

The problem that arose for me was that the configuration in (active) profiles was interpolated, specifically ${basedir}.

And I did not invent anything, just rebased it. ^^

Copy link
Author

Choose a reason for hiding this comment

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

Without wanting to be rude, but the overall direction of the original PR-code is rather being a work-around that being a real fix :D

I guess the work remaining now is to fix the underlying issues so the workarounds (version, keepRaw) are not necessary anymore.


}
16 changes: 16 additions & 0 deletions src/main/java/org/codehaus/mojo/flatten/FlattenDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,22 @@ public void setIssueManagement( ElementHandling issueManagement )
setHandling( PomProperty.ISSUE_MANAGEMENT, issueManagement );
}

/**
* @return {@link ElementHandling} for {@link Model#getLicenses() licenses}.
*/
public ElementHandling getLicenses()
{
return getHandling( PomProperty.LICENSES );
}

/**
* @param licenses the {@link #getLicenses() licenses} to set.
*/
public void setLicenses( ElementHandling licenses )
{
setHandling( PomProperty.LICENSES, licenses );
}

/**
* @return {@link ElementHandling} for {@link Model#getCiManagement() ciManagement}.
*/
Expand Down
34 changes: 33 additions & 1 deletion src/main/java/org/codehaus/mojo/flatten/FlattenMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,12 @@ public enum FlattenMode
/** Only resolves variables revision, sha1 and changelist. Keeps everything else.
* See <a href="https://maven.apache.org/maven-ci-friendly.html">Maven CI Friendly</a> for further details.
*/
resolveCiFriendliesOnly;
resolveCiFriendliesOnly,

/**
* Fix for {@link #resolveCiFriendliesOnly}
*/
version;
Comment on lines +82 to +85
Copy link
Member

Choose a reason for hiding this comment

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

So if this is a "fix" for resolveCiFriendliesOnly why do we have to introduce a new mode then?
My impression is also that this plugin can not be understood by an avergage user anymore. So what does version mode do? Where is it documentated? I already had concerns like this with the naming of resolveCiFriendliesOnly and the documentation is already lacking there but I did not want to be a blocker for urgently requested features. I think that your PR is in general providing a very helpful improvement as I also experienced some problems/bugs with this resolveCiFriendliesOnly. However the way we address this should IMHO need some improvement.
@lasselindqvist do you have some cents on this?

Copy link
Author

Choose a reason for hiding this comment

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

I just kept it during rebase, no other reasons from my side. ^^


/**
* @return the {@link FlattenDescriptor} defined by this {@link FlattenMode}.
Expand Down Expand Up @@ -147,6 +151,34 @@ public FlattenDescriptor getDescriptor()
descriptor.setUrl( ElementHandling.interpolate );
descriptor.setVersion( ElementHandling.resolve );
break;
case version:
descriptor.setBuild(ElementHandling.keepRaw);
descriptor.setCiManagement(ElementHandling.keepRaw);
descriptor.setContributors(ElementHandling.keepRaw);
descriptor.setDependencies(ElementHandling.keepRaw);
descriptor.setDependencyManagement(ElementHandling.keepRaw);
descriptor.setDescription(ElementHandling.keepRaw);
descriptor.setDevelopers(ElementHandling.keepRaw);
descriptor.setDistributionManagement(ElementHandling.keepRaw);
descriptor.setInceptionYear(ElementHandling.keepRaw);
descriptor.setIssueManagement(ElementHandling.keepRaw);
descriptor.setLicenses(ElementHandling.keepRaw);
descriptor.setMailingLists(ElementHandling.keepRaw);
descriptor.setModules(ElementHandling.keepRaw);
descriptor.setName(ElementHandling.keepRaw);
descriptor.setOrganization(ElementHandling.keepRaw);
descriptor.setParent(ElementHandling.resolve);
descriptor.setPluginManagement(ElementHandling.keepRaw);
descriptor.setPluginRepositories(ElementHandling.keepRaw);
descriptor.setPrerequisites(ElementHandling.keepRaw);
descriptor.setProfiles(ElementHandling.keepRaw);
descriptor.setProperties(ElementHandling.keepRaw);
descriptor.setReporting(ElementHandling.keepRaw);
descriptor.setRepositories(ElementHandling.keepRaw);
descriptor.setScm(ElementHandling.keepRaw);
descriptor.setUrl(ElementHandling.keepRaw);
descriptor.setVersion(ElementHandling.resolve);
break;
case clean:
// nothing to do...
break;
Expand Down
25 changes: 23 additions & 2 deletions src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.apache.maven.model.building.ModelBuildingResult;
import org.apache.maven.model.building.ModelProblemCollector;
import org.apache.maven.model.interpolation.ModelInterpolator;
import org.apache.maven.model.io.xpp3.MavenXpp3Reader;
import org.apache.maven.model.io.xpp3.MavenXpp3Writer;
import org.apache.maven.model.profile.ProfileActivationContext;
import org.apache.maven.model.profile.ProfileInjector;
Expand All @@ -62,6 +63,7 @@
import org.codehaus.plexus.util.IOUtil;
import org.codehaus.plexus.util.StringUtils;
import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
import org.eclipse.aether.artifact.DefaultArtifact;
import org.eclipse.aether.impl.ArtifactDescriptorReader;
import org.eclipse.aether.resolution.ArtifactDescriptorException;
Expand All @@ -75,6 +77,7 @@
import javax.xml.parsers.SAXParserFactory;
import java.io.File;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
Expand Down Expand Up @@ -551,6 +554,7 @@ protected Model createFlattenedPom( File pomFile ) throws MojoExecutionException
Model originalPom = this.project.getOriginalModel();
Model resolvedPom = this.project.getModel();
Model interpolatedPom = createResolvedPom( buildingRequest );
Model rawPom = getRawPom(pomFile);

// copy the configured additional POM elements...

Expand All @@ -559,7 +563,7 @@ protected Model createFlattenedPom( File pomFile ) throws MojoExecutionException
if ( property.isElement() )
{
Model sourceModel = getSourceModel( descriptor, property, effectivePom, originalPom, resolvedPom,
interpolatedPom, cleanPom );
interpolatedPom, cleanPom, rawPom );
if ( sourceModel == null )
{
if ( property.isRequired() )
Expand All @@ -578,6 +582,20 @@ protected Model createFlattenedPom( File pomFile ) throws MojoExecutionException
return flattenedPom;
}

private Model getRawPom(File pomFile) throws MojoExecutionException
{
MavenXpp3Reader reader = new MavenXpp3Reader();
Model rawPom = null;
try
{
rawPom = reader.read(new FileReader(pomFile));
}
catch(IOException | XmlPullParserException e) {
throw new MojoExecutionException("Error reading raw model.", e);
}
return rawPom;
}

private Model createResolvedPom( ModelBuildingRequest buildingRequest )
{
LoggingModelProblemCollector problems = new LoggingModelProblemCollector( getLog() );
Expand Down Expand Up @@ -658,7 +676,8 @@ protected Model createCleanPom( Model effectivePom ) throws MojoExecutionExcepti
}

private Model getSourceModel( FlattenDescriptor descriptor, PomProperty<?> property, Model effectivePom,
Model originalPom, Model resolvedPom, Model interpolatedPom, Model cleanPom )
Model originalPom, Model resolvedPom, Model interpolatedPom, Model cleanPom,
Model rawPom )
{

ElementHandling handling = descriptor.getHandling( property );
Expand All @@ -678,6 +697,8 @@ private Model getSourceModel( FlattenDescriptor descriptor, PomProperty<?> prope
return cleanPom;
case remove:
return null;
case keepRaw:
return rawPom;
default:
throw new IllegalStateException( handling.toString() );
}
Expand Down