Bug 175216

Summary: Separate jsc stress test script writer from run-jsc-stress-tests
Product: WebKit Reporter: Stephan Szabo <stephan.szabo>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, commit-queue, don.olmstead, fpizlo, keith_miller, lforschler, mark.lam, msaboff, pvollan, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 174985    
Attachments:
Description Flags
Separate out script building/running from other parts of run-jsc-stress-tests
mark.lam: review-
Separate out script building/running from other parts of run-jsc-stress-tests none

Description Stephan Szabo 2017-08-04 15:28:39 PDT
For running JSC tests on windows without a unix shell, in https://lists.webkit.org/pipermail/webkit-dev/2017-August/029370.html we proposed a multiple step process for that with the first being splitting off the test script writing and related code from the other code in run-jsc-stress-tests to eventually allow an alternate implementation that doesn't require shell.
Comment 1 Stephan Szabo 2017-08-07 16:46:00 PDT
Created attachment 317504 [details]
Separate out script building/running from other parts of run-jsc-stress-tests
Comment 2 Mark Lam 2017-08-08 14:50:09 PDT
Comment on attachment 317504 [details]
Separate out script building/running from other parts of run-jsc-stress-tests

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

r- because parseRunCommands is duplicated.

> Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:217
> +#$runCommandOptions = {}

Why have this here?n  This looks like a copy-paste that hasn't been cleaned up.

> Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:320
> +# Returns true if there were run commands found in the file ($benchmarkDirectory +
> +# $benchmark), in which case those run commands have already been executed. Otherwise
> +# returns false, in which case you're supposed to add your own run commands.
> +def parseRunCommands
> +    oldDidAddRunCommand = $didAddRunCommand
> +    $didAddRunCommand = false
> +
> +    Dir.chdir($outputDir) {
> +        File.open($benchmarkDirectory + $benchmark) {
> +            | inp |
> +            inp.each_line {
> +                | line |
> +                begin
> +                    doesMatch = line =~ /^\/\/@/
> +                rescue Exception => e
> +                    # Apparently this happens in the case of some UTF8 stuff in some files, where
> +                    # Ruby tries to be strict and throw exceptions.
> +                    next
> +                end
> +                next unless doesMatch
> +                eval $~.post_match
> +            }
> +        }
> +    }
> +
> +    result = $didAddRunCommand
> +    $didAddRunCommand = result or oldDidAddRunCommand
> +    result
> +end

There's still a copy of this function in run-jsc-stress-tests.  Is there a reason for this?
Comment 3 Stephan Szabo 2017-08-08 15:21:33 PDT
Created attachment 317628 [details]
Separate out script building/running from other parts of run-jsc-stress-tests

Thanks, for parseRunCommands I hadn't intended to move it and missed clearing it out of the writer file with it back in run-jsc-stress-tests. 

The other line was inside a block of things that was moving so I went back and forth but thought it made more sense in the main script. I was swapping which was commented out and forgot to remove the commented out one at the end.
Comment 4 Mark Lam 2017-08-08 15:46:21 PDT
Comment on attachment 317628 [details]
Separate out script building/running from other parts of run-jsc-stress-tests

r=me if EWS is green.
Comment 5 WebKit Commit Bot 2017-08-08 17:18:11 PDT
Comment on attachment 317628 [details]
Separate out script building/running from other parts of run-jsc-stress-tests

Clearing flags on attachment: 317628

Committed r220431: <http://trac.webkit.org/changeset/220431>
Comment 6 WebKit Commit Bot 2017-08-08 17:18:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-08-08 17:18:55 PDT
<rdar://problem/33788991>