Skip to content

Commit ca2039a

Browse files
olpawolpaw
committed
Support position-independent LayerVerification
1 parent b3ecc8b commit ca2039a

File tree

5 files changed

+78
-50
lines changed

5 files changed

+78
-50
lines changed

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/NativeImageClassLoaderOptions.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,18 @@ public class NativeImageClassLoaderOptions {
3737
public static final String AddExportsAndOpensFormat = "<module>/<package>=<target-module>(,<target-module>)*";
3838
public static final String AddReadsFormat = "<module>=<target-module>(,<target-module>)*";
3939

40-
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error)//
40+
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error, Positional = false)//
4141
@APIOption(name = "add-exports", extra = true, launcherOption = true, valueSeparator = {APIOption.WHITESPACE_SEPARATOR, '='})//
4242
@Option(help = "Value " + AddExportsAndOpensFormat + " updates <module> to export <package> to <target-module>, regardless of module declaration." +
4343
" <target-module> can be ALL-UNNAMED to export to all unnamed modules.")//
4444
public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> AddExports = new HostedOptionKey<>(AccumulatingLocatableMultiOptionValue.Strings.build());
4545

46-
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error)//
46+
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error, Positional = false)//
4747
@APIOption(name = "add-opens", extra = true, launcherOption = true, valueSeparator = {APIOption.WHITESPACE_SEPARATOR, '='})//
4848
@Option(help = "Value " + AddExportsAndOpensFormat + " updates <module> to open <package> to <target-module>, regardless of module declaration.")//
4949
public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> AddOpens = new HostedOptionKey<>(AccumulatingLocatableMultiOptionValue.Strings.build());
5050

51-
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error)//
51+
@LayerVerification(Kind = Kind.Removed, Severity = Severity.Error, Positional = false)//
5252
@APIOption(name = "add-reads", extra = true, valueSeparator = {APIOption.WHITESPACE_SEPARATOR, '='})//
5353
@Option(help = "Value " + AddReadsFormat + " updates <module> to read <target-module>, regardless of module declaration." +
5454
" <target-module> can be ALL-UNNAMED to read all unnamed modules.")//

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,9 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
173173
@BundleMember(role = Role.Input) //
174174
public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Paths> LayerUse = new HostedOptionKey<>(AccumulatingLocatableMultiOptionValue.Paths.build());
175175

