Skip to content
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

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

Open
hiroyuki-sato opened this issue Apr 18, 2021 · 3 comments · May be fixed by #1654
Open

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

hiroyuki-sato opened this issue Apr 18, 2021 · 3 comments · May be fixed by #1654

Comments

@hiroyuki-sato
Copy link
Contributor

hiroyuki-sato commented Apr 18, 2021

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.

final Path runnerPath = tempDir.resolve("runner.sh"); // absolute

Workaround: Windows user uses 0.9.42 until fix this issue.

Ref: twitter

@hiroyuki-sato
Copy link
Contributor Author

hiroyuki-sato commented Apr 18, 2021

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
Copy link
Contributor Author

@szyn Could you comment about this issue?

@szyn
Copy link
Member

szyn commented Oct 3, 2021

@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?

@hiroyuki-sato hiroyuki-sato linked a pull request Oct 4, 2021 that will close this issue
@kono kono mentioned this issue Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants