-
Notifications
You must be signed in to change notification settings - Fork 80
Fix race condition if tests are executed in parallel #195
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.
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
Changes from all commits
b48b7fe
a8f6f1c
1219cbd
bd9d033
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /* | ||
| * Copyright (c) 2020, 2022 Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * The Universal Permissive License (UPL), Version 1.0 | ||
| * | ||
| * Subject to the condition set forth below, permission is hereby granted to any | ||
| * person obtaining a copy of this software, associated documentation and/or | ||
| * data (collectively the "Software"), free of charge and under any and all | ||
| * copyright rights in the Software, and any and all patent rights owned or | ||
| * freely licensable by each licensor hereunder covering either (i) the | ||
| * unmodified Software as contributed to or provided by such licensor, or (ii) | ||
| * the Larger Works (as defined below), to deal in both | ||
| * | ||
| * (a) the Software, and | ||
| * | ||
| * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if | ||
| * one is included with the Software each a "Larger Work" to which the Software | ||
| * is contributed by such licensors), | ||
| * | ||
| * without restriction, including without limitation the rights to copy, create | ||
| * derivative works of, display, perform, and distribute the Software and make, | ||
| * use, sell, offer for sale, import, export, have made, and have sold the | ||
| * Software and the Larger Work(s), and to sublicense the foregoing rights on | ||
| * either these or other terms. | ||
| * | ||
| * This license is subject to the following condition: | ||
| * | ||
| * The above copyright notice and either this complete permission notice or at a | ||
| * minimum a reference to the UPL must be included in all copies or substantial | ||
| * portions of the Software. | ||
| * | ||
| * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| * SOFTWARE. | ||
| */ | ||
| package org.graalvm.buildtools.utils; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
|
|
||
| import static org.graalvm.buildtools.utils.SharedConstants.GRAALVM_EXE_EXTENSION; | ||
|
|
||
| public class NativeImageUtils { | ||
| public static void maybeCreateConfigureUtilSymlink(File configureUtilFile, Path nativeImageExecutablePath) { | ||
| if (!configureUtilFile.exists()) { | ||
| // possibly the symlink is missing | ||
| Path target = configureUtilFile.toPath(); | ||
| Path source = nativeImageExecutablePath.getParent().getParent().resolve("lib/svm/bin/" + nativeImageConfigureFileName()); | ||
| if (Files.exists(source)) { | ||
| try { | ||
| Files.createLink(target, source); | ||
| } catch (IOException e) { | ||
| // ignore as this is handled by consumers | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static String nativeImageConfigureFileName() { | ||
| return "native-image-configure" + GRAALVM_EXE_EXTENSION; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| /* | ||
| * Copyright (c) 2020, 2022 Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * The Universal Permissive License (UPL), Version 1.0 | ||
| * | ||
| * Subject to the condition set forth below, permission is hereby granted to any | ||
| * person obtaining a copy of this software, associated documentation and/or | ||
| * data (collectively the "Software"), free of charge and under any and all | ||
| * copyright rights in the Software, and any and all patent rights owned or | ||
| * freely licensable by each licensor hereunder covering either (i) the | ||
| * unmodified Software as contributed to or provided by such licensor, or (ii) | ||
| * the Larger Works (as defined below), to deal in both | ||
| * | ||
| * (a) the Software, and | ||
| * | ||
| * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if | ||
| * one is included with the Software each a "Larger Work" to which the Software | ||
| * is contributed by such licensors), | ||
| * | ||
| * without restriction, including without limitation the rights to copy, create | ||
| * derivative works of, display, perform, and distribute the Software and make, | ||
| * use, sell, offer for sale, import, export, have made, and have sold the | ||
| * Software and the Larger Work(s), and to sublicense the foregoing rights on | ||
| * either these or other terms. | ||
| * | ||
| * This license is subject to the following condition: | ||
| * | ||
| * The above copyright notice and either this complete permission notice or at a | ||
| * minimum a reference to the UPL must be included in all copies or substantial | ||
| * portions of the Software. | ||
| * | ||
| * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| * SOFTWARE. | ||
| */ | ||
| package org.graalvm.buildtools.gradle; | ||
|
|
||
| import org.graalvm.buildtools.gradle.dsl.NativeImageOptions; | ||
| import org.graalvm.buildtools.gradle.internal.GraalVMLogger; | ||
| import org.graalvm.buildtools.utils.NativeImageUtils; | ||
| import org.gradle.api.Action; | ||
| import org.gradle.api.Task; | ||
| import org.gradle.api.file.Directory; | ||
| import org.gradle.api.file.FileSystemOperations; | ||
| import org.gradle.api.logging.Logger; | ||
| import org.gradle.api.provider.Provider; | ||
| import org.gradle.process.ExecOperations; | ||
| import org.gradle.process.ExecResult; | ||
|
|
||
| import java.io.File; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import static org.graalvm.buildtools.gradle.internal.NativeImageExecutableLocator.findNativeImageExecutable; | ||
| import static org.graalvm.buildtools.utils.NativeImageUtils.nativeImageConfigureFileName; | ||
|
|
||
| class MergeAgentFiles implements Action<Task> { | ||
| private final Provider<Boolean> agent; | ||
| private final Provider<String> graalvmHomeProvider; | ||
| private final Provider<Directory> outputDir; | ||
| private final Provider<Boolean> disableToolchainDetection; | ||
| private final ExecOperations execOperations; | ||
| private final NativeImageOptions options; | ||
| private final FileSystemOperations fileOperations; | ||
| private final Logger logger; | ||
|
|
||
| MergeAgentFiles(Provider<Boolean> agent, | ||
| Provider<String> graalvmHomeProvider, | ||
| Provider<Directory> outputDir, | ||
| Provider<Boolean> disableToolchainDetection, | ||
| NativeImageOptions options, | ||
| ExecOperations execOperations, | ||
| FileSystemOperations fileOperations, | ||
| Logger logger) { | ||
| this.agent = agent; | ||
| this.graalvmHomeProvider = graalvmHomeProvider; | ||
| this.outputDir = outputDir; | ||
| this.disableToolchainDetection = disableToolchainDetection; | ||
| this.options = options; | ||
| this.execOperations = execOperations; | ||
| this.fileOperations = fileOperations; | ||
| this.logger = logger; | ||
| } | ||
|
|
||
| @Override | ||
| public void execute(Task task) { | ||
| if (agent.get()) { | ||
| File nativeImage = findNativeImageExecutable(options, disableToolchainDetection, graalvmHomeProvider, execOperations, GraalVMLogger.of(logger)); | ||
| File workingDir = nativeImage.getParentFile(); | ||
| File launcher = new File(workingDir, nativeImageConfigureFileName()); | ||
| if (!launcher.exists()) { | ||
| logger.info("Installing native-image-configure"); | ||
| execOperations.exec(spec -> { | ||
| spec.executable(nativeImage); | ||
| spec.args("--macro:native-image-configure-launcher"); | ||
| }); | ||
| NativeImageUtils.maybeCreateConfigureUtilSymlink(launcher, nativeImage.toPath()); | ||
| } | ||
| if (launcher.exists()) { | ||
| File[] files = outputDir.get().getAsFile().listFiles(); | ||
| List<String> args = new ArrayList<>(files.length + 2); | ||
| args.add("generate"); | ||
| sessionDirectoriesFrom(files) | ||
| .map(f -> "--input-dir=" + f.getAbsolutePath()) | ||
| .forEach(args::add); | ||
| if (args.size() > 1) { | ||
| logger.info("Merging agent files"); | ||
| args.add("--output-dir=" + outputDir.get().getAsFile().getAbsolutePath()); | ||
| ExecResult exec = execOperations.exec(spec -> { | ||
| spec.executable(launcher); | ||
| spec.args(args); | ||
| spec.setStandardOutput(System.out); | ||
| spec.setErrorOutput(System.err); | ||
| }); | ||
| if (exec.getExitValue() == 0) { | ||
| fileOperations.delete(spec -> sessionDirectoriesFrom(files).forEach(spec::delete)); | ||
| } else { | ||
| exec.rethrowFailure(); | ||
| } | ||
| } | ||
| } else { | ||
| logger.warn("Cannot merge agent files because native-image-configure is not installed. Please upgrade to a newer version of GraalVM."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private Stream<File> sessionDirectoriesFrom(File[] files) { | ||
| return Arrays.stream(files) | ||
| .filter(File::isDirectory) | ||
| .filter(f -> f.getName().startsWith("session-")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ | |
|
|
||
| package org.graalvm.buildtools.gradle.internal; | ||
|
|
||
| import org.graalvm.buildtools.utils.SharedConstants; | ||
| import org.gradle.api.file.DirectoryProperty; | ||
| import org.gradle.api.provider.ListProperty; | ||
| import org.gradle.api.provider.Property; | ||
|
|
@@ -82,7 +83,7 @@ public Iterable<String> asArguments() { | |
| 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"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at #189, in context of this PR, it seems that Not a blocker for merging this PR, but something to think about. We might want to add wrapper Gradle/Maven agent properties for following agentlib parameters:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea with #190 is now to have an enum which tells what we want to generate: trace output or config output. I'm not sure what config merge dir is used for. In any case, this should probably not be mixed into this PR.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From docs:
Basically we would need to reproduce this behavior using I agree that this should not be mixed with this PR, but it is something to keep in mind when designing tasks related to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, in combination with this race condition this starts to become bind blowing. |
||
| } | ||
| agentOptions.add("config-output-dir=" + outputDir.getAbsolutePath()); | ||
| agentOptions.add("config-output-dir=" + outputDir.getAbsolutePath() + File.separator + SharedConstants.AGENT_SESSION_SUBDIR); | ||
| return Arrays.asList( | ||
| "-agentlib:native-image-agent=" + String.join(",", agentOptions), | ||
| "-Dorg.graalvm.nativeimage.imagecode=agent" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should migrate logic that is currently located in
ProcessGeneratedGraalResourceFilesto thisnative-image-configureinvocation.So something like:
(we should also probably make that configurable somehow).
That would be useful as a workaround for oracle/graal#3968 that is one of blockers for Mockito support (cc @raphw):
We should then probably change
MergeAgentFilesto something likeProcessAgentFiles.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced we should merge them. The "merge" logic can be seen as an internal implementation detail of the agent implementation: instead of outputting n files, it should output a single file. Therefore, this logic needs to be invoked as part of the task execution itself, so that, for example, we don't mix files from different invocations.
The "process agent files" task, on the other hand, is, as its name states, a post processing task which, yes, could be made more configurable, but I don't think we should collate both aspects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable. I have no strong opinions about this, so exact steps in my proposal aren't set in stone. My logic was that we might want to do it in a single
native-image-configureinvocation so it sounded like a job for a single task.I however believe that we should (somehow) utilize
native-image-configureto filter out Gradle entries, and add a way for end user to append values intogetFilterableEntries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to imply that
native-image-configurecan be used to filter entries. Is this documented somewhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contains the following argument:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Let's start with what we have now and we can always change the implementation if needed, I'm not too worried.