Bug 231101 – [WinCairo] Support run-jsc-stress-tests without posix commands

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

[Stephan Szabo]

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)

[Stephan Szabo]

Created attachment 439912 [details]
Patch

[Angelos Oikonomopoulos]

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 :-)

[Stephan Szabo]

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).

[Angelos Oikonomopoulos]

Yah, the EWS JSC bots for armv7 should take this path.

[Stephan Szabo]

Created attachment 440092 [details]
Patch

[Adrian Perez]

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.

[Stephan Szabo]

(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.

[Stephan Szabo]

Created attachment 440282 [details]
Patch for EWS testing

Checking a version without find on EWS.

[Angelos Oikonomopoulos]

(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).

[Stephan Szabo]

Created attachment 440399 [details]
Patch version using find on remote and ruby local

[Angelos Oikonomopoulos]

(In reply to Stephan Szabo from comment #10)
> Created attachment 440399 [details]
> Patch version using find on remote and ruby local

Looks good.

[Adrian Perez]

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

:)

[Stephan Szabo]

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.

[Adrian Perez]

Comment on attachment 440399 [details]
Patch version using find on remote and ruby local

Let me know if you need a committer

[Adrian Perez]

(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 :)

[Adrian Perez]

(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.

=)

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/83999466>