176+
@Option(help = "Experimental: Perform strict checking of options used for layered image build.")//
177+
public static final HostedOptionKey<Boolean> LayerVerificationStrict = new HostedOptionKey<>(true);
178+
176179
@Option(help = "Mark singleton as application layer only")//
177180
public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> ApplicationLayerOnlySingletons = new HostedOptionKey<>(AccumulatingLocatableMultiOptionValue.Strings.build());
178181

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/LayerVerification.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,6 @@ enum Kind {
5959
Changed,
6060
Added
6161
}
62+
63+
boolean Positional() default true;
6264
}

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/HostedImageLayerBuildingSupport.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,8 @@ public static HostedImageLayerBuildingSupport initialize(HostedOptionValues valu
282282
if (buildingExtensionLayer) {
283283
Path layerFileName = getLayerUseValue(values);
284284
loadLayerArchiveSupport = new LoadLayerArchiveSupport(layerName, layerFileName, builderTempDir, archiveSupport);
285-
loadLayerArchiveSupport.verifyCompatibility(imageClassLoader.classLoaderSupport, collectLayerVerifications(imageClassLoader));
285+
boolean strict = SubstrateOptions.LayerVerificationStrict.getValue(values);
286+
loadLayerArchiveSupport.verifyCompatibility(imageClassLoader.classLoaderSupport, collectLayerVerifications(imageClassLoader), strict);
286287
try {
287288
graphsChannel = FileChannel.open(loadLayerArchiveSupport.getSnapshotGraphsPath());
288289

@@ -307,15 +308,21 @@ public static HostedImageLayerBuildingSupport initialize(HostedOptionValues valu
307308
return imageLayerBuildingSupport;
308309
}
309310

310-
public static Map<String, Map<LayerVerification.Kind, LayerVerification>> collectLayerVerifications(ImageClassLoader loader) {
311+
record OptionLayerVerificationRequests(OptionDescriptor option, Map<LayerVerification.Kind, LayerVerification> requests) {
312+
public OptionLayerVerificationRequests(OptionDescriptor option) {
313+
this(option, new HashMap<>());
314+
}
315+
}
316+
317+
public static Map<String, OptionLayerVerificationRequests> collectLayerVerifications(ImageClassLoader loader) {
311318
Iterable<OptionDescriptors> optionDescriptors = OptionsContainer.getDiscoverableOptions(loader.getClassLoader());
312319
EconomicMap<String, OptionDescriptor> hostedOptions = EconomicMap.create();
313320
EconomicMap<String, OptionDescriptor> runtimeOptions = EconomicMap.create();
314321
HostedOptionParser.collectOptions(optionDescriptors, hostedOptions, runtimeOptions);
315-
Map<String, Map<LayerVerification.Kind, LayerVerification>> result = new HashMap<>();
322+
Map<String, OptionLayerVerificationRequests> result = new HashMap<>();
316323
for (OptionDescriptor optionDescriptor : hostedOptions.getValues()) {
317324
for (LayerVerification layerVerification : OptionUtils.getAnnotationsByType(optionDescriptor, LayerVerification.class)) {
318-
result.computeIfAbsent(optionDescriptor.getName(), key -> new HashMap()).put(layerVerification.Kind(), layerVerification);
325+
result.computeIfAbsent(optionDescriptor.getName(), key -> new OptionLayerVerificationRequests(optionDescriptor)).requests.put(layerVerification.Kind(), layerVerification);
319326
}
320327
}
321328
return result;

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/LoadLayerArchiveSupport.java

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,19 @@
3030
import java.util.List;
3131
import java.util.Map;
3232
import java.util.Set;
33+
import java.util.function.Function;
3334
import java.util.stream.Stream;
3435

3536
import com.oracle.svm.core.option.LayerVerification;
3637
import com.oracle.svm.core.option.LayerVerification.Kind;
3738
import com.oracle.svm.core.option.OptionOrigin;
39+
import com.oracle.svm.core.option.SubstrateOptionsParser;
3840
import com.oracle.svm.core.util.ArchiveSupport;
3941
import com.oracle.svm.core.util.UserError;
42+
import com.oracle.svm.core.util.VMError;
4043
import com.oracle.svm.hosted.NativeImageClassLoaderSupport;
44+
import com.oracle.svm.hosted.imagelayer.HostedImageLayerBuildingSupport.OptionLayerVerificationRequests;
45+
import com.oracle.svm.hosted.imagelayer.LoadLayerArchiveSupport.ArgumentOrigin.NameValue;
4146
import com.oracle.svm.hosted.util.DiffTool;
4247
import com.oracle.svm.util.LogUtils;
4348

@@ -66,63 +71,74 @@ protected void validateLayerFile(Path layerFile) {
6671
}
6772
}
6873

69-
public void verifyCompatibility(NativeImageClassLoaderSupport classLoaderSupport, Map<String, Map<Kind, LayerVerification>> verifications) {
70-
71-
// var errorMessagePrefix = "Layer Compatibility Error: ";
72-
var strippedBuilderArguments = builderArguments.stream()
73-
.map(argument -> splitArgumentOrigin(argument).argument)
74-
.toList();
75-
List<String> currentBuilderArguments = classLoaderSupport.getHostedOptionParser().getArguments();
76-
var strippedCurrentBuilderArguments = currentBuilderArguments.stream()
77-
.map(argument -> splitArgumentOrigin(argument).argument)
78-
.toList();
74+
public void verifyCompatibility(NativeImageClassLoaderSupport classLoaderSupport, Map<String, OptionLayerVerificationRequests> allRequests, boolean strict) {
75+
Function<String, String> filterFunction = argument -> splitArgumentOrigin(argument).argument;
76+
verifyCompatibility(builderArguments, classLoaderSupport.getHostedOptionParser().getArguments(), filterFunction, allRequests, strict, true);
77+
verifyCompatibility(builderArguments, classLoaderSupport.getHostedOptionParser().getArguments(), filterFunction, allRequests, strict, false);
78+
}
7979

80-
var diffResults = DiffTool.diffResults(strippedBuilderArguments, strippedCurrentBuilderArguments);
80+
private static void verifyCompatibility(List<String> parentArgs, List<String> currentArgs, Function<String, String> filterFunction,
81+
Map<String, OptionLayerVerificationRequests> allRequests, boolean strict, boolean positional) {
8182

82-
if (Boolean.getBoolean("org.graalvm.nativeimage.layers.verification.verbose") && !diffResults.isEmpty()) {
83-
System.out.println("{".repeat(40));
84-
for (var diffResult : diffResults) {
85-
System.out.println(diffResult.toString(builderArguments, currentBuilderArguments));
86-
}
87-
System.out.println("}".repeat(40));
83+
List<String> left;
84+
List<String> right;
85+
if (positional) {
86+
left = parentArgs;
87+
right = currentArgs;
88+
} else {
89+
// Use sorted lists for position-independent diff results
90+
left = parentArgs.stream().sorted().toList();
91+
right = currentArgs.stream().sorted().toList();
8892
}
8993

94+
List<String> filteredLeft = left.stream().map(filterFunction).toList();
95+
List<String> filteredRight = right.stream().map(filterFunction).toList();
96+
var diffResults = DiffTool.diffResults(filteredLeft, filteredRight);
9097
for (var diffResult : diffResults) {
9198
Set<Kind> verificationKinds = switch (diffResult.kind()) {
9299
case Removed -> Set.of(Kind.Removed, Kind.Changed);
93100
case Added -> Set.of(Kind.Added, Kind.Changed);
94101
default -> Set.of();
95102
};
96103
for (Kind verificationKind : verificationKinds) {
97-
ArgumentOrigin argumentOrigin = splitArgumentOrigin(diffResult.getEntry(builderArguments, currentBuilderArguments));
98-
Map<Kind, LayerVerification> perOptionVerifications = verifications.get(argumentOrigin.nameValue().name);
104+
ArgumentOrigin argumentOrigin = splitArgumentOrigin(diffResult.getEntry(left, right));
105+
NameValue argumentNameAndValue = argumentOrigin.nameValue();
106+
var perOptionVerifications = allRequests.get(argumentNameAndValue.name);
99107
if (perOptionVerifications == null) {
100108
continue;
101109
}
102-
LayerVerification verification = perOptionVerifications.get(verificationKind);
103-
if (verification != null) {
104-
OptionOrigin origin = OptionOrigin.from(argumentOrigin.origin);
105-
String message = "Parent layer '" + layerProperties.layerName() + "' specified via layer-file '" + layerFile.getFileName() + "'" +
106-
" was built with option argument '" + argumentOrigin.argument + "' from " + origin + ".";
107-
String suffix;
108-
if (!verification.Message().isEmpty()) {
109-
suffix = verification.Message();
110-
} else {
111-
suffix = switch (verificationKind) {
112-
case Removed, Changed -> "This is also required to be specified for the current layered image build.";
113-
case Added -> "This is also required to be specified for the parent layer build.";
114-
};
115-
}
116-
message += " " + suffix;
117-
switch (verification.Severity()) {
118-
case Info -> LogUtils.info(message);
119-
case Warn -> LogUtils.warning(message);
120-
case Error -> {
121-
if (!Boolean.getBoolean("org.graalvm.nativeimage.layers.verification.strict")) {
122-
LogUtils.warning(message);
123-
} else {
124-
UserError.abort(message);
125-
}
110+
LayerVerification request = perOptionVerifications.requests().get(verificationKind);
111+
if (request == null || request.Positional() != positional) {
112+
continue;
113+
}
114+
115+
OptionOrigin origin = OptionOrigin.from(argumentOrigin.origin);
116+
String argument = SubstrateOptionsParser.commandArgument(perOptionVerifications.option().getOptionKey(), argumentNameAndValue.value);
117+
String message = switch (diffResult.kind()) {
118+
case Removed -> "Parent layer was";
119+
case Added -> "Current layer gets";
120+
case Equal -> throw VMError.shouldNotReachHere("diff for equal");
121+
} + " built with option argument '" + argument + "' from " + origin + ".";
122+
String suffix;
123+
if (!request.Message().isEmpty()) {
124+
suffix = request.Message();
125+
} else {
126+
/* fallback to generic verification message */
127+
suffix = "This is also required to be specified for the " + switch (diffResult.kind()) {
128+
case Removed -> "current layered image build";
129+
case Added -> "parent layer build";
130+
case Equal -> throw VMError.shouldNotReachHere("diff for equal");
131+
};
132+
}
133+
message += " " + suffix + (positional ? " at the same position." : ".");
134+
switch (request.Severity()) {
135+
case Info -> LogUtils.info(message);
136+
case Warn -> LogUtils.warning(message);
137+
case Error -> {
138+
if (strict) {
139+
UserError.abort(message);
140+
} else {
141+
LogUtils.warning(message);
126142
}
127143
}
128144
}

0 commit comments

Comments
 (0)