A change was made to collect up the status files that is using "find" which is failing on native windows without posix commands. Probably will need to add an alternate path using Dir.glob for --ruby-runner (and maybe for playstation as well? Need to check)
Created attachment 439912 [details] Patch
Oops. Patch looks reasonable. Minor nit: looks like you can avoid repeating Dir.chdir by hoisting it out of the if/else? Even more minor nit: you can implement a class StatusFileEnumerator def each_line Dir.glob("#{STATUS_FILE_PREFIX}*").each do |name| if File.size(name) > 0 line = File.open(name).first yield "./#{name} #{line}" end end end end and then do something like Dir.chdir($runnerDir) { statusFileEnumerator = StatusFileEnumerator.new if $testRunnerType != :ruby and $testWriter != "playstation" statusFileEnumerator = IO.popen(find_cmd) end statuFileEnumerator.each_line { | line | processStatusLine(map, line) } } but that might be overkill :-)
Created attachment 440073 [details] Patch Updated and I went with the class version, tried it on a tiny test subset on windows native and going to see if EWS is happy (hoping that there's some bot that will exercise this).
Yah, the EWS JSC bots for armv7 should take this path.
Created attachment 440092 [details] Patch
Comment on attachment 440092 [details] Patch Hello, and thanks for the patch. I think this is going in the right direction, but I have a couple of suggestions to make it even better. Please read on below :) View in context: https://bugs.webkit.org/attachment.cgi?id=440092&action=review > Tools/Scripts/run-jsc-stress-tests:2334 > + def each_line I suppose this function could be just a function; there is no need to put it inside a class, right? > Tools/Scripts/run-jsc-stress-tests:2367 > + statusFileEnumerator.each_line { I would completely remove the usage of the “find” command and use the same code that uses Ruby functions to locate and read the result files. The advantages would be having only one code path to maintain that works for all ports, and avoiding spawning a bunch of subprocesses here.
(In reply to Adrian Perez from comment #6) > Comment on attachment 440092 [details] > Patch > > Hello, and thanks for the patch. I think this is going in the right > direction, but I have a couple of suggestions to make it even better. > Please read on below :) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440092&action=review > > > Tools/Scripts/run-jsc-stress-tests:2334 > > + def each_line > > I suppose this function could be just a function; there is no > need to put it inside a class, right? Hmm, probably. Ruby isn't high on my list of languages, so I figured that it was suggested as a class to make it look similar to the popen output. If we don't switch between them per the below, then even the function might not be strictly necessary. > > Tools/Scripts/run-jsc-stress-tests:2367 > > + statusFileEnumerator.each_line { > > I would completely remove the usage of the “find” command and use the > same code that uses Ruby functions to locate and read the result files. > The advantages would be having only one code path to maintain that works > for all ports, and avoiding spawning a bunch of subprocesses here. I'd thought about that, but I wasn't sure how to best manage that with the $remote codepath and was concerned that it might be wanted to keep the same code between the local non-windows and remote case, but I can change either way.
Created attachment 440282 [details] Patch for EWS testing Checking a version without find on EWS.
(In reply to Adrian Perez from comment #6) > Comment on attachment 440092 [details] > Patch > > Hello, and thanks for the patch. I think this is going in the right > direction, but I have a couple of suggestions to make it even better. > Please read on below :) Thanks for taking a look Adrian, IMHO you're more right than you think :-) > View in context: > https://bugs.webkit.org/attachment.cgi?id=440092&action=review > > > Tools/Scripts/run-jsc-stress-tests:2334 > > + def each_line > > I suppose this function could be just a function; there is no > need to put it inside a class, right? The reason I suggested a class is so that it would be interchangeable with the IO object produced by IO.popen(find_cmd). Technically you could wrap /that/ in a function so as to have a common interface. I thought it was more idiomatic that way. Opinions may differ. > > Tools/Scripts/run-jsc-stress-tests:2367 > > + statusFileEnumerator.each_line { > > I would completely remove the usage of the “find” command and use the > same code that uses Ruby functions to locate and read the result files. > The advantages would be having only one code path to maintain that works > for all ports, and avoiding spawning a bunch of subprocesses here. The reason I didn't suggest that is I introduced find_cmd locally so as to share the local code with the one dealing with remote boxes. This way, changes to this part of the code would automatically apply to the "remote" code as well. I didn't realize when replying, but this implicitly prioritizes the find-based remote code path over the ruby-based non-posix code path. However, I think the choice here is clear: the find code should be much slower (because of all those sh invocations), so we should go with the ruby version for the local case (though measuring would be nice at some point).
Created attachment 440399 [details] Patch version using find on remote and ruby local
(In reply to Stephan Szabo from comment #10) > Created attachment 440399 [details] > Patch version using find on remote and ruby local Looks good.
(In reply to Angelos Oikonomopoulos from comment #11) > (In reply to Stephan Szabo from comment #10) > > Created attachment 440399 [details] > > Patch version using find on remote and ruby local > > Looks good. LGTM, but you haven't set the r? or cq? flags. Do you still want to change anything in the patch before approving it, or may I set r+ and cq+ on it? :)
Sorry about that, I sometimes wait for EWS to come back before setting r? and kept waiting on the jsc-*-tests results and meant to set it yesterday.
Comment on attachment 440399 [details] Patch version using find on remote and ruby local Let me know if you need a committer
(In reply to Adrian Perez from comment #14) > Comment on attachment 440399 [details] > Patch version using find on remote and ruby local > > Let me know if you need a committer ...to set r+ But then I noticed you are a committer yourself :)
(In reply to Stephan Szabo from comment #13) > Sorry about that, I sometimes wait for EWS to come back before setting r? > and kept waiting on the jsc-*-tests results and meant to set it yesterday. No problem at all! Sometimes I also set r? without setting cq? and wait for the EWS to be green before changing to cq+ even if a reviewer had approved already the patch. =)
Committed r283745 (242667@main): <https://commits.webkit.org/242667@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440399 [details].
<rdar://problem/83999466>