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

Jump to bottom

[MRELEASE-1154] [REGRESSION] MRELEASE-1109 breaks release of Maven Su… #229

Closed
wants to merge 1 commit into from
Closed

[MRELEASE-1154] [REGRESSION] MRELEASE-1109 breaks release of Maven Su… #229

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
150 changes: 48 additions & 102 deletions ...manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,12 @@
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.TimeZone;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.ArtifactUtils;
Expand Down Expand Up @@ -92,18 +89,6 @@ public abstract class AbstractRewritePomsPhase extends AbstractReleasePhase impl
*/
private String modelETL = JDomModelETLFactory.NAME;

/**
* Regular expression pattern matching Maven expressions (i.e. references to Maven properties).
* The first group selects the property name the expression refers to.
*/
private static final Pattern EXPRESSION_PATTERN = Pattern.compile("\\$\\{(.+)\\}");

/**
* All Maven properties allowed to be referenced in parent versions via expressions
* @see <a href="https://maven.apache.org/maven-ci-friendly.html">CI-Friendly Versions</a>
*/
private static final List<String> CI_FRIENDLY_PROPERTIES = Arrays.asList("revision", "sha1", "changelist");
Copy link

Choose a reason for hiding this comment

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

Be aware that the list of supported properties is extensible


private long startTime = -1 * 1000;

