This is an automated email from the ASF dual-hosted git repository.
cstamas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven.git
The following commit(s) were added to refs/heads/master by this push:
new 793443920e [IT] Refactor executor (#2175)
793443920e is described below
commit 793443920ebc040aff09b90cfb002737a53aa733
Author: Tamas Cservenak <ta...@cservenak.net>
AuthorDate: Fri Mar 21 14:30:15 2025 +0100
[IT] Refactor executor (#2175)
There was overlapping concerns, so better to separate them. Also, this now
allows to use tool with other maven that the one being tested, so result should
not be affected.
Also contains fix for ExecutorTool implementation to enforce that process
do produce output, as under some circumstances (on Windows) the process may
exit with 0 exit code (no error) but produce empty output
on standard output, which should not be possible.
---
.../maven/cling/executor/ExecutorHelper.java | 2 +-
.../maven/cling/executor/internal/HelperImpl.java | 26 ----------------------
.../maven/cling/executor/internal/ToolboxTool.java | 25 ++++++++++++++++-----
.../{HelperImplTest.java => ToolboxToolTest.java} | 21 +++++++++--------
.../main/java/org/apache/maven/it/Verifier.java | 21 ++++++++++-------
5 files changed, 45 insertions(+), 50 deletions(-)
diff --git
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java
index 14a5e2abd2..c6dc27feb0 100644
---
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java
+++
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java
@@ -26,7 +26,7 @@
/**
* Helper class for routing Maven execution based on preferences and/or issued
execution requests.
*/
-public interface ExecutorHelper extends ExecutorTool {
+public interface ExecutorHelper {
/**
* The modes of execution.
*/
diff --git
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java
index 4304f6d871..9a94ddc2dd 100644
---
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java
+++
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java
@@ -21,7 +21,6 @@
import java.nio.file.Path;
import java.util.Collections;
import java.util.HashMap;
-import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import org.apache.maven.api.annotations.Nullable;
@@ -29,7 +28,6 @@
import org.apache.maven.api.cli.ExecutorException;
import org.apache.maven.api.cli.ExecutorRequest;
import org.apache.maven.cling.executor.ExecutorHelper;
-import org.apache.maven.cling.executor.ExecutorTool;
import static java.util.Objects.requireNonNull;
@@ -40,7 +38,6 @@ public class HelperImpl implements ExecutorHelper {
private final Mode defaultMode;
private final Path installationDirectory;
private final Path userHomeDirectory;
- private final ExecutorTool executorTool;
private final HashMap<Mode, Executor> executors;
private final ConcurrentHashMap<String, String> cache;
@@ -58,7 +55,6 @@ public HelperImpl(
this.userHomeDirectory = userHomeDirectory != null
? ExecutorRequest.getCanonicalPath(userHomeDirectory)
: ExecutorRequest.discoverUserHomeDirectory();
- this.executorTool = new ToolboxTool(this);
this.executors = new HashMap<>();
this.executors.put(Mode.EMBEDDED, requireNonNull(embedded,
"embedded"));
@@ -89,28 +85,6 @@ public String mavenVersion() {
});
}
- @Override
- public Map<String, String> dump(ExecutorRequest.Builder request) throws
ExecutorException {
- return executorTool.dump(request);
- }
-
- @Override
- public String localRepository(ExecutorRequest.Builder request) throws
ExecutorException {
- return executorTool.localRepository(request);
- }
-
- @Override
- public String artifactPath(ExecutorRequest.Builder request, String gav,
String repositoryId)
- throws ExecutorException {
- return executorTool.artifactPath(request, gav, repositoryId);
- }
-
- @Override
- public String metadataPath(ExecutorRequest.Builder request, String gav,
String repositoryId)
- throws ExecutorException {
- return executorTool.metadataPath(request, gav, repositoryId);
- }
-
protected Executor getExecutor(Mode mode, ExecutorRequest request) throws
ExecutorException {
return switch (mode) {
case AUTO -> getExecutorByRequest(request);
diff --git
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java
index b23a4948a3..2c08093ce6 100644
---
a/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java
+++
b/impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java
@@ -59,7 +59,8 @@ public Map<String, String> dump(ExecutorRequest.Builder
executorRequest) throws
doExecute(builder);
try {
Properties properties = new Properties();
- properties.load(new ByteArrayInputStream(stdout.toByteArray()));
+ properties.load(new ByteArrayInputStream(
+ validateOutput(false, stdout, stderr).getBytes()));
return properties.entrySet().stream()
.collect(Collectors.toMap(
e -> String.valueOf(e.getKey()),
@@ -79,7 +80,7 @@ public String localRepository(ExecutorRequest.Builder
executorRequest) throws Ex
.stdOut(stdout)
.stdErr(stderr);
doExecute(builder);
- return shaveStdout(stdout);
+ return validateOutput(true, stdout, stderr);
}
@Override
@@ -95,7 +96,7 @@ public String artifactPath(ExecutorRequest.Builder
executorRequest, String gav,
builder.argument("-Drepository=" + repositoryId + "::unimportant");
}
doExecute(builder);
- return shaveStdout(stdout);
+ return validateOutput(true, stdout, stderr);
}
@Override
@@ -111,7 +112,7 @@ public String metadataPath(ExecutorRequest.Builder
executorRequest, String gav,
builder.argument("-Drepository=" + repositoryId + "::unimportant");
}
doExecute(builder);
- return shaveStdout(stdout);
+ return validateOutput(true, stdout, stderr);
}
private ExecutorRequest.Builder mojo(ExecutorRequest.Builder builder,
String mojo) {
@@ -131,7 +132,19 @@ private void doExecute(ExecutorRequest.Builder builder) {
}
}
- private String shaveStdout(ByteArrayOutputStream stdout) {
- return stdout.toString().replace("\n", "").replace("\r", "");
+ /**
+ * Performs "sanity check" for output, making sure no insane values like
empty strings are returned.
+ */
+ private String validateOutput(boolean shave, ByteArrayOutputStream stdout,
ByteArrayOutputStream stderr) {
+ String result = stdout.toString();
+ if (shave) {
+ result = result.replace("\n", "").replace("\r", "");
+ }
+ // sanity checks: stderr has any OR result is empty string (no method
should emit empty string)
+ if (stderr.size() > 0 || result.trim().isEmpty()) {
+ throw new ExecutorException(
+ "Unexpected stdout[" + stdout.size() + "]=" + stdout + ";
stderr[" + stderr.size() + "]=" + stderr);
+ }
+ return result;
}
}
diff --git
a/impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/HelperImplTest.java
b/impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/ToolboxToolTest.java
similarity index 88%
rename from
impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/HelperImplTest.java
rename to
impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/ToolboxToolTest.java
index ee4c70bd20..d34fc00f08 100644
---
a/impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/HelperImplTest.java
+++
b/impl/maven-executor/src/test/java/org/apache/maven/cling/executor/impl/ToolboxToolTest.java
@@ -27,6 +27,7 @@
import org.apache.maven.cling.executor.ExecutorHelper;
import org.apache.maven.cling.executor.MavenExecutorTestSupport;
import org.apache.maven.cling.executor.internal.HelperImpl;
+import org.apache.maven.cling.executor.internal.ToolboxTool;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.api.io.TempDir;
@@ -38,7 +39,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
-public class HelperImplTest {
+public class ToolboxToolTest {
@TempDir
private static Path userHome;
@@ -52,7 +53,7 @@ void dump3(ExecutorHelper.Mode mode) throws Exception {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- Map<String, String> dump = helper.dump(helper.executorRequest());
+ Map<String, String> dump = new
ToolboxTool(helper).dump(helper.executorRequest());
assertEquals(System.getProperty("maven3version"),
dump.get("maven.version"));
}
@@ -66,7 +67,7 @@ void dump4(ExecutorHelper.Mode mode) throws Exception {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- Map<String, String> dump = helper.dump(helper.executorRequest());
+ Map<String, String> dump = new
ToolboxTool(helper).dump(helper.executorRequest());
assertEquals(System.getProperty("maven4version"),
dump.get("maven.version"));
}
@@ -106,7 +107,7 @@ void localRepository3(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String localRepository =
helper.localRepository(helper.executorRequest());
+ String localRepository = new
ToolboxTool(helper).localRepository(helper.executorRequest());
Path local = Paths.get(localRepository);
assertTrue(Files.isDirectory(local));
}
@@ -122,7 +123,7 @@ void localRepository4(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String localRepository =
helper.localRepository(helper.executorRequest());
+ String localRepository = new
ToolboxTool(helper).localRepository(helper.executorRequest());
Path local = Paths.get(localRepository);
assertTrue(Files.isDirectory(local));
}
@@ -137,7 +138,8 @@ void artifactPath3(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String path = helper.artifactPath(helper.executorRequest(),
"aopalliance:aopalliance:1.0", "central");
+ String path = new ToolboxTool(helper)
+ .artifactPath(helper.executorRequest(),
"aopalliance:aopalliance:1.0", "central");
// split repository: assert "ends with" as split may introduce prefixes
assertTrue(
path.endsWith("aopalliance" + File.separator + "aopalliance" +
File.separator + "1.0" + File.separator
@@ -155,7 +157,8 @@ void artifactPath4(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String path = helper.artifactPath(helper.executorRequest(),
"aopalliance:aopalliance:1.0", "central");
+ String path = new ToolboxTool(helper)
+ .artifactPath(helper.executorRequest(),
"aopalliance:aopalliance:1.0", "central");
// split repository: assert "ends with" as split may introduce prefixes
assertTrue(
path.endsWith("aopalliance" + File.separator + "aopalliance" +
File.separator + "1.0" + File.separator
@@ -173,7 +176,7 @@ void metadataPath3(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String path = helper.metadataPath(helper.executorRequest(),
"aopalliance", "someremote");
+ String path = new
ToolboxTool(helper).metadataPath(helper.executorRequest(), "aopalliance",
"someremote");
// split repository: assert "ends with" as split may introduce prefixes
assertTrue(path.endsWith("aopalliance" + File.separator +
"maven-metadata-someremote.xml"), "path=" + path);
}
@@ -188,7 +191,7 @@ void metadataPath4(ExecutorHelper.Mode mode) {
userHome,
MavenExecutorTestSupport.EMBEDDED_MAVEN_EXECUTOR,
MavenExecutorTestSupport.FORKED_MAVEN_EXECUTOR);
- String path = helper.metadataPath(helper.executorRequest(),
"aopalliance", "someremote");
+ String path = new
ToolboxTool(helper).metadataPath(helper.executorRequest(), "aopalliance",
"someremote");
// split repository: assert "ends with" as split may introduce prefixes
assertTrue(path.endsWith("aopalliance" + File.separator +
"maven-metadata-someremote.xml"), "path=" + path);
}
diff --git
a/its/core-it-support/maven-it-helper/src/main/java/org/apache/maven/it/Verifier.java
b/its/core-it-support/maven-it-helper/src/main/java/org/apache/maven/it/Verifier.java
index 1c9406fcc1..04be2c6dbf 100644
---
a/its/core-it-support/maven-it-helper/src/main/java/org/apache/maven/it/Verifier.java
+++
b/its/core-it-support/maven-it-helper/src/main/java/org/apache/maven/it/Verifier.java
@@ -48,9 +48,11 @@
import org.apache.maven.api.cli.ExecutorException;
import org.apache.maven.api.cli.ExecutorRequest;
import org.apache.maven.cling.executor.ExecutorHelper;
+import org.apache.maven.cling.executor.ExecutorTool;
import org.apache.maven.cling.executor.embedded.EmbeddedMavenExecutor;
import org.apache.maven.cling.executor.forked.ForkedMavenExecutor;
import org.apache.maven.cling.executor.internal.HelperImpl;
+import org.apache.maven.cling.executor.internal.ToolboxTool;
import org.codehaus.plexus.util.FileUtils;
import org.codehaus.plexus.util.StringUtils;
@@ -88,6 +90,8 @@ public class Verifier {
private final ExecutorHelper executorHelper;
+ private final ExecutorTool executorTool;
+
private final Path basedir; // the basedir of IT
private final Path tempBasedir; // empty basedir for queries
@@ -145,6 +149,7 @@ public Verifier(String basedir, List<String>
defaultCliArguments) throws Verific
this.userHomeDirectory,
EMBEDDED_MAVEN_EXECUTOR,
FORKED_MAVEN_EXECUTOR);
+ this.executorTool = new ToolboxTool(executorHelper);
this.defaultCliArguments =
new ArrayList<>(defaultCliArguments != null ?
defaultCliArguments : DEFAULT_CLI_ARGUMENTS);
this.logFile = this.basedir.resolve(logFileName);
@@ -235,7 +240,7 @@ public void execute() throws VerificationException {
if (ret > 0) {
String dump;
try {
- dump = executorHelper.dump(request.toBuilder()).toString();
+ dump = executorTool.dump(request.toBuilder()).toString();
} catch (Exception e) {
dump = "FAILED: " + e.getMessage();
}
@@ -340,14 +345,14 @@ public String getLocalRepositoryWithSettings(String
settingsXml) {
if (!Files.isRegularFile(settingsFile)) {
throw new IllegalArgumentException("settings xml does not
exist: " + settingsXml);
}
- return executorHelper.localRepository(executorHelper
+ return executorTool.localRepository(executorHelper
.executorRequest()
.cwd(tempBasedir)
.userHomeDirectory(userHomeDirectory)
.argument("-s")
.argument(settingsFile.toString()));
} else {
- return executorHelper.localRepository(
+ return executorTool.localRepository(
executorHelper.executorRequest().cwd(tempBasedir).userHomeDirectory(userHomeDirectory));
}
}
@@ -644,7 +649,7 @@ public String getArtifactPath(String gid, String aid,
String version, String ext
}
return getLocalRepository()
+ File.separator
- +
executorHelper.artifactPath(executorHelper.executorRequest(), gav, null);
+ + executorTool.artifactPath(executorHelper.executorRequest(),
gav, null);
}
private String getSupportArtifactPath(String artifact) {
@@ -701,7 +706,7 @@ public String getSupportArtifactPath(String gid, String
aid, String version, Str
gav = gid + ":" + aid + ":" + ext + ":" + version;
}
return outerLocalRepository
- .resolve(executorHelper.artifactPath(
+ .resolve(executorTool.artifactPath(
executorHelper.executorRequest().argument("-Dmaven.repo.local=" +
outerLocalRepository),
gav,
null))
@@ -776,7 +781,7 @@ public String getArtifactMetadataPath(String gid, String
aid, String version, St
gav += filename;
return getLocalRepository()
+ File.separator
- +
executorHelper.metadataPath(executorHelper.executorRequest(), gav, repoId);
+ + executorTool.metadataPath(executorHelper.executorRequest(),
gav, repoId);
}
/**
@@ -806,7 +811,7 @@ public void deleteArtifact(String org, String name, String
version, String ext)
* @since 1.2
*/
public void deleteArtifacts(String gid) throws IOException {
- String mdPath =
executorHelper.metadataPath(executorHelper.executorRequest(), gid, null);
+ String mdPath =
executorTool.metadataPath(executorHelper.executorRequest(), gid, null);
Path dir = Paths.get(getLocalRepository()).resolve(mdPath).getParent();
FileUtils.deleteDirectory(dir.toFile());
}
@@ -826,7 +831,7 @@ public void deleteArtifacts(String gid, String aid, String
version) throws IOExc
requireNonNull(version, "version is null");
String mdPath =
- executorHelper.metadataPath(executorHelper.executorRequest(),
gid + ":" + aid + ":" + version, null);
+ executorTool.metadataPath(executorHelper.executorRequest(),
gid + ":" + aid + ":" + version, null);
Path dir = Paths.get(getLocalRepository()).resolve(mdPath).getParent();
FileUtils.deleteDirectory(dir.toFile());
}
