From a7cb71b776da92ee5ed2211ade55c902dba882ba Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Thu, 14 Nov 2024 09:14:03 +1100 Subject: [PATCH 1/3] Revert "Remove ProcessLauncher dependency from Gradle core" This reverts commit 71a50a61b04b98498a3e3c877ebceb804c121c91. --- .../services/NativeServices.java | 14 ++++++++++++++ .../services/NativeServicesTest.groovy | 7 ++++++- subprojects/core/build.gradle.kts | 2 +- .../process/internal/DefaultExecHandle.java | 11 +++++++---- .../process/internal/ExecHandleRunner.java | 18 +++++------------- 5 files changed, 33 insertions(+), 19 deletions(-) diff --git a/platforms/core-runtime/native/src/main/java/org/gradle/internal/nativeintegration/services/NativeServices.java b/platforms/core-runtime/native/src/main/java/org/gradle/internal/nativeintegration/services/NativeServices.java index 4dab4e98bc312..22b990f37c427 100644 --- a/platforms/core-runtime/native/src/main/java/org/gradle/internal/nativeintegration/services/NativeServices.java +++ b/platforms/core-runtime/native/src/main/java/org/gradle/internal/nativeintegration/services/NativeServices.java @@ -19,12 +19,14 @@ import net.rubygrapefruit.platform.NativeException; import net.rubygrapefruit.platform.NativeIntegrationUnavailableException; import net.rubygrapefruit.platform.Process; +import net.rubygrapefruit.platform.ProcessLauncher; import net.rubygrapefruit.platform.SystemInfo; import net.rubygrapefruit.platform.WindowsRegistry; import net.rubygrapefruit.platform.file.FileEvents; import net.rubygrapefruit.platform.file.FileSystems; import net.rubygrapefruit.platform.file.Files; import net.rubygrapefruit.platform.file.PosixFiles; +import net.rubygrapefruit.platform.internal.DefaultProcessLauncher; import net.rubygrapefruit.platform.memory.Memory; import net.rubygrapefruit.platform.terminal.Terminals; import org.gradle.api.JavaVersion; @@ -440,6 +442,18 @@ protected Memory createMemory(OperatingSystem operatingSystem) { return notAvailable(Memory.class, operatingSystem); } + @Provides + protected ProcessLauncher createProcessLauncher() { + if (useNativeIntegrations) { + try { + return net.rubygrapefruit.platform.Native.get(ProcessLauncher.class); + } catch (NativeIntegrationUnavailableException e) { + LOGGER.debug("Native-platform process launcher is not available. Continuing with fallback."); + } + } + return new DefaultProcessLauncher(); + } + @Provides protected PosixFiles createPosixFiles(OperatingSystem operatingSystem) { if (useNativeIntegrations) { diff --git a/platforms/core-runtime/native/src/test/groovy/org/gradle/internal/nativeintegration/services/NativeServicesTest.groovy b/platforms/core-runtime/native/src/test/groovy/org/gradle/internal/nativeintegration/services/NativeServicesTest.groovy index fac5bd299655d..4857e17882f3c 100644 --- a/platforms/core-runtime/native/src/test/groovy/org/gradle/internal/nativeintegration/services/NativeServicesTest.groovy +++ b/platforms/core-runtime/native/src/test/groovy/org/gradle/internal/nativeintegration/services/NativeServicesTest.groovy @@ -15,7 +15,7 @@ */ package org.gradle.internal.nativeintegration.services - +import net.rubygrapefruit.platform.ProcessLauncher import net.rubygrapefruit.platform.SystemInfo import net.rubygrapefruit.platform.WindowsRegistry import org.gradle.api.internal.file.temp.TemporaryFileProvider @@ -78,6 +78,11 @@ class NativeServicesTest extends Specification { services.get(SystemInfo) != null } + def "makes a ProcessLauncher available"() { + expect: + services.get(ProcessLauncher) != null + } + def "makes a TemporaryFileProvider available"() { expect: services.get(TemporaryFileProvider) != null diff --git a/subprojects/core/build.gradle.kts b/subprojects/core/build.gradle.kts index 19e0881552a7c..f45d22753b407 100644 --- a/subprojects/core/build.gradle.kts +++ b/subprojects/core/build.gradle.kts @@ -125,6 +125,7 @@ dependencies { api(libs.guava) api(libs.inject) api(libs.jsr305) + api(libs.nativePlatform) implementation(projects.buildOperationsTrace) implementation(projects.io) @@ -132,7 +133,6 @@ dependencies { implementation(projects.modelGroovy) implementation(projects.serviceRegistryBuilder) - implementation(libs.nativePlatform) implementation(libs.asmCommons) implementation(libs.commonsCompress) implementation(libs.commonsIo) diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecHandle.java b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecHandle.java index 367ef6cf0a405..6794e6d0407f7 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecHandle.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecHandle.java @@ -17,11 +17,13 @@ package org.gradle.process.internal; import com.google.common.base.Joiner; +import net.rubygrapefruit.platform.ProcessLauncher; import org.gradle.api.logging.Logger; import org.gradle.api.logging.Logging; import org.gradle.initialization.BuildCancellationToken; import org.gradle.internal.UncheckedException; import org.gradle.internal.event.ListenerBroadcast; +import org.gradle.internal.nativeintegration.services.NativeServices; import org.gradle.internal.operations.CurrentBuildOperationRef; import org.gradle.process.ExecResult; import org.gradle.process.internal.shutdown.ShutdownHooks; @@ -88,6 +90,7 @@ public class DefaultExecHandle implements ExecHandle, ProcessSettings { private final StreamsHandler outputHandler; private final StreamsHandler inputHandler; private final boolean redirectErrorStream; + private final ProcessLauncher processLauncher; private int timeoutMillis; private boolean daemon; @@ -136,9 +139,9 @@ public class DefaultExecHandle implements ExecHandle, ProcessSettings { this.stateChanged = lock.newCondition(); this.state = ExecHandleState.INIT; this.buildCancellationToken = buildCancellationToken; - this.shutdownHookAction = new ExecHandleShutdownHookAction(this); - this.broadcast = new ListenerBroadcast(ExecHandleListener.class); - + processLauncher = NativeServices.getInstance().get(ProcessLauncher.class); + shutdownHookAction = new ExecHandleShutdownHookAction(this); + broadcast = new ListenerBroadcast(ExecHandleListener.class); broadcast.addAll(listeners); } @@ -262,7 +265,7 @@ public ExecHandle start() { broadcast.getSource().beforeExecutionStarted(this); execHandleRunner = new ExecHandleRunner( - this, new CompositeStreamsHandler(), executor, CurrentBuildOperationRef.instance().get() + this, new CompositeStreamsHandler(), processLauncher, executor, CurrentBuildOperationRef.instance().get() ); executor.execute(execHandleRunner); diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/ExecHandleRunner.java b/subprojects/core/src/main/java/org/gradle/process/internal/ExecHandleRunner.java index 9a145cc89b42a..a024000a35e12 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/ExecHandleRunner.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/ExecHandleRunner.java @@ -16,7 +16,7 @@ package org.gradle.process.internal; -import net.rubygrapefruit.platform.NativeException; +import net.rubygrapefruit.platform.ProcessLauncher; import org.gradle.api.logging.Logger; import org.gradle.api.logging.Logging; import org.gradle.internal.operations.BuildOperationRef; @@ -32,6 +32,7 @@ public class ExecHandleRunner implements Runnable { private final ProcessBuilderFactory processBuilderFactory; private final DefaultExecHandle execHandle; private final Lock lock = new ReentrantLock(); + private final ProcessLauncher processLauncher; private final Executor executor; private Process process; @@ -40,9 +41,7 @@ public class ExecHandleRunner implements Runnable { private volatile BuildOperationRef associatedBuildOperation; public ExecHandleRunner( - DefaultExecHandle execHandle, - StreamsHandler streamsHandler, - Executor executor, + DefaultExecHandle execHandle, StreamsHandler streamsHandler, ProcessLauncher processLauncher, Executor executor, BuildOperationRef associatedBuildOperation ) { if (execHandle == null) { @@ -50,6 +49,7 @@ public ExecHandleRunner( } this.execHandle = execHandle; this.streamsHandler = streamsHandler; + this.processLauncher = processLauncher; this.executor = executor; this.associatedBuildOperation = associatedBuildOperation; this.processBuilderFactory = new ProcessBuilderFactory(); @@ -119,7 +119,7 @@ private void startProcess() { throw new IllegalStateException("Process has already been aborted"); } ProcessBuilder processBuilder = processBuilderFactory.createProcessBuilder(execHandle); - Process process = start(processBuilder); + Process process = processLauncher.start(processBuilder); streamsHandler.connectStreams(process, execHandle.getDisplayName(), executor); this.process = process; } finally { @@ -127,14 +127,6 @@ private void startProcess() { } } - private Process start(ProcessBuilder processBuilder) { - try { - return processBuilder.start(); - } catch (Exception e) { - throw new NativeException(String.format("Could not start '%s'", processBuilder.command().get(0)), e); - } - } - private void completed(int exitValue) { if (aborted) { execHandle.aborted(exitValue); From 24fcf5673812556661b2edf14a850fb2240f99b3 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Thu, 14 Nov 2024 14:13:41 +1100 Subject: [PATCH 2/3] Add reproducer for #31282 --- .../integtests/ExecIntegrationTest.groovy | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/platforms/jvm/language-java/src/integTest/groovy/org/gradle/integtests/ExecIntegrationTest.groovy b/platforms/jvm/language-java/src/integTest/groovy/org/gradle/integtests/ExecIntegrationTest.groovy index 414f77c76d57a..b407302d86dbc 100644 --- a/platforms/jvm/language-java/src/integTest/groovy/org/gradle/integtests/ExecIntegrationTest.groovy +++ b/platforms/jvm/language-java/src/integTest/groovy/org/gradle/integtests/ExecIntegrationTest.groovy @@ -25,6 +25,8 @@ import org.gradle.integtests.fixtures.UnsupportedWithConfigurationCache import org.gradle.internal.os.OperatingSystem import org.gradle.process.TestJavaMain import org.gradle.test.fixtures.dsl.GradleDsl +import org.gradle.test.precondition.Requires +import org.gradle.test.preconditions.UnitTestPreconditions import org.gradle.util.internal.TextUtil import org.junit.Rule import spock.lang.Issue @@ -424,6 +426,37 @@ class ExecIntegrationTest extends AbstractIntegrationSpec { "javaexec" | javaExecSpec() | "Using method javaexec(Closure)" | "ExecOperations.javaexec(Action) or ProviderFactory.javaexec(Action)" } + @Issue("https://github.com/gradle/gradle/issues/31282") + @Requires(UnitTestPreconditions.NotWindows) + def "running multiple tasks that fork processes is multi-thread safe"() { + def numOfProjects = 1000 + numOfProjects.times { + settingsFile << """ + include 'project$it' + """ + file("project${it}/build.gradle") << """ + abstract class MyExec extends DefaultTask { + @Inject + abstract ExecOperations getExecOperations() + + @TaskAction + void doIt() { + def script = new File(temporaryDir, "script.sh") + script.text = "#!/bin/bash" + script.executable = true + execOperations.exec { + commandLine script.absolutePath + } + script.delete() + } + } + tasks.register("run", MyExec) + """ + } + expect: + succeeds("run", "--max-workers=100", "--parallel") + } + @UnsupportedWithConfigurationCache(because = "Runs external process at configuration time") def "#method in #location is deprecated in Groovy at configuration time"() { def initScript = groovyFile("init.gradle", "") From 994b294a8b1d1f668241d0eb3792396a85a521e6 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Tue, 19 Nov 2024 08:28:45 -0500 Subject: [PATCH 3/3] accept performance regression This is a revert to the previous behavior, which introduces extra locking to workaround a JDK/Linux fork-behavior. Signed-off-by: Christoph Obexer --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index f2b5f13655938..00acaf46732c8 100644 --- a/gradle.properties +++ b/gradle.properties @@ -23,7 +23,7 @@ develocity.internal.testdistribution.writeTraceFile=true gradle.internal.testdistribution.queryResponseTimeout=PT20S develocity.internal.testdistribution.queryResponseTimeout=PT20S # Default performance baseline -defaultPerformanceBaselines=8.11-commit-4a6676a6310 +defaultPerformanceBaselines=8.11.1-commit-a7cb71b776d # Skip dependency analysis for tests systemProp.dependency.analysis.test.analysis=false