[JSC] Fix run-jsc-stress-tests missing escape $
Created attachment 430033 [details] Patch
Comment on attachment 430033 [details] Patch rs=me
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].
<rdar://problem/78628571>
Reopening to attach new patch.
Created attachment 430044 [details] Patch
Comment on attachment 430044 [details] Patch rs=me
Created attachment 430055 [details] Patch
Test patch with MIPS JSC EWS
Created attachment 430057 [details] Patch
Comment on attachment 430057 [details] Patch I'm just going to keep saying rs=me until it lands :)
Created attachment 430072 [details] Patch
uh, back to the complex solution. Test with 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].
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?
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 " ]
(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.
(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.