protected AbstractRewritePomsPhase(
Expand Down Expand Up @@ -281,7 +266,7 @@ private void transformDocument(

Properties properties = modelTarget.getProperties();

String parentVersion = rewriteParent(project, modelTarget, result, releaseDescriptor, simulate);
String parentVersion = rewriteParent(project, modelTarget, releaseDescriptor, simulate);

String projectId = ArtifactUtils.versionlessKey(project.getGroupId(), project.getArtifactId());

Expand Down Expand Up @@ -455,29 +440,8 @@ private void rewriteVersion(
modelTarget.setVersion(version);
}

/**
* Extracts the Maven property name from a given expression.
* @param expression the expression
* @return either {@code null} if value is no expression otherwise the property referenced in the expression
*/
public static String extractPropertyFromExpression(String expression) {
Matcher matcher = EXPRESSION_PATTERN.matcher(expression);
if (!matcher.matches()) {
return null;
}
return matcher.group(1);
}

public static boolean isCiFriendlyVersion(String version) {
return CI_FRIENDLY_PROPERTIES.contains(extractPropertyFromExpression(version));
}

private String rewriteParent(
MavenProject project,
Model targetModel,
ReleaseResult result,
ReleaseDescriptor releaseDescriptor,
boolean simulate)
MavenProject project, Model targetModel, ReleaseDescriptor releaseDescriptor, boolean simulate)
throws ReleaseFailureException {
String parentVersion = null;
if (project.hasParent()) {
Expand All @@ -494,13 +458,7 @@ private String rewriteParent(
throw new ReleaseFailureException("Version for parent '" + parent.getName() + "' was not mapped");
}
} else {
if (!isCiFriendlyVersion(targetModel.getParent().getVersion())) {
targetModel.getParent().setVersion(parentVersion);
} else {
logInfo(
result,
" Ignoring parent version update for CI friendly expression " + parent.getVersion());
}
targetModel.getParent().setVersion(parentVersion);
}
}
return parentVersion;
Expand Down Expand Up @@ -563,73 +521,61 @@ private void rewriteArtifactVersions(
if (rawVersion.equals(originalVersion)) {
logInfo(result, " Updating " + artifactId + " to " + mappedVersion);
coordinate.setVersion(mappedVersion);
} else {
String property = extractPropertyFromExpression(rawVersion);
if (property != null) {
if (property.startsWith("project.")
|| property.startsWith("pom.")
|| "version".equals(property)) {
if (!mappedVersion.equals(getNextVersion(releaseDescriptor, projectId))) {
logInfo(result, " Updating " + artifactId + " to " + mappedVersion);
coordinate.setVersion(mappedVersion);
} else {
logInfo(result, " Ignoring artifact version update for expression " + rawVersion);
}
} else if (properties != null) {
// version is an expression, check for properties to update instead
String propertyValue = properties.getProperty(property);
if (propertyValue != null) {
if (propertyValue.equals(originalVersion)) {
logInfo(result, " Updating " + rawVersion + " to " + mappedVersion);
// change the property only if the property is the same as what's in the reactor
properties.setProperty(property, mappedVersion);
} else if (mappedVersion.equals(propertyValue)) {
// this property may have been updated during processing a sibling.
logInfo(
result,
" Ignoring artifact version update for expression " + rawVersion
+ " because it is already updated");
} else if (!mappedVersion.equals(rawVersion)) {
// WARNING: ${pom.*} prefix support and ${version} is about to be dropped in mvn4!
// https://issues.apache.org/jira/browse/MNG-7404
// https://issues.apache.org/jira/browse/MNG-7244
if (mappedVersion.matches("\\$\\{project.+\\}")
|| mappedVersion.matches("\\$\\{pom.+\\}")
|| "${version}".equals(mappedVersion)) {
logInfo(
result,
" Ignoring artifact version update for expression " + mappedVersion);
// ignore... we cannot update this expression
} else {
// the value of the expression conflicts with what the user wanted to release
throw new ReleaseFailureException("The artifact (" + key + ") requires a "
+ "different version (" + mappedVersion + ") than what is found ("
+ propertyValue + ") for the expression (" + rawVersion + ") in the "
+ "project (" + projectId + ").");
}
}
} else {
if (CI_FRIENDLY_PROPERTIES.contains(property)) {
Copy link

Choose a reason for hiding this comment

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

More than one property is supported here!

} else if (rawVersion.matches("\\$\\{.+\\}")) {
String expression = rawVersion.substring(2, rawVersion.length() - 1);

if (expression.startsWith("project.")
|| expression.startsWith("pom.")
|| "version".equals(expression)) {
if (!mappedVersion.equals(getNextVersion(releaseDescriptor, projectId))) {
logInfo(result, " Updating " + artifactId + " to " + mappedVersion);
coordinate.setVersion(mappedVersion);
} else {
logInfo(result, " Ignoring artifact version update for expression " + rawVersion);
}
} else if (properties != null) {
// version is an expression, check for properties to update instead

String propertyValue = properties.getProperty(expression);

if (propertyValue != null) {
if (propertyValue.equals(originalVersion)) {
logInfo(result, " Updating " + rawVersion + " to " + mappedVersion);
// change the property only if the property is the same as what's in the reactor
properties.setProperty(expression, mappedVersion);
} else if (mappedVersion.equals(propertyValue)) {
// this property may have been updated during processing a sibling.
logInfo(
result,
" Ignoring artifact version update for expression " + rawVersion
+ " because it is already updated");
} else if (!mappedVersion.equals(rawVersion)) {
// WARNING: ${pom.*} prefix support and ${version} is about to be dropped in mvn4!
// https://issues.apache.org/jira/browse/MNG-7404
// https://issues.apache.org/jira/browse/MNG-7244
if (mappedVersion.matches("\\$\\{project.+\\}")
|| mappedVersion.matches("\\$\\{pom.+\\}")
|| "${version}".equals(mappedVersion)) {
logInfo(
result,
" Ignoring artifact version update for CI friendly expression "
+ rawVersion);
" Ignoring artifact version update for expression " + mappedVersion);
// ignore... we cannot update this expression
} else {
// the expression used to define the version of this artifact may be inherited
// TODO needs a better error message, what pom? what dependency?
throw new ReleaseFailureException(
"Could not find property resolving version expression: " + rawVersion);
// the value of the expression conflicts with what the user wanted to release
throw new ReleaseFailureException("The artifact (" + key + ") requires a "
+ "different version (" + mappedVersion + ") than what is found ("
+ propertyValue + ") for the expression (" + expression + ") in the "
+ "project (" + projectId + ").");
}
}
} else {
// the expression used to define the version of this artifact may be inherited
// TODO needs a better error message, what pom? what dependency?
throw new ReleaseFailureException(
"Could not find properties resolving version expression : " + rawVersion);
throw new ReleaseFailureException("The version could not be updated: " + rawVersion);
}
} else {
// different/previous version not related to current release
}
} else {
// different/previous version not related to current release
}
} else if (resolvedSnapshotVersion != null) {
logInfo(result, " Updating " + artifactId + " to " + resolvedSnapshotVersion);
Expand Down
17 changes: 2 additions & 15 deletions ...ease-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.apache.maven.model.Profile;
import org.apache.maven.model.Reporting;
import org.apache.maven.model.Scm;
import org.apache.maven.shared.release.phase.AbstractRewritePomsPhase;
import org.jdom2.Document;
import org.jdom2.Element;
import org.jdom2.Text;
Expand Down Expand Up @@ -179,9 +178,7 @@ public void setVersion(String version) {
}

if (versionElement == null) {
// never add version when parent references CI friendly property
if (!(parentVersion != null && AbstractRewritePomsPhase.isCiFriendlyVersion(parentVersion))
&& !version.equals(parentVersion)) {
if (!version.equals(parentVersion)) {
// we will add this after artifactId, since it was missing but different from the inherited version
Element artifactIdElement = project.getChild("artifactId", project.getNamespace());
int index = project.indexOf(artifactIdElement);
Expand All @@ -192,17 +189,7 @@ public void setVersion(String version) {
project.addContent(index + 2, versionElement);
}
} else {
if (AbstractRewritePomsPhase.isCiFriendlyVersion(versionElement.getTextNormalize())) {
// try to rewrite property if CI friendly expression is used
String ciFriendlyPropertyName =
AbstractRewritePomsPhase.extractPropertyFromExpression(versionElement.getTextNormalize());
Properties properties = getProperties();
if (properties != null) {
properties.computeIfPresent(ciFriendlyPropertyName, (k, v) -> version);
}
} else {
JDomUtils.rewriteValue(versionElement, version);
}
JDomUtils.rewriteValue(versionElement, version);
}
}
}
2 changes: 1 addition & 1 deletion ...ase-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomParent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public JDomParent(Element parent) {

@Override
public String getVersion() {
return parent.getChildText("version", parent.getNamespace());
throw new UnsupportedOperationException();
}

@Override
Expand Down
14 changes: 0 additions & 14 deletions ...r/src/test/java/org/apache/maven/shared/release/phase/RewritePomsForReleasePhaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,20 +320,6 @@ public void testRewritePomWithParentAndProperties() throws Exception {
assertTrue(comparePomFiles(reactorProjects));
}

// MRELEASE-1109
@Test
public void testRewritePomWithCiFriendlyReactor() throws Exception {
List<MavenProject> reactorProjects = createReactorProjects("pom-with-parent-and-cifriendly-expressions");

ReleaseDescriptorBuilder builder =
createDescriptorFromProjects(reactorProjects, "pom-with-parent-and-cifriendly-expressions");
builder.addReleaseVersion("groupId:artifactId", NEXT_VERSION);
builder.addReleaseVersion("groupId:subproject1", NEXT_VERSION);
phase.execute(ReleaseUtils.buildReleaseDescriptor(builder), new DefaultReleaseEnvironment(), reactorProjects);

assertTrue(comparePomFiles(reactorProjects));
}

// MRELEASE-311
@Test
public void testRewritePomWithDependencyPropertyCoordinate() throws Exception {
Expand Down
9 changes: 1 addition & 8 deletions ...-manager/src/test/java/org/apache/maven/shared/release/transform/jdom2/JDomModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,7 @@ public void testSetVersion() throws Exception {
model.setVersion(null);
assertNull(model.getVersion());

// inherit from parent via CI friendly
content = "<project><parent><version>${revision}</version></parent></project>";
projectElm = builder.build(new StringReader(content)).getRootElement();
model = new JDomModel(projectElm);
assertNull(model.getVersion());
model.setVersion("PARENT_VERSION");
assertNull(getVersion(projectElm));

// inherit from parent
// this business logic might need to moved.
content = "<project><parent><version>PARENT_VERSION</version></parent></project>";
projectElm = builder.build(new StringReader(content)).getRootElement();
Expand Down
11 changes: 3 additions & 8 deletions ...manager/src/test/java/org/apache/maven/shared/release/transform/jdom2/JDomParentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@
*/
package org.apache.maven.shared.release.transform.jdom2;

import java.io.IOException;
import java.io.StringReader;

import org.jdom2.Element;
import org.jdom2.JDOMException;
import org.jdom2.input.SAXBuilder;
import org.junit.Test;

Expand All @@ -46,12 +44,9 @@ public void testGetRelativePath() {
new JDomParent(null).getRelativePath();
}

@Test
public void testGetVersion() throws JDOMException, IOException {
String content = "<parent><version>1.0</version></parent>";
Element parentElm = builder.build(new StringReader(content)).getRootElement();

assertEquals("1.0", new JDomParent(parentElm).getVersion());
@Test(expected = UnsupportedOperationException.class)
public void testGetVersion() {
new JDomParent(null).getVersion();
}

@Test(expected = UnsupportedOperationException.class)
Expand Down
39 changes: 0 additions & 39 deletions .../projects/rewrite-for-release/pom-with-parent-and-cifriendly-expressions/expected-pom.xml
View file
Open in desktop

This file was deleted.

Loading