Bug 237031 – [JSC] Set ssh keepalive in run-jsc-stress-tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Angelos Oikonomopoulos]

[JSC] Set ssh keepalive in run-jsc-stress-tests

[Angelos Oikonomopoulos]

Created attachment 452852 [details]
Patch

[Jonathan Bedard]

Stopped https://ews-build.webkit.org/#/builders/70/builds/1022, the 3 failures it flagged are already known.

[Adrian Perez]

Comment on attachment 452852 [details]
Patch

The new option added and moving the always used SSH options into a common
array LGTM, but there is one cleanup I would like to see done as part of
this patch =)

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

> Tools/Scripts/run-jsc-stress-tests:2560
> +    IO.popen("ssh #{SSH_OPTIONS_DEFAULT.join(" ")} -p #{remoteHost.port}" + (remoteHost.identity_file_path ? " -i #{remoteHost.identity_file_path}" : "") + " #{remoteHost.user}@#{remoteHost.host} '#{cmd}'", "r") {

While doing modifications here, could we change this to pass the array of
command+arguments to IO.popen instead of using a command string that will
be passed through the shell? The “ssh” invocation itself does not need to
go through the local shell — the remote command (“cmd” here) will still
run in a shell remotely because that's how SSH behaves.

[Angelos Oikonomopoulos]

Created attachment 452961 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Adrian Perez from comment #3)

> While doing modifications here, could we change this to pass the array of
> command+arguments to IO.popen instead of using a command string that will
> be passed through the shell? The “ssh” invocation itself does not need to
> go through the local shell — the remote command (“cmd” here) will still
> run in a shell remotely because that's how SSH behaves.

Sure thing! While here I also changed the next snippet to not open-code a read loop.

[Adrian Perez]

Comment on attachment 452961 [details]
Patch

r=me, thanks!

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

> Tools/Scripts/run-jsc-stress-tests:2566
> +        result = inp.read

Not sure if placebo effect, but this does look more readable to me, as a bonus :-}

[EWS]

Committed r290369 (247685@main): <https://commits.webkit.org/247685@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89351769>