digdag icon indicating copy to clipboard operation
digdag copied to clipboard

sh> operator of the Digdag v0.10.0 on Windows platform

Open hiroyuki-sato opened this issue 5 years ago • 3 comments

Relates to #1540

The sh> operator on the Digdag 0.10.0 doesn't work correctly on a Windows Platform.

@hundyhundy investigated this issue. This article was written in Japanese In summary, It needs to change script name like runner.ps1 instead of runner.sh when Digdag uses PowerShell. https://github.com/treasure-data/digdag/blob/42ea7cb71270957ea6424c2df88dd7c65d248789/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java#L136

Workaround: Windows user uses 0.9.42 until fix this issue.

Ref: twitter

hiroyuki-sato avatar Apr 18 '21 13:04 hiroyuki-sato

This patch fixes this issue. Could you tell me what do you think this change? I believe the current behavior doesn't change anything.

If this patch seems good, what branch should I use for PR? (v0.11? or master)

diff --git a/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java b/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
index e6cf6f3608..8ff5540fde 100644
--- a/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
+++ b/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
@@ -133,14 +133,15 @@ public class ShOperatorFactory
         {
             final Path tempDir = workspace.createTempDir(String.format("digdag-sh-%d-", request.getTaskId()));
             final Path workingDirectory = workspace.getPath(); // absolute
-            final Path runnerPath = tempDir.resolve("runner.sh"); // absolute
+            final Boolean winOS = isWindowsPlatform();
+            final Path runnerPath = tempDir.resolve(winOS ? "runner.ps1" : "runner.sh"); // absolute
 
             final List<String> shell;
             if (params.has("shell")) {
                 shell = params.getListOrEmpty("shell", String.class);
             }
             else {
-                shell = ImmutableList.of("/bin/sh");
+                shell = ImmutableList.of(winOS ? "PowerShell.exe" : "/bin/sh");
             }
 
             final ImmutableList.Builder<String> cmdline = ImmutableList.builder();
@@ -223,5 +224,14 @@ public class ShOperatorFactory
                     .ioDirectory(ioDirectory)
                     .build();
         }
+        private boolean isWindowsPlatform()
+        {
+            final String os = System.getProperty("os.name").toLowerCase();
+            if ( os.startsWith("windows")) {
+                return true;
+            }
+            System.out.println("not windows");
+            return false;
+        }
     }
 }

Another idea. Add script_name option

diff --git a/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java b/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
index e6cf6f3608..57dcaddfca 100644
--- a/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
+++ b/digdag-standards/src/main/java/io/digdag/standards/operator/ShOperatorFactory.java
@@ -133,7 +133,7 @@ public class ShOperatorFactory
         {
             final Path tempDir = workspace.createTempDir(String.format("digdag-sh-%d-", request.getTaskId()));
             final Path workingDirectory = workspace.getPath(); // absolute
-            final Path runnerPath = tempDir.resolve("runner.sh"); // absolute
+            final Path runnerPath = tempDir.resolve(params.get("script_name", String.class, "runner.sh")); // absolute

             final List<String> shell;
             if (params.has("shell")) {

hiroyuki-sato avatar Apr 18 '21 13:04 hiroyuki-sato

@szyn Could you comment about this issue?

hiroyuki-sato avatar Aug 17 '21 11:08 hiroyuki-sato

@hiroyuki-sato Sorry, I overlooked your mention.

Thank you for raising and investigating this! In my opinion, the former patch looks good to me. Could you create a PR for master branch when you have time?

szyn avatar Oct 03 '21 02:10 szyn