829942 - Make jit_test.py Py3k-readier

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Christian Holler (:decoder)]

Comment on attachment 701460 [details] [diff] [review]
Patch v1

Looks good to me, but deferring review to terrence because I'm not a JS reviewer :)

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 701460 [details] [diff] [review]
Patch v1

Review of attachment 701460 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/jit_test.py
@@ +161,5 @@
>      scriptdir_var = os.path.dirname(path);
>      if not scriptdir_var.endswith('/'):
>          scriptdir_var += '/'
> +    consts = (('platform', sys.platform), ('libdir', libdir_var), ('scriptdir', scriptdir_var))
> +    expr = '; '.join(define_const(k,v) for (k,v) in consts)

This is super-gross and still not adequate for the general case: e.g. what if the scriptdir contains quotes? Just encode the string quotes directly in the format string and use |str.format()| for python3 compatibility.

@@ -575,5 @@
>      max_jobs_default = 1
>      if HAVE_MULTIPROCESSING:
>          try:
>              max_jobs_default = cpu_count()
> -            print "Defaulting to %d jobs in parallel" % max_jobs_default

Thank you!

@@ +760,5 @@
>          if not ok:
>              sys.exit(2)
>      except OSError:
>          if not os.path.exists(JS):
> +            print("JS shell argument: file does not exist: '%s'"%JS, file=sys.stderr)

Use str.format() here.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 701460 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 701460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/jit_test.py
> @@ +161,5 @@
> >      scriptdir_var = os.path.dirname(path);
> >      if not scriptdir_var.endswith('/'):
> >          scriptdir_var += '/'
> > +    consts = (('platform', sys.platform), ('libdir', libdir_var), ('scriptdir', scriptdir_var))
> > +    expr = '; '.join(define_const(k,v) for (k,v) in consts)
> 
> This is super-gross and still not adequate for the general case: e.g. what
> if the scriptdir contains quotes? Just encode the string quotes directly in
> the format string and use |str.format()| for python3 compatibility.

Will do.

> @@ -575,5 @@
> >      max_jobs_default = 1
> >      if HAVE_MULTIPROCESSING:
> >          try:
> >              max_jobs_default = cpu_count()
> > -            print "Defaulting to %d jobs in parallel" % max_jobs_default
> 
> Thank you!

Decoder secretly already landed that :)

Comment 4

[Jeff Hammel]

Comment on attachment 701460 [details] [diff] [review]
Patch v1

what terrence said. I would have tended to have define_const => define_consts and take all the arguments, returning `expr = '; '.join(define_const(k,v) for (k,v) in consts)` from that function, but no big deal

Comment 5

[Chris Peterson [:cpeterson]]

Ms2ger: did you forget to land this r+'d patch from January 2013? :)

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think I hit some issue on try at the time... That seems to have been magically fixed, though, so I'll land it.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1971bf0f83eb