Bug 226385 – [JSC] Fix run-jsc-stress-tests missing escape $

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zhifei Fang]

[JSC] Fix run-jsc-stress-tests missing escape $

[Zhifei Fang]

Created attachment 430033 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 430033 [details]
Patch

rs=me

[EWS]

Committed r278218 (238257@main): <https://commits.webkit.org/238257@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430033 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78628571>

[Zhifei Fang]

Reopening to attach new patch.

[Zhifei Fang]

Created attachment 430044 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 430044 [details]
Patch

rs=me

[Zhifei Fang]

Created attachment 430055 [details]
Patch

[Zhifei Fang]

Test patch with MIPS JSC EWS

[Zhifei Fang]

Created attachment 430057 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 430057 [details]
Patch

I'm just going to keep saying rs=me until it lands :)

[Zhifei Fang]

Created attachment 430072 [details]
Patch

[Zhifei Fang]

uh, back to the complex solution. Test with EWS.

[EWS]

Committed r278234 (238271@main): <https://commits.webkit.org/238271@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430072 [details].

[Darin Adler]

Comment on attachment 430072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430072&action=review

> Tools/ChangeLog:3
> +        [JSC] Escape $ only when run with SSH

Why?

[Angelos Oikonomopoulos]

So, AFAIU this happens when using `--remote-config-file` but not `--gnu-parallel-runner`, right? The symptom I see is:

sh: line 0: cd: ../.vm: No such file or directory
sh: line 0: cd: ../.vm/..: No such file or directory

Which does not result into any errors for me (which I guess is why I didn't notice this when testing). Presumably, that's because I've been testing with $hostOS == "linux" so have been taking the path (in prepareBundle) that runs generate-bundle. As generate-bundle creates a shell wrapper that sets LD_LIBRARY_PATH, I guess the value of LD_LIBRARY_PATH doesn't matter on linux.

That said, it would be great to know which configuration this failed on!

IIUC, this could be fixed by switching to single quotes around the remoteScript (in runTestRunner). At the very least, this makes the symptom above go away for me. The patch would be:

diff --git a/Tools/Scripts/run-jsc-stress-tests b/Tools/Scripts/run-jsc-stress-tests
index e6141b5e7249..0c24c3d5f398 100755
--- a/Tools/Scripts/run-jsc-stress-tests
+++ b/Tools/Scripts/run-jsc-stress-tests
@@ -2197,14 +2197,9 @@ def copyBundleToRemote(remoteHost)
     mysys(["scp", "-o", "NoHostAuthenticationForLocalhost=yes", "-P", remoteHost.port.to_s, ($outputDir.dirname + $tarFileName).to_s, "#{remoteHost.user}@#{remoteHost.host}:#{remoteHost.remoteDirectory}"])
 end
 
-def exportBaseEnvironmentVariables(escape)
-    if escape
-        dyldFrameworkPath = "\\$(cd #{$testingFrameworkPath.dirname}; pwd)"
-        ldLibraryPath = "\\$(cd #{$testingFrameworkPath.dirname}/..; pwd)/#{$jscPath.dirname}"
-    else
-        dyldFrameworkPath = "\$(cd #{$testingFrameworkPath.dirname}; pwd)"
-        ldLibraryPath = "\$(cd #{$testingFrameworkPath.dirname}/..; pwd)/#{$jscPath.dirname}"
-    end
+def exportBaseEnvironmentVariables
+    dyldFrameworkPath = "\$(cd #{$testingFrameworkPath.dirname}; pwd)"
+    ldLibraryPath = "\$(cd #{$testingFrameworkPath.dirname}/..; pwd)/#{$jscPath.dirname}"
     [
         "export DYLD_FRAMEWORK_PATH=#{dyldFrameworkPath} && ",
         "export LD_LIBRARY_PATH=#{ldLibraryPath} &&",
@@ -2220,14 +2215,14 @@ def runTestRunner(remoteIndex=0)
         remoteHost = $remoteHosts[remoteIndex]
         getRemoteDirectoryIfNeeded(remoteHost)
         copyBundleToRemote(remoteHost)
-        remoteScript = "\""
+        remoteScript = "'"
         remoteScript += "cd #{remoteHost.remoteDirectory} && "
         remoteScript += "rm -rf #{$outputDir.basename} && "
         remoteScript += "tar xzf #{$tarFileName} && "
         remoteScript += "cd #{$outputDir.basename}/.runner && "
-        remoteScript += exportBaseEnvironmentVariables(true)
+        remoteScript += exportBaseEnvironmentVariables
         $envVars.each { |var| remoteScript += "export " << var << "\n" }
-        remoteScript += "#{testRunnerCommand(remoteIndex)}\""
+        remoteScript += "#{testRunnerCommand(remoteIndex)}'"
         runAndMonitorTestRunnerCommand(["ssh", "-o", "NoHostAuthenticationForLocalhost=yes", "-p", remoteHost.port.to_s, "#{remoteHost.user}@#{remoteHost.host}", remoteScript])
     else
         Dir.chdir($runnerDir) {
@@ -2637,7 +2632,7 @@ def runGnuParallelRunner(remoteHosts, inputs, options={})
             "--timeout", timeout.to_s,
             "-a", inputs,
             "if test -e #{$outputDir.basename}/.runner; then cd #{$outputDir.basename}/.runner; else echo #{PARALLEL_REMOTE_STATE_LOST_MARKER}; false; fi && " +
-            exportBaseEnvironmentVariables(false) +
+            exportBaseEnvironmentVariables +
             $envVars.collect { |var | "export #{var} &&"}.join("") +
             "sh "
         ]

[Zhifei Fang]

(In reply to Darin Adler from comment #15)
> Comment on attachment 430072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430072&action=review
> 
> > Tools/ChangeLog:3
> > +        [JSC] Escape $ only when run with SSH
> 
> Why?

We have to run path, with --remote-config-file, we will run the script with SSH, without escape &, that will be considered as a local script instead of part command string running remotely.

We also have --gnu-parallel-runner, with this we should not escape $, since that will be part of the script that needed to be run locally.

[Zhifei Fang]

(In reply to Angelos Oikonomopoulos from comment #16)
> So, AFAIU this happens when using `--remote-config-file` but not
> `--gnu-parallel-runner`, right? The symptom I see is:
> 
> sh: line 0: cd: ../.vm: No such file or directory
> sh: line 0: cd: ../.vm/..: No such file or directory
> 
> Which does not result into any errors for me (which I guess is why I didn't
> notice this when testing). Presumably, that's because I've been testing with
> $hostOS == "linux" so have been taking the path (in prepareBundle) that runs
> generate-bundle. As generate-bundle creates a shell wrapper that sets
> LD_LIBRARY_PATH, I guess the value of LD_LIBRARY_PATH doesn't matter on
> linux.
> 
> That said, it would be great to know which configuration this failed on!
> 
> IIUC, this could be fixed by switching to single quotes around the
> remoteScript (in runTestRunner). At the very least, this makes the symptom
> above go away for me. The patch would be:
> 
> diff --git a/Tools/Scripts/run-jsc-stress-tests
> b/Tools/Scripts/run-jsc-stress-tests
> index e6141b5e7249..0c24c3d5f398 100755
> --- a/Tools/Scripts/run-jsc-stress-tests
> +++ b/Tools/Scripts/run-jsc-stress-tests
> @@ -2197,14 +2197,9 @@ def copyBundleToRemote(remoteHost)
>      mysys(["scp", "-o", "NoHostAuthenticationForLocalhost=yes", "-P",
> remoteHost.port.to_s, ($outputDir.dirname + $tarFileName).to_s,
> "#{remoteHost.user}@#{remoteHost.host}:#{remoteHost.remoteDirectory}"])
>  end
>  
> -def exportBaseEnvironmentVariables(escape)
> -    if escape
> -        dyldFrameworkPath = "\\$(cd #{$testingFrameworkPath.dirname}; pwd)"
> -        ldLibraryPath = "\\$(cd #{$testingFrameworkPath.dirname}/..;
> pwd)/#{$jscPath.dirname}"
> -    else
> -        dyldFrameworkPath = "\$(cd #{$testingFrameworkPath.dirname}; pwd)"
> -        ldLibraryPath = "\$(cd #{$testingFrameworkPath.dirname}/..;
> pwd)/#{$jscPath.dirname}"
> -    end
> +def exportBaseEnvironmentVariables
> +    dyldFrameworkPath = "\$(cd #{$testingFrameworkPath.dirname}; pwd)"
> +    ldLibraryPath = "\$(cd #{$testingFrameworkPath.dirname}/..;
> pwd)/#{$jscPath.dirname}"
>      [
>          "export DYLD_FRAMEWORK_PATH=#{dyldFrameworkPath} && ",
>          "export LD_LIBRARY_PATH=#{ldLibraryPath} &&",
> @@ -2220,14 +2215,14 @@ def runTestRunner(remoteIndex=0)
>          remoteHost = $remoteHosts[remoteIndex]
>          getRemoteDirectoryIfNeeded(remoteHost)
>          copyBundleToRemote(remoteHost)
> -        remoteScript = "\""
> +        remoteScript = "'"
>          remoteScript += "cd #{remoteHost.remoteDirectory} && "
>          remoteScript += "rm -rf #{$outputDir.basename} && "
>          remoteScript += "tar xzf #{$tarFileName} && "
>          remoteScript += "cd #{$outputDir.basename}/.runner && "
> -        remoteScript += exportBaseEnvironmentVariables(true)
> +        remoteScript += exportBaseEnvironmentVariables
>          $envVars.each { |var| remoteScript += "export " << var << "\n" }
> -        remoteScript += "#{testRunnerCommand(remoteIndex)}\""
> +        remoteScript += "#{testRunnerCommand(remoteIndex)}'"
>          runAndMonitorTestRunnerCommand(["ssh", "-o",
> "NoHostAuthenticationForLocalhost=yes", "-p", remoteHost.port.to_s,
> "#{remoteHost.user}@#{remoteHost.host}", remoteScript])
>      else
>          Dir.chdir($runnerDir) {
> @@ -2637,7 +2632,7 @@ def runGnuParallelRunner(remoteHosts, inputs,
> options={})
>              "--timeout", timeout.to_s,
>              "-a", inputs,
>              "if test -e #{$outputDir.basename}/.runner; then cd
> #{$outputDir.basename}/.runner; else echo
> #{PARALLEL_REMOTE_STATE_LOST_MARKER}; false; fi && " +
> -            exportBaseEnvironmentVariables(false) +
> +            exportBaseEnvironmentVariables +
>              $envVars.collect { |var | "export #{var} &&"}.join("") +
>              "sh "
>          ]

Yeah, the issue here is export is using pwd, with the cd part failed, it won't export to the right path when run with remote device.