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

[GR-47186] Fix native-image --add-modules. #7076

Merged
merged 5 commits into from
Jul 28, 2023
Merged

[GR-47186] Fix native-image --add-modules. #7076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba3f3ad
Allow builder module path extension via macro options
olpaw Jul 20, 2023
d9b47fe
Only forward --add-modules arguments that refer to modules of the bui…
olpaw Jul 20, 2023
03cbf9b
Implement a filter for the application-module-path and image addmods …
chumer Jul 25, 2023
f361f03
Remove normalization to fix Windows; Handle an empty module-path prop…
chumer Jul 25, 2023
e4e4dc5
Provide original addModules list to builder
olpaw Jul 26, 2023
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: 0 additions & 2 deletions substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/DefaultOptionHandler.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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