1283911 - Convert autospider.sh to Python

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

The shell script has gotten a little out of hand.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Convert autospider.sh to autospider.py, and switch to using JSON for the variants files

...though if I'd known how much trouble it would be, I probably wouldn't have started doing this. Dealing with the Windows path styles burned way, way more time than it should have. Especially since my local test wasn't a great match for CI because CI uses a vs2015u2 package that I don't have.

Anyway, it's ugly but it's working finally.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8767295 [details] [diff] [review]
Convert autospider.sh to autospider.py, and switch to using JSON for the variants files

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

Looks like this is the wrong patch, so I'm not sure how helpful these comments will be.

::: js/src/devtools/automation/autospider.py
@@ +119,5 @@
> +        os.mkdir(name)
> +    except OSError:
> +        if clobber:
> +            raise
> +        pass

Extraneous |pass|.

@@ +182,5 @@
> +                         ['PATH', 'INCLUDE', 'LIB', 'LIBPATH', 'CC', 'CXX'])
> +
> +if word_bits == 64:
> +    if platform.system() == 'Windows':
> +        CONFIGURE_ARGS += ' --target=x86_64-pc-mingw32 --host=x86_64-pc-mingw32'

Wait, we build the shell with mingw32 on windows? I'd think that since we build the browser with cl, we'd want to test the same build tool for shell builds.

@@ +189,5 @@
> +        env['CC'] = '{CC} -arch i386'.format(**env)
> +        env['CXX'] = '{CXX} -arch i386'.format(**env)
> +    elif platform.system() != 'Windows':
> +        env.setdefault('CC', 'gcc')
> +        env.setdefault('CXX', 'g++')

What the...? Am I looking at the right patch?

::: js/src/devtools/automation/variants/compacting
@@ +7,5 @@
> +    },
> +    "skip-tests": {
> +        "win32": ["jstests"],
> +        "win64": ["jstests"]
> +    }

It's awesome how these files now encode all the details about how the target gets run as well!

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 8767295 [details] [diff] [review]
> Convert autospider.sh to autospider.py, and switch to using JSON for the
> variants files
> 
> Review of attachment 8767295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like this is the wrong patch, so I'm not sure how helpful these
> comments will be.

Actually, it looks like you *do* have the current version.

> @@ +182,5 @@
> > +                         ['PATH', 'INCLUDE', 'LIB', 'LIBPATH', 'CC', 'CXX'])
> > +
> > +if word_bits == 64:
> > +    if platform.system() == 'Windows':
> > +        CONFIGURE_ARGS += ' --target=x86_64-pc-mingw32 --host=x86_64-pc-mingw32'
> 
> Wait, we build the shell with mingw32 on windows? I'd think that since we
> build the browser with cl, we'd want to test the same build tool for shell
> builds.

We do build with cl.exe. But I just double-checked with firefox builds, and on 32-bit we use --target=i686-pc-mingw32 --host=i686-pc-mingw32 and on 64-bit we use --target=x86_64-pc-mingw32 --host=x86_64-pc-mingw32. I don't know exactly what effect those flags have, but they do seem to produce the right thing. (Though I'm worried now that the win32 one might now or in the future be a 64-bit build; leaving off the options as the shell build does now seems dangerous.)


> 
> @@ +189,5 @@
> > +        env['CC'] = '{CC} -arch i386'.format(**env)
> > +        env['CXX'] = '{CXX} -arch i386'.format(**env)
> > +    elif platform.system() != 'Windows':
> > +        env.setdefault('CC', 'gcc')
> > +        env.setdefault('CXX', 'g++')
> 
> What the...? Am I looking at the right patch?

Why? This is saying the non-Windows default is gcc. (This logic is going to change a bit to support tsan builds, but that doesn't really matter here.)

> ::: js/src/devtools/automation/variants/compacting
> @@ +7,5 @@
> > +    },
> > +    "skip-tests": {
> > +        "win32": ["jstests"],
> > +        "win64": ["jstests"]
> > +    }
> 
> It's awesome how these files now encode all the details about how the target
> gets run as well!

I wish I could move the rest of the gunk to those files, but unless I want to make one file per platform x config x shell configuration x ..., I can't quite.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8767295 [details] [diff] [review]
Convert autospider.sh to autospider.py, and switch to using JSON for the variants files

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

Okay, upon closer reading, this is fine.

::: js/src/devtools/automation/autospider.py
@@ +189,5 @@
> +        env['CC'] = '{CC} -arch i386'.format(**env)
> +        env['CXX'] = '{CXX} -arch i386'.format(**env)
> +    elif platform.system() != 'Windows':
> +        env.setdefault('CC', 'gcc')
> +        env.setdefault('CXX', 'g++')

Ah, I see: "!=". Immediately after we have an == on the same condition and before we have another == on the same condition. How about we move the != Windows block down and make it an else block instead.

@@ +224,5 @@
> +    finally:
> +        ACTIVE_PROCESSES.discard(proc)
> +    status = proc.wait()
> +    if check and status != 0:
> +        raise(subprocess.CalledProcessError(status, command, output=stderr))

|raise| is a keyword, not a function, so the outer parens here are useless. Please make this |raise subprocess.Called....|.

@@ +294,5 @@
> +    results.append(run_test_command([MAKE, 'check-jstests']))
> +
> +for st in results:
> +    if st != 0:
> +        sys.exit(st)

This might be clearer as:

failures = set(results) - {0}
if failures:
    sys.exit(failures.pop())

Or maybe just:

if any(results):
    sys.exit((set(results) - {0}).pop())

Or maybe I'm overthinking it and a simple iterative solution is clearest.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #4)
> @@ +294,5 @@
> > +    results.append(run_test_command([MAKE, 'check-jstests']))
> > +
> > +for st in results:
> > +    if st != 0:
> > +        sys.exit(st)
> 
> This might be clearer as:
> 
> failures = set(results) - {0}
> if failures:
>     sys.exit(failures.pop())
> 
> Or maybe just:
> 
> if any(results):
>     sys.exit((set(results) - {0}).pop())
> 
> Or maybe I'm overthinking it and a simple iterative solution is clearest.

I think I'll stick with the iteration, because I'd like to preserve the semantics in the comment:

# Always run all enabled tests, even if earlier ones failed. But return the
# first failed status.

The set() would explicitly discard the ordering. In practice, I'm guessing Python would always choose a consistent ordering between runs, so it's not like the behavior would vary much.

I could definitely see an argument for sys.exit(max(results)), though. Or perhaps sys.exit(max(results, key=lambda st: abs(st))) to get signal throws. But that would be implying that we have some semantics to our different error codes that we probably don't have, so I'd rather not make that implication.

(This isn't completely pedantic, btw -- I think some status codes may get mapped differently to red vs orange on treeherder. So right now adding an earlier orange could obscure a later red. But I don't want to have to depend on that mapping here, so I'd rather just pick the simple "first failure wins".)

Comment 6

[Terrence Cole [:terrence]]

Nice! Thanks for the explanation, I didn't realize there was so much riding on the order.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8d7bc4e780
Convert autospider.sh to autospider.py, and switch to using JSON for the variants files, r=terrence

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b8d7bc4e780

Comment 9

[Sander Mathijs van Veen]

There is a bug here (https://hg.mozilla.org/mozilla-central/annotate/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/js/src/devtools/automation/autospider.py#l136) that prevents building arm-sim when autoconf-2.13 does not exist but autoconf2.13 does. The bug is causes by checking if call_alternatives returns a non-zero value, but it will always return None (implicitly).

A fix for this issue could be:

    diff --git a/js/src/devtools/automation/autospider.py b/js/src/devtools/automation/autospider.py
    --- a/js/src/devtools/automation/autospider.py
    +++ b/js/src/devtools/automation/autospider.py
    @@ -135,28 +139,32 @@ if args.variant == 'nonunified':
         # Note that this modifies the current checkout.
         for dirpath, dirnames, filenames in os.walk(DIR.js_src):
             if 'moz.build' in filenames:
                 subprocess.check_call(['sed', '-i', 's/UNIFIED_SOURCES/SOURCES/',
                                        os.path.join(dirpath, 'moz.build')])
     
     if not args.nobuild:
         autoconfs = ['autoconf-2.13', 'autoconf2.13', 'autoconf213']
    -    if call_alternates(autoconfs, [], cwd=DIR.js_src) != 0:
    +    try:
    +        call_alternates(autoconfs, [], cwd=DIR.js_src)
    +    except OSError:
             logging.error('autoconf failed')
             sys.exit(1)

Comment 10

[Steve Fink [:sfink] [:s:]]

I don't follow.

call_alternates should never be returning None. It is doing |return subprocess.call(...)| within call_alternates, which should return the return code of the call unless it raises an exception.

In your example, autoconf-2.13 doesn't exist, so subprocess.call should throw an OSError, which will cause it to try autoconf2.13, which should run and return its exit code.

Are you seeing different behavior?

Comment 11

[Sander Mathijs van Veen]

I'm getting the following output without the fix:

smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ (cd ~/work/leaningtech/gecko-hg/; python2.7 js/src/devtools/automation/autospider.py arm-sim)
sh: 1: autoconf-2.13: not found
ERROR:root:autoconf failed
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ which autoconf2.13
/usr/bin/autoconf2.13
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ which autoconf-2.13
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ lsb_release  -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04 LTS
Release:        16.04
Codename:       xenial

Comment 12

[Steve Fink [:sfink] [:s:]]

Ah! Ok, is because my Windows fix broke call_alternates. I switched from invoking the binary directly to running it via sh -c. I think that's probably because other things required sh -c so that I could use the right type of paths? I don't see why it's needed here, and in fact it breaks the (unused) command_args parameter since that'll get passed to sh instead of the command.

Ugh.

Let me try going back to calling it directly and see if a try push gets upset about it. If so, then I'll have to use the return value (it'll be 127 if sh couldn't find the program.)

I don't understand why your fix works, though. On my machine, I don't get an OSError at all if I'm running sh. I would have expected the failed autoconf-2.13 to return an error value, then the script to continue on as if the autoconf had succeeded.

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: fix alternate binary searching in autospider.py

Ok, how about this? It converts the shell return of 127 (command not found) into an OSError so it can follow the same path. Even if different systems handle this differently, this should make them the same?

Comment 14

[Sander Mathijs van Veen]

Comment on attachment 8776337 [details] [diff] [review]
fix alternate binary searching in autospider.py

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

I can confirm that this works on Ubuntu 16.04. Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbed984c0df
fix alternate binary searching in autospider.py, r=sandervv

Comment 16

[Mike Hommey [:glandium]]

And yet another thing not reviewed by a build system peer, who would have told you autospider.py shouldn't be bothering with autoconf in the first place...

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ffbed984c0df

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: Make it possible to copy configure.in to configure

It's a total hack (and it prevents you from using eg parentheses in dnl comments), but it's kind of nice to be able to do

  cp configure.in configure
  chmod +x configure

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Stop running autoconf in autospider.py

Stop running autoconf.

Comment 20

[Steve Fink [:sfink] [:s:]]

Oops, shouldn't be doing this on a FIXED bug. Migrating patches to bug 1295751.

Comment 21

[Steve Fink [:sfink] [:s:]]

Attachment: Stop running autoconf in autospider.py

MozReview-Commit-ID: 9COvbxsmRf2

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8782046 [details] [diff] [review]
Stop running autoconf in autospider.py

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

This is the wrong bug, btw, this should be attached to bug 1295751