Skip to content

Do not add option "config-output-dir" when conflicting options are pr… #190

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

Closed
wants to merge 2 commits into from
Closed

Do not add option "config-output-dir" when conflicting options are pr… #190

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
6bc3e7e
Do not add option "config-output-dir" when conflicting options are pr…
hannesd Jan 6, 2022
e08907f
Wrapped agentOutputDirectoryFor with Provider
hannesd Jan 13, 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
37 changes: 36 additions & 1 deletion native-gradle-plugin/src/main/java/org/graalvm/buildtools/gradle/NativeImagePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

package org.graalvm.buildtools.gradle;

import java.util.List;
import org.graalvm.buildtools.VersionInfo;
import org.graalvm.buildtools.gradle.dsl.AgentConfiguration;
import org.graalvm.buildtools.gradle.dsl.GraalVMExtension;
Expand Down Expand Up @@ -74,6 +75,7 @@
import org.gradle.api.file.DuplicatesStrategy;
import org.gradle.api.file.FileCollection;
import org.gradle.api.file.FileSystemLocation;
import org.gradle.api.internal.provider.AbstractMinimalProvider;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.plugins.ApplicationPlugin;
import org.gradle.api.plugins.JavaApplication;
Expand All @@ -97,7 +99,9 @@

import javax.inject.Inject;
import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.Callable;
Expand Down Expand Up @@ -507,7 +511,7 @@ private static void configureAgent(Project project,
AgentCommandLineProvider cliProvider = project.getObjects().newInstance(AgentCommandLineProvider.class);
Provider<Boolean> agent = agents.get(nativeImageOptions.getName());
cliProvider.getEnabled().set(agent);
Provider<Directory> outputDir = project.getLayout().getBuildDirectory().dir(AGENT_OUTPUT_FOLDER + "/" + instrumentedTask.getName());
Provider<Directory> outputDir = agentOutputDirectoryFor(project, nativeImageOptions, instrumentedTask);
cliProvider.getOutputDirectory().set(outputDir);
cliProvider.getAgentOptions().set(nativeImageOptions.getAgent().getOptions());
instrumentedTask.get().getJvmArgumentProviders().add(cliProvider);
Expand All @@ -528,6 +532,37 @@ private static void configureAgent(Project project,
nativeImageOptions.getConfigurationFileDirectories().from(files);
}

private static Provider<Directory> agentOutputDirectoryFor(Project project, NativeImageOptions nativeImageOptions, TaskProvider<? extends JavaForkOptions> instrumentedTask) {
Copy link
Collaborator

@melix melix Jan 12, 2022

Choose a reason for hiding this comment

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

This isn't correct, as you are effectively eagerly reading the provider (orElse(AGENT_...), which defeats the laziness.

Copy link
Author

@hannesd hannesd Jan 13, 2022

Choose a reason for hiding this comment

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

It's my first time working with gradle, so I might be mistaken, but as far as I can tell, there are no lazy input values to the computation of the "outputDirUnresolved" variable (the part of the code ending with orElse(AGENT_...). All the values involved come from the plugin's configuration which I expect to be constant from start to finish. And since .getBuildDirectory().dir(outputDirUnresolved) returns a provider I would assume that whatever it does with outputDirUnresolved will be done lazily.

In case I'm missing something, I have pushed another version where the whole computation is wrapped into one of the Provider implementations I found in gradle's plugin code.

Copy link
Collaborator

@melix melix Feb 1, 2022

Choose a reason for hiding this comment

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

❌ this isn't correct either. You should not implement your own provider type. I will take a stab at it once the CLA issues are fixed.

return new AbstractMinimalProvider<Directory>() {
@Override
public Class<Directory> getType() {
return Directory.class;
}

@Override
protected Value<? extends Directory> calculateOwnValue(ValueConsumer consumer) {
final List<String> options = nativeImageOptions.getAgent().getOptions().getOrElse(Collections.emptyList());
final String outputDirUnresolved = new ArrayList<>(options).stream()
.filter(option -> option.startsWith("config-output-dir"))
.findFirst()
.map(option -> {
final int firstEqualsPos = option.indexOf('=');
if (firstEqualsPos == -1) {
throw new IllegalArgumentException("agent option 'config-output-dir' is missing its value assignment '=...'.");
}
final String path = option.substring(firstEqualsPos + 1).trim();
if (path.isEmpty()) {
throw new IllegalArgumentException("value of agent option 'config-output-dir' must not be empty.");
}
return path;
})
.orElse(AGENT_OUTPUT_FOLDER + "/" + instrumentedTask.getName());

return Value.of(project.getLayout().getBuildDirectory().dir(outputDirUnresolved).get());
}
};
}

private static void injectTestPluginDependencies(Project project, Property<Boolean> testSupportEnabled) {
project.afterEvaluate(p -> {
if (testSupportEnabled.get()) {
Expand Down
10 changes: 7 additions & 3 deletions ...plugin/src/main/java/org/graalvm/buildtools/gradle/internal/AgentCommandLineProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,14 @@ public Iterable<String> asArguments() {
if (getEnabled().get()) {
File outputDir = getOutputDirectory().getAsFile().get();
List<String> agentOptions = new ArrayList<>(getAgentOptions().getOrElse(Collections.emptyList()));
if (agentOptions.stream().map(s -> s.split("=")[0]).anyMatch(s -> s.contains("config-output-dir"))) {
throw new IllegalStateException("config-output-dir cannot be supplied as an agent option");

// Do not add config-output-dir when a conflicting option is already present. Otherwise, this happens:
// native-image-agent: can only once specify exactly one of trace-output=, config-output-dir= or config-merge-dir=.
final List<String> mutuallyExclusiveOptions = Arrays.asList("config-output-dir", "trace-output", "config-merge-dir");
if (agentOptions.stream().allMatch(option -> mutuallyExclusiveOptions.stream().noneMatch(option::startsWith))) {
agentOptions.add("config-output-dir=" + outputDir.getAbsolutePath());
}
agentOptions.add("config-output-dir=" + outputDir.getAbsolutePath());

return Arrays.asList(
"-agentlib:native-image-agent=" + String.join(",", agentOptions),
"-Dorg.graalvm.nativeimage.imagecode=agent"
Expand Down
42 changes: 32 additions & 10 deletions native-maven-plugin/src/main/java/org/graalvm/buildtools/maven/NativeExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@

package org.graalvm.buildtools.maven;

import static org.graalvm.buildtools.utils.SharedConstants.AGENT_OUTPUT_FOLDER;

import org.apache.maven.AbstractMavenLifecycleParticipant;
import org.apache.maven.MavenExecutionException;
import org.apache.maven.execution.MavenSession;
Expand All @@ -55,6 +57,7 @@

import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.function.BiConsumer;
Expand Down Expand Up @@ -85,12 +88,35 @@ static String testIdsDirectory(String baseDir) {

static String buildAgentArgument(String baseDir, Context context, List<String> agentOptions) {
List<String> options = new ArrayList<>(agentOptions);
options.add("config-output-dir=" + agentOutputDirectoryFor(baseDir, context));
// Do not add config-output-dir when a conflicting option is already present. Otherwise, this happens:
// native-image-agent: can only once specify exactly one of trace-output=, config-output-dir= or config-merge-dir=.
final List<String> mutuallyExclusiveOptions = Arrays.asList("config-output-dir", "trace-output", "config-merge-dir");
if (agentOptions.stream().allMatch(option -> mutuallyExclusiveOptions.stream().noneMatch(option::startsWith))) {
options.add("config-output-dir=" + agentDefaultOutputDirectoryFor(baseDir, context));
}
return "-agentlib:native-image-agent=" + String.join(",", options);
}

private static String agentOutputDirectoryFor(String baseDir, Context context) {
return (baseDir + "/native/agent-output/" + context).replace('/', File.separatorChar);
private static String agentDefaultOutputDirectoryFor(String baseDir, Context context) {
return (baseDir + "/" + AGENT_OUTPUT_FOLDER + "/" + context).replace('/', File.separatorChar);
}

private static String agentOutputDirectoryFor(List<String> agentOptions, String baseDir, Context context) {
return agentOptions.stream()
.filter(option -> option.startsWith("config-output-dir"))
.findFirst()
.map(option -> {
int firstEqualsPos = option.indexOf('=');
if (firstEqualsPos == -1) {
throw new IllegalArgumentException("agent option 'config-output-dir' is missing its value assignment '=...'.");
}
final String path = option.substring(firstEqualsPos + 1).trim();
if (path.isEmpty()) {
throw new IllegalArgumentException("value of agent option 'config-output-dir' must not be empty.");
}
return path;
})
.orElseGet(() -> agentDefaultOutputDirectoryFor(baseDir, context));
}

@Override
Expand All @@ -114,6 +140,7 @@ public void afterProjectsRead(MavenSession session) throws MavenExecutionExcepti

// Main configuration
if (isAgentEnabled) {
List<String> agentOptions = getAgentOptions(nativePlugin, Context.main, selectedOptionsName);
withPlugin(build, "exec-maven-plugin", execPlugin ->
updatePluginConfiguration(execPlugin, (exec, config) -> {
if ("java-agent".equals(exec.getId())) {
Expand All @@ -127,7 +154,6 @@ public void afterProjectsRead(MavenSession session) throws MavenExecutionExcepti

// Agent argument
Xpp3Dom arg = new Xpp3Dom("argument");
List<String> agentOptions = getAgentOptions(nativePlugin, Context.main, selectedOptionsName);
arg.setValue(buildAgentArgument(target, Context.main, agentOptions));
children.add(0, arg);

Expand All @@ -151,7 +177,7 @@ public void afterProjectsRead(MavenSession session) throws MavenExecutionExcepti
updatePluginConfiguration(nativePlugin, (exec, configuration) -> {
Context context = exec.getGoals().stream().anyMatch("test"::equals) ? Context.test : Context.main;
Xpp3Dom agentResourceDirectory = findOrAppend(configuration, "agentResourceDirectory");
agentResourceDirectory.setValue(agentOutputDirectoryFor(target, context));
agentResourceDirectory.setValue(agentOutputDirectoryFor(agentOptions, target, context));
});
}
});
Expand Down Expand Up @@ -242,11 +268,7 @@ private static List<String> getAgentOptions(Plugin nativePlugin, Context context

private static void processOptionNodes(Xpp3Dom options, List<String> optionsList) {
for (Xpp3Dom option : options.getChildren("option")) {
String value = assertNotEmptyAndTrim(option.getValue(), "<option> must declare a value");
if (value.contains("config-output-dir")) {
throw new IllegalStateException("config-output-dir cannot be supplied as an agent option");
}
optionsList.add(value);
optionsList.add(assertNotEmptyAndTrim(option.getValue(), "<option> must declare a value"));
}
}

Expand Down