Skip to content

[IT] Refactor executor #2175

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

Merged
cstamas merged 2 commits into from
Mar 21, 2025
Merged

[IT] Refactor executor #2175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa855a6
Refactor executor
cstamas Mar 21, 2025
92a2122
This was rename, do not leave junk behind
cstamas Mar 21, 2025
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
2 changes: 1 addition & 1 deletion impl/maven-executor/src/main/java/org/apache/maven/cling/executor/ExecutorHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
26 changes: 0 additions & 26 deletions impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/HelperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,13 @@
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;
import org.apache.maven.api.cli.Executor;
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;

Expand All @@ -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;
Expand All @@ -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"));
Expand Down Expand Up @@ -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);
Expand Down
25 changes: 19 additions & 6 deletions impl/maven-executor/src/main/java/org/apache/maven/cling/executor/internal/ToolboxTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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;
}
}
21 changes: 12 additions & 9 deletions ...n/cling/executor/impl/HelperImplTest.java → .../cling/executor/impl/ToolboxToolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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"));
}

Expand All @@ -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"));
}

Expand Down Expand Up @@ -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));
}
Expand All @@ -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));
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
21 changes: 13 additions & 8 deletions its/core-it-support/maven-it-helper/src/main/java/org/apache/maven/it/Verifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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));
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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());
}
Expand All @@ -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());
}
Expand Down