Skip to content

Commit

Permalink
[GR-47186] Fix native-image --add-modules.
Browse files Browse the repository at this point in the history 
PullRequest: graal/15069
  • Loading branch information
@olpaw @chumer
olpaw authored and chumer committed Jul 28, 2023
2 parents 1ee928c + e4e4dc5 commit dd23914
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 16 deletions.
2 changes: 0 additions & 2 deletions substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/DefaultOptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public boolean consume(ArgumentQueue args) {
if (addModulesArgs == null) {
NativeImage.showError(headArg + moduleSetModifierOptionErrorMessage);
}
nativeImage.addImageBuilderJavaArgs(addModulesOption, addModulesArgs);
nativeImage.addAddedModules(addModulesArgs);
return true;
case limitModulesOption:
Expand Down Expand Up @@ -189,7 +188,6 @@ public boolean consume(ArgumentQueue args) {
if (addModulesArgs.isEmpty()) {
NativeImage.showError(headArg + moduleSetModifierOptionErrorMessage);
}
nativeImage.addImageBuilderJavaArgs(addModulesOption, addModulesArgs);
nativeImage.addAddedModules(addModulesArgs);
return true;
}
Expand Down
2 changes: 2 additions & 0 deletions substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/MacroOptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ private void applyEnabled(MacroOption.EnabledOption enabledOption, String argume

enabledOption.forEachPropertyValue(config,
"ImageBuilderClasspath", entry -> nativeImage.addImageBuilderClasspath(Path.of(entry)), PATH_SEPARATOR_REGEX);
enabledOption.forEachPropertyValue(config,
"ImageBuilderModulePath", entry -> nativeImage.addImageBuilderModulePath(Path.of(entry)), PATH_SEPARATOR_REGEX);
boolean explicitImageModulePath = enabledOption.forEachPropertyValue(config,
"ImageModulePath", entry -> nativeImage.addImageModulePath(Path.of((entry))), PATH_SEPARATOR_REGEX);
boolean explicitImageClasspath = enabledOption.forEachPropertyValue(config,
Expand Down
140 changes: 126 additions & 14 deletions substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.io.InputStream;
import java.io.InputStreamReader;
import java.lang.reflect.Method;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
Expand All @@ -44,10 +45,12 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;
import java.util.Set;
Expand All @@ -56,6 +59,7 @@
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.jar.Attributes;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
Expand Down Expand Up @@ -1178,9 +1182,6 @@ private int completeImageBuild() {
return ExitStatus.FALLBACK_IMAGE.getValue();
}

if (!addModules.isEmpty()) {
imageBuilderJavaArgs.add("-D" + ModuleSupport.PROPERTY_IMAGE_EXPLICITLY_ADDED_MODULES + "=" + String.join(",", addModules));
}
if (!limitModules.isEmpty()) {
imageBuilderJavaArgs.add("-D" + ModuleSupport.PROPERTY_IMAGE_EXPLICITLY_LIMITED_MODULES + "=" + String.join(",", limitModules));
}
Expand Down Expand Up @@ -1425,20 +1426,13 @@ protected int buildImage(List<String> javaArgs, LinkedHashSet<Path> cp, LinkedHa
if (!cp.isEmpty()) {
arguments.addAll(Arrays.asList("-cp", cp.stream().map(Path::toString).collect(Collectors.joining(File.pathSeparator))));
}

if (!mp.isEmpty()) {
List<String> strings = Arrays.asList("--module-path", mp.stream().map(Path::toString).collect(Collectors.joining(File.pathSeparator)));
arguments.addAll(strings);
}

arguments.addAll(config.getGeneratorMainClass());

if (IS_AOT && OS.getCurrent().hasProcFS) {
/*
* GR-8254: Ensure image-building VM shuts down even if native-image dies unexpected
* (e.g. using CTRL-C in Gradle daemon mode)
*/
arguments.addAll(Arrays.asList(SubstrateOptions.WATCHPID_PREFIX, "" + ProcessProperties.getProcessID()));
}
String javaExecutable = canonicalize(config.getJavaExecutable()).toString();

if (useBundle()) {
LogUtils.warning("Native Image Bundles are an experimental feature.");
Expand All @@ -1450,12 +1444,68 @@ protected int buildImage(List<String> javaArgs, LinkedHashSet<Path> cp, LinkedHa
Function<Path, Path> substituteClassPath = useBundle() ? bundleSupport::substituteClassPath : Function.identity();
List<Path> finalImageClassPath = imagecp.stream().map(substituteClassPath).collect(Collectors.toList());
Function<Path, Path> substituteModulePath = useBundle() ? bundleSupport::substituteModulePath : Function.identity();
List<Path> finalImageModulePath = imagemp.stream().map(substituteModulePath).collect(Collectors.toList());
List<Path> substitutedImageModulePath = imagemp.stream().map(substituteModulePath).toList();

Map<String, Path> modules = listModulesFromPath(javaExecutable, Stream.concat(mp.stream(), imagemp.stream()).distinct().toList());
if (!addModules.isEmpty()) {

arguments.add("-D" + ModuleSupport.PROPERTY_IMAGE_EXPLICITLY_ADDED_MODULES + "=" +
String.join(",", addModules));

List<String> addModulesForBuilderVM = new ArrayList<>();
for (String module : addModules) {
Path jarPath = modules.get(module);
if (jarPath == null) {
// boot module
addModulesForBuilderVM.add(module);
}
}

if (!addModulesForBuilderVM.isEmpty()) {
arguments.add(DefaultOptionHandler.addModulesOption + "=" + String.join(",", addModulesForBuilderVM));
}
}

arguments.addAll(config.getGeneratorMainClass());

if (IS_AOT && OS.getCurrent().hasProcFS) {
/*
* GR-8254: Ensure image-building VM shuts down even if native-image dies unexpected
* (e.g. using CTRL-C in Gradle daemon mode)
*/
arguments.addAll(Arrays.asList(SubstrateOptions.WATCHPID_PREFIX, "" + ProcessProperties.getProcessID()));
}

/*
* Workaround for GR-47186: Native image cannot handle modules on the image module path,
* that are also already installed in the JDK as boot module. As a workaround we filter all
* modules from the module-path that are either already installed in the JDK as boot module,
* or were explicitly added to the builder module-path.
*
* First compute all module-jar paths that are not on the builder module-path.
*/
Set<Path> nonBuilderModulePaths = modules.values().stream()
.filter(Objects::nonNull)
.filter(Predicate.not(mp::contains))
.collect(Collectors.toSet());

/*
* Now we need to filter the substituted module path list for all the modules that may
* remain on the module-path.
*
* This should normally not be necessary, as the nonBuilderModulePaths should already be the
* set of jar files for the image module path. Nevertheless, we use the original definition
* of the module path to preserve the order of the original module path and as a precaution
* to protect against --list-modules returning too many modules.
*/
List<Path> finalImageModulePath = substitutedImageModulePath.stream()
.filter(nonBuilderModulePaths::contains)
.toList();

List<String> finalImageBuilderArgs = createImageBuilderArgs(finalImageArgs, finalImageClassPath, finalImageModulePath);

/* Construct ProcessBuilder command from final arguments */
List<String> command = new ArrayList<>();
String javaExecutable = canonicalize(config.getJavaExecutable()).toString();
command.add(javaExecutable);
command.add(createVMInvocationArgumentFile(arguments));
command.add(createImageBuilderArgumentFile(finalImageBuilderArgs));
Expand Down Expand Up @@ -1520,6 +1570,68 @@ protected int buildImage(List<String> javaArgs, LinkedHashSet<Path> cp, LinkedHa
}
}

/**
* Resolves and lists all modules given a module path.
*
* @see #callListModules(String, List)
*/
private static Map<String, Path> listModulesFromPath(String javaExecutable, Collection<Path> modulePath) {
if (modulePath.isEmpty()) {
return Map.of();
}
String modulePathEntries = modulePath.stream()
.map(Path::toString)
.collect(Collectors.joining(File.pathSeparator));
return callListModules(javaExecutable, List.of("-p", modulePathEntries));
}

/**
* Calls <code>java $arguments --list-modules</code> to list all modules and parse the output.
* The output consists of a map with module name as key and {@link Path} to jar file if the
* module is not installed as part of the JDK. If the module is installed as part of the
* jdk/boot-layer then a <code>null</code> path will be returned.
* <p>
* This is a much more robust solution then trying to parse the JDK file structure manually.
*/
private static Map<String, Path> callListModules(String javaExecutable, List<String> arguments) {
Process listModulesProcess = null;
Map<String, Path> result = new LinkedHashMap<>();
try {
var pb = new ProcessBuilder(javaExecutable);
pb.command().addAll(arguments);
pb.command().add("--list-modules");
pb.environment().clear();
listModulesProcess = pb.start();
try (var br = new BufferedReader(new InputStreamReader(listModulesProcess.getInputStream()))) {
while (true) {
var line = br.readLine();
if (line == null) {
break;
}
String[] splitString = StringUtil.split(line, " ", 3);
String[] splitModuleNameAndVersion = StringUtil.split(splitString[0], "@", 2);
Path externalPath = null;
if (splitString.length > 1) {
String pathURI = splitString[1]; // url: file://path/to/file
externalPath = Path.of(URI.create(pathURI)).toAbsolutePath();
}
result.put(splitModuleNameAndVersion[0], externalPath);
}
}
int exitStatus = listModulesProcess.waitFor();
if (exitStatus != 0) {
throw showError("Determining image-builder observable modules failed (Exit status %d).".formatted(exitStatus));
}
} catch (IOException | InterruptedException e) {
throw showError(e.getMessage());
} finally {
if (listModulesProcess != null) {
listModulesProcess.destroy();
}
}
return result;
}

/**
* Adds a shutdown hook to kill the image builder process if it's still alive.
*
Expand Down

0 comments on commit dd23914

Please sign in to comment.