Bug 231101 - [WinCairo] Support run-jsc-stress-tests without posix commands
Summary: [WinCairo] Support run-jsc-stress-tests without posix commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-01 13:56 PDT by Stephan Szabo
Modified: 2021-10-07 14:30 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2021-10-01 14:25 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2021-10-04 10:21 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2021-10-04 12:59 PDT, Stephan Szabo
aperez: review-
Details | Formatted Diff | Diff
Patch for EWS testing (2.70 KB, patch)
2021-10-05 15:35 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff
Patch version using find on remote and ruby local (1.51 KB, patch)
2021-10-06 11:48 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Szabo 2021-10-01 13:56:46 PDT
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)
Comment 1 Stephan Szabo 2021-10-01 14:25:05 PDT
Created attachment 439912 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2021-10-04 09:48:29 PDT
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 :-)
Comment 3 Stephan Szabo 2021-10-04 10:21:06 PDT
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).
Comment 4 Angelos Oikonomopoulos 2021-10-04 10:25:39 PDT
Yah, the EWS JSC bots for armv7 should take this path.
Comment 5 Stephan Szabo 2021-10-04 12:59:55 PDT
Created attachment 440092 [details]
Patch
Comment 6 Adrian Perez 2021-10-05 14:36:43 PDT
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.
Comment 7 Stephan Szabo 2021-10-05 14:43:40 PDT
(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.
Comment 8 Stephan Szabo 2021-10-05 15:35:34 PDT
Created attachment 440282 [details]
Patch for EWS testing

Checking a version without find on EWS.
Comment 9 Angelos Oikonomopoulos 2021-10-06 09:12:21 PDT
(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).
Comment 10 Stephan Szabo 2021-10-06 11:48:58 PDT
Created attachment 440399 [details]
Patch version using find on remote and ruby local
Comment 11 Angelos Oikonomopoulos 2021-10-07 03:04:03 PDT
(In reply to Stephan Szabo from comment #10)
> Created attachment 440399 [details]
> Patch version using find on remote and ruby local

Looks good.
Comment 12 Adrian Perez 2021-10-07 04:58:28 PDT
(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?

:)
Comment 13 Stephan Szabo 2021-10-07 08:50:16 PDT
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 14 Adrian Perez 2021-10-07 12:29:32 PDT
Comment on attachment 440399 [details]
Patch version using find on remote and ruby local

Let me know if you need a committer
Comment 15 Adrian Perez 2021-10-07 12:30:23 PDT
(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 :)
Comment 16 Adrian Perez 2021-10-07 12:31:21 PDT
(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.

=)
Comment 17 EWS 2021-10-07 14:29:47 PDT
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].
Comment 18 Radar WebKit Bug Importer 2021-10-07 14:30:20 PDT
<rdar://problem/83999466>