Address comments

rasharab · rasharab · commit eb959c77e678 · 2025-01-16T13:00:39.000-08:00
1. Add abiility for tests to override environment behaviour
2. Fix tests.
diff --git a/src/main/kotlin/com/github/gradle/node/NodeExtension.kt b/src/main/kotlin/com/github/gradle/node/NodeExtension.kt
@@ -5,6 +5,7 @@ import com.github.gradle.node.util.Platform
 import org.gradle.api.Project
 import org.gradle.kotlin.dsl.create
 import org.gradle.kotlin.dsl.getByType
+import org.gradle.kotlin.dsl.mapProperty
 import org.gradle.kotlin.dsl.property
 
 open class NodeExtension(project: Project) {
@@ -181,6 +182,9 @@ open class NodeExtension(project: Project) {
      */
     val resolvedPlatform = project.objects.property<Platform>()
 
+
+    val environment = project.objects.mapProperty<String, String>().convention(System.getenv())
+
     init {
         distBaseUrl.set("https://nodejs.org/dist")
     }
diff --git a/src/main/kotlin/com/github/gradle/node/exec/ExecRunner.kt b/src/main/kotlin/com/github/gradle/node/exec/ExecRunner.kt
@@ -13,9 +13,10 @@ import java.io.File
  *
  * @param execConfiguration configuration to get environment variables from
  */
-fun computeEnvironment(execConfiguration: ExecConfiguration): Map<String, String> {
+fun computeEnvironment(extension: NodeExtension, execConfiguration: ExecConfiguration): Map<String, String> {
     val execEnvironment = mutableMapOf<String, String>()
     execEnvironment += System.getenv()
+    execEnvironment += extension.environment.getOrElse(emptyMap())
     execEnvironment += execConfiguration.environment
     if (execConfiguration.additionalBinPaths.isNotEmpty()) {
         // Take care of Windows environments that may contain "Path" OR "PATH" - both existing
@@ -34,25 +35,6 @@ fun computeWorkingDir(nodeProjectDir: DirectoryProperty, execConfiguration: Exec
     workingDir.mkdirs()
     return workingDir
 }
-
-/**
- * Helper function to find the best matching executable in the system PATH.
- *
- * @param executableName The name of the executable to search for.
- * @return The best matching executable path as a String.
- */
-fun findBestExecutableMatch(executableName: String): String {
-    val pathVariable = System.getenv("PATH") ?: return executableName
-    val paths = pathVariable.split(File.pathSeparator)
-    for (path in paths) {
-        val executableFile = File(path, executableName)
-        if (executableFile.exists() && executableFile.canExecute()) {
-            return executableFile.absolutePath
-        }
-    }
-    return executableName  // Return the original executable if no match is found
-}
-
 /**
  * Basic execution runner that runs a given ExecConfiguration.
  *
@@ -61,11 +43,10 @@ fun findBestExecutableMatch(executableName: String): String {
  */
 class ExecRunner {
     fun execute(projectHelper: ProjectApiHelper, extension: NodeExtension, execConfiguration: ExecConfiguration): ExecResult {
-        val executablePath = findBestExecutableMatch(execConfiguration.executable)
         return projectHelper.exec {
-            executable = executablePath
+            executable = execConfiguration.executable
             args = execConfiguration.args
-            environment = computeEnvironment(execConfiguration)
+            environment = computeEnvironment(extension, execConfiguration)
             isIgnoreExitValue = execConfiguration.ignoreExitValue
             workingDir = computeWorkingDir(extension.nodeProjectDir, execConfiguration)
             execConfiguration.execOverrides?.execute(this)
diff --git a/src/main/kotlin/com/github/gradle/node/variant/VariantComputer.kt b/src/main/kotlin/com/github/gradle/node/variant/VariantComputer.kt
@@ -8,19 +8,47 @@ import org.gradle.api.file.Directory
 import org.gradle.api.file.DirectoryProperty
 import org.gradle.api.provider.Property
 import org.gradle.api.provider.Provider
+import java.io.File
+
+/**
+ * Helper function to find the best matching executable in the system PATH.
+ *
+ * @param executableName The name of the executable to search for.
+ * @return The best matching executable path as a String.
+ */
+fun findBestExecutableMatch(
+    executableName: String,
+    environmentProvider: Provider<Map<String, String>>,
+): String {
+    val environment = environmentProvider.orNull ?: return executableName
+    val pathEnvironmentVariableName = if (environment["Path"] != null) "Path" else "PATH"
+    val pathVariable = environment[pathEnvironmentVariableName] ?: return executableName
+    val paths = pathVariable.split(File.pathSeparator)
+    for (path in paths) {
+        val executableFile = File(path, executableName)
+        if (executableFile.exists() && executableFile.canExecute()) {
+            return executableFile.absolutePath
+        }
+    }
+    return executableName  // Return the original executable if no match is found
+}
 
 /**
  * Get the expected node binary name, node.exe on Windows and node everywhere else.
  */
-fun computeNodeExec(nodeExtension: NodeExtension, nodeBinDirProvider: Provider<Directory>): Provider<String> {
+fun computeNodeExec(
+    nodeExtension: NodeExtension,
+    nodeBinDirProvider: Provider<Directory>,
+): Provider<String> {
     return zip(nodeExtension.download, nodeBinDirProvider).map {
         val (download, nodeBinDir) = it
         if (download) {
             val nodeCommand = if (nodeExtension.resolvedPlatform.get().isWindows()) "node.exe" else "node"
             nodeBinDir.dir(nodeCommand).asFile.absolutePath
-        } else "node"
+        } else findBestExecutableMatch("node", nodeExtension.environment)
     }
 }
+
 fun computeNpmScriptFile(nodeDirProvider: Provider<Directory>, command: String, isWindows: Boolean): Provider<String> {
     return nodeDirProvider.map { nodeDir ->
         if (isWindows) nodeDir.dir("node_modules/npm/bin/$command-cli.js").asFile.path
@@ -51,7 +79,7 @@ internal fun computeExec(nodeExtension: NodeExtension, binDirProvider: Provider<
         val command = if (nodeExtension.resolvedPlatform.get().isWindows()) {
             cfgCommand.mapIf({ it == unixCommand }) { windowsCommand }
         } else cfgCommand
-        if (download) binDir.dir(command).asFile.absolutePath else command
+        if (download) binDir.dir(command).asFile.absolutePath else findBestExecutableMatch(command, nodeExtension.environment)
     }
 }
 
diff --git a/src/test/groovy/com/github/gradle/node/npm/task/NpmInstallTaskTest.groovy b/src/test/groovy/com/github/gradle/node/npm/task/NpmInstallTaskTest.groovy
@@ -13,6 +13,7 @@ class NpmInstallTaskTest extends AbstractTaskTest {
     def "exec npm install task with configured proxy"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
         GradleProxyHelper.setHttpsProxyHost("my-super-proxy.net")
         GradleProxyHelper.setHttpsProxyPort(11235)
 
@@ -32,6 +33,7 @@ class NpmInstallTaskTest extends AbstractTaskTest {
     def "exec npm install task with configured proxy but disabled"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
         GradleProxyHelper.setHttpsProxyHost("my-super-proxy.net")
         GradleProxyHelper.setHttpsProxyPort(11235)
         nodeExtension.nodeProxySettings.set(ProxySettings.OFF)
diff --git a/src/test/groovy/com/github/gradle/node/npm/task/NpmTaskTest.groovy b/src/test/groovy/com/github/gradle/node/npm/task/NpmTaskTest.groovy
@@ -6,16 +6,92 @@ import com.github.gradle.node.npm.proxy.ProxySettings
 import com.github.gradle.node.task.AbstractTaskTest
 import com.github.gradle.node.util.PlatformHelperKt
 
+import java.nio.file.Files
+
 import static com.github.gradle.node.NodeExtension.DEFAULT_NODE_VERSION
 
 class NpmTaskTest extends AbstractTaskTest {
     def cleanup() {
         GradleProxyHelper.resetProxy()
     }
 
+    /**
+     * Creates a temporary directory with a dummy executable file.
+     *
+     * @param fileName The name of the dummy executable (e.g., "npm").
+     * @param scriptContent (Optional) The content inside the executable. Defaults to a simple echo script.
+     * @return File object pointing to the dummy executable.
+     */
+    File createTempDummyExecutable(String fileName, String scriptContent = "#!/bin/bash\necho 'Dummy executable ran with args: \$@'") {
+        // Create a temporary directory
+        File tempDir = Files.createTempDirectory("dummy-bin").toFile()
+
+        // Create the dummy executable inside the temp directory
+        File executableFile = new File(tempDir, fileName)
+
+        // Write the script content
+        executableFile.text = scriptContent
+
+        // Set executable permissions
+        executableFile.setExecutable(true)
+
+        return executableFile
+    }
+
+    def "exec npm task with environment variable"() {
+        given:
+        def npmFile = createTempDummyExecutable("npm")
+
+        nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set(["PATH": npmFile.parent])
+
+        def task = project.tasks.create('simple', NpmTask)
+        mockProjectApiHelperExec(task)
+        task.args.set(['a', 'b'])
+        task.environment.set(['a': '1'])
+        task.ignoreExitValue.set(true)
+        task.workingDir.set(projectDir)
+
+        when:
+        project.evaluate()
+        task.exec()
+
+        then:
+        1 * execSpec.setIgnoreExitValue(true)
+        1 * execSpec.setEnvironment({ it['a'] == '1' && containsPath(it) })
+        1 * execSpec.setExecutable(npmFile.absolutePath)
+        1 * execSpec.setArgs(['a', 'b'])
+    }
+
+    def "exec npm task with environment variable (windows)"() {
+        given:
+        def npmFile = createTempDummyExecutable("npm.cmd")
+
+        nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Windows", "x86_64", {}))
+        nodeExtension.environment.set(["Path": npmFile.parent])
+
+        def task = project.tasks.create('simple', NpmTask)
+        mockProjectApiHelperExec(task)
+        task.args.set(['a', 'b'])
+        task.environment.set(['a': '1'])
+        task.ignoreExitValue.set(true)
+        task.workingDir.set(projectDir)
+
+        when:
+        project.evaluate()
+        task.exec()
+
+        then:
+        1 * execSpec.setIgnoreExitValue(true)
+        1 * execSpec.setEnvironment({ it['a'] == '1' && containsPath(it) })
+        1 * execSpec.setExecutable(npmFile.absolutePath)
+        1 * execSpec.setArgs(['a', 'b'])
+    }
+
     def "exec npm task"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
 
         def task = project.tasks.create('simple', NpmTask)
         mockProjectApiHelperExec(task)
@@ -57,6 +133,39 @@ class NpmTaskTest extends AbstractTaskTest {
         1 * execSpec.setArgs(['a', 'b'])
     }
 
+    def "exec npm task with path respects download"() {
+        given:
+        def npmFile = createTempDummyExecutable("npm")
+
+        nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set(["PATH": npmFile.parent])
+        nodeExtension.download.set(true)
+
+        def nodeDir = projectDir.toPath().resolve(".gradle").resolve("nodejs")
+                .resolve("node-v${DEFAULT_NODE_VERSION}-linux-x64")
+
+        def task = project.tasks.create('simple', NpmTask)
+        mockProjectApiHelperExec(task)
+        task.args.set(["run", "command"])
+
+        when:
+        project.evaluate()
+        task.exec()
+
+        then:
+        1 * execSpec.setIgnoreExitValue(false)
+        1 * execSpec.setExecutable({ executable ->
+            def expectedNodePath = nodeDir.resolve("bin").resolve("node")
+            return fixAbsolutePath(executable) == expectedNodePath.toAbsolutePath().toString()
+        })
+        1 * execSpec.setArgs({ args ->
+            def command = nodeDir
+                    .resolve("lib").resolve("node_modules").resolve("npm").resolve("bin")
+                    .resolve("npm-cli.js").toAbsolutePath().toString()
+            return fixAbsolutePaths(args) == [command, "run", "command"]
+        })
+    }
+
     def "exec npm task (download)"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
@@ -121,6 +230,8 @@ class NpmTaskTest extends AbstractTaskTest {
     def "exec npm task with configured proxy"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
+
         GradleProxyHelper.setHttpsProxyHost("my-super-proxy.net")
         GradleProxyHelper.setHttpsProxyPort(11235)
 
@@ -141,6 +252,8 @@ class NpmTaskTest extends AbstractTaskTest {
     def "exec npm task with configured proxy but disabled"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
+
         GradleProxyHelper.setHttpsProxyHost("my-super-proxy.net")
         GradleProxyHelper.setHttpsProxyPort(11235)
         nodeExtension.nodeProxySettings.set(ProxySettings.OFF)
diff --git a/src/test/groovy/com/github/gradle/node/npm/task/NpxTaskTest.groovy b/src/test/groovy/com/github/gradle/node/npm/task/NpxTaskTest.groovy
@@ -10,6 +10,7 @@ class NpxTaskTest extends AbstractTaskTest {
     def "exec npx task"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
 
         def task = project.tasks.create('simple', NpxTask)
         mockProjectApiHelperExec(task)
diff --git a/src/test/groovy/com/github/gradle/node/task/NodeTaskTest.groovy b/src/test/groovy/com/github/gradle/node/task/NodeTaskTest.groovy
@@ -18,6 +18,7 @@ class NodeTaskTest extends AbstractTaskTest {
     def "exec node task"() {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Linux", "x86_64", {}))
+        nodeExtension.environment.set([:])
         nodeExtension.download.set(false)
 
         def task = project.tasks.create('simple', NodeTask)
@@ -93,6 +94,7 @@ class NodeTaskTest extends AbstractTaskTest {
         given:
         nodeExtension.resolvedPlatform.set(PlatformHelperKt.parsePlatform("Windows", "x86_64", {}))
         nodeExtension.download.set(false)
+        nodeExtension.environment.set([:])
 
         def task = project.tasks.create('simple', NodeTask)
         mockProjectApiHelperExec(task)
diff --git a/src/test/groovy/com/github/gradle/node/variant/VariantComputerTest.groovy b/src/test/groovy/com/github/gradle/node/variant/VariantComputerTest.groovy
@@ -157,6 +157,7 @@ class VariantComputerTest extends Specification {
         nodeExtension.resolvedPlatform.set(platform)
         nodeExtension.download.set(download)
         nodeExtension.npmVersion.set(npmVersion)
+        nodeExtension.environment.set([:])
 
         def variantComputer = new VariantComputer()
 
@@ -208,6 +209,7 @@ class VariantComputerTest extends Specification {
         nodeExtension.resolvedPlatform.set(platform)
         nodeExtension.download.set(download)
         nodeExtension.npmVersion.set(npmVersion)
+        nodeExtension.environment.set([:])
 
         def variantComputer = new VariantComputer()
 
