1410459 - [mozharness] Upgrade to argparse for config parsing

Status: NEW

(Release Engineering :: Applications: MozharnessCore, enhancement)

Description

[Andrew Halberstadt [:ahal]]

I was trying to fix bug 1391130 with some light command line magic, but realized that mozharness is still using optparse to parse the command line and the feature I needed isn't supported (argparse.REMAINDER).

There's probably some hacky optparse way to accomplish what I need, but it'll be just as easy to upgrade to argparse.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1410459 - [mozharness] Upgrade from optparse to argparse in config.py,

Upgrade from optparse to argparse:

1. 'type' field now needs to be callable (deleted if type was 'string' as that is the default)
2. 'extend' action re-implemented for argparse
3. 'callback' action no longer exists, re-implemented as a custom argparse action (only used in buildbase.py)
4. minor api changes, e.g 'add_option' -> 'add_argument'

Review commit: https://reviewboard.mozilla.org/r/192034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192034/

Comment 2

[Andrew Halberstadt [:ahal]]

This ended up being a little more complicated than I thought, but this patch seems to work:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b7668a9f6b6226fd895cc6560ffc6f4daf66516

I don't know if there's additional testing that's needed. This could technically affect the more "releng-y" jobs like balrog, beetmover, etc, though I think the risk is pretty small. (I'm not sure how to test these jobs, I don't have scopes to run them on try).

Comment 3

[Jordan Lund (:jlund)]

yikes, not an easy version bump :)

I'll have a look tomorrow to give full review.

fwiw, we are near ready to start doing staging releases. This means we will have a way to test patches like this prior to a release \o/. Hopefully we start advertising a way to do this in the next quarter.

Until then, we can just land patches like this and do a new build_num (release) if it causes bustage.

Comment 4

[Jordan Lund (:jlund)]

Comment on attachment 8921064 [details]
Bug 1410459 - [mozharness] Upgrade from optparse to argparse in config.py,

https://reviewboard.mozilla.org/r/192034/#review198354

so it looks good to me. Nice work by the way. I didn't scrutinize heavily line by line as it was hard to keep track of what variable names map to what piece of config management. A reflection of my own build and config code that's more complex than it needs to be rather than of your patch itself. I think as long as this passes tests and try, I'm not so concerned about release automation. Particularly since the changes to the end scripts themselves are largely about changing how we define options, e.g. "int" vs int

Comment 5

[Andrew Halberstadt [:ahal]]

The previously linked try run has a good smattering of builds and tests across most of the mozharness scripts, so I'm pretty confident there. I'll also check the hg log to make sure no new arguments slipped in in the meantime.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8921064 [details]
Bug 1410459 - [mozharness] Upgrade from optparse to argparse in config.py,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192034/diff/1-2/

Comment 7

[Andrew Halberstadt [:ahal]]

The latest push just fixes some instances under 'testing/mozharness/examples' that I previously missed. I also grepped for cases of 'type': {'choices', 'string', 'int'} and found nothing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1455a5d2e05
[mozharness] Upgrade from optparse to argparse in config.py, r=jlund

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for breaking talos:

https://hg.mozilla.org/integration/autoland/rev/738a4b6c0dadebc3dfe20fa86520eb4c0c904b71

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e1455a5d2e050fd75598cc53365040cd10843a6a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139902292&repo=autoland

========= Started '/tools/buildbot/bin/python scripts/scripts/talos_script.py ...' failed (results: 2, elapsed: 0 secs) (at 2017-10-26 08:50:56.272930) =========
/tools/buildbot/bin/python scripts/scripts/talos_script.py --suite h1-e10s --add-option --webServer,localhost --branch-name Autoland-Non-PGO --cfg talos/linux_config.py --download-symbols ondemand --use-talos-json --blob-upload-branch Autoland-Non-PGO
 in dir /builds/slave/test/. (timeout 3600 secs) (maxTime 7200 secs)
 watching logfiles {}
 argv: ['/tools/buildbot/bin/python', 'scripts/scripts/talos_script.py', '--suite', 'h1-e10s', '--add-option', '--webServer,localhost', '--branch-name', 'Autoland-Non-PGO', '--cfg', 'talos/linux_config.py', '--download-symbols', 'ondemand', '--use-talos-json', '--blob-upload-branch', 'Autoland-Non-PGO']
 environment:
  DISPLAY=:0
  HOME=/home/cltbld
  LANG=en_US.UTF-8
  LANGUAGE=en_US:en
  LOGNAME=cltbld
  MAIL=/var/mail/cltbld
  MOZ_CRASHREPORTER_NO_REPORT=1
  MOZ_NO_REMOTE=1
  NODE_PATH=/usr/lib/nodejs:/usr/lib/node_modules:/usr/share/javascript
  NO_EM_RESTART=1
  PATH=/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
  PROPERTIES_FILE=/builds/slave/test/buildprops.json
  PWD=/builds/slave/test
  SHELL=/bin/bash
  SHLVL=1
  TERM=linux
  TMOUT=86400
  USER=cltbld
  XDG_SESSION_COOKIE=523ca41ca1e0623de333ad52000001bf-1509033003.832902-1712798542
  XPCOM_DEBUG_BREAK=warn
  _=/tools/buildbot/bin/python
 using PTY: False
usage: talos_script.py [-h] [--work-dir WORK_DIR]
                       [--base-work-dir BASE_WORK_DIR] [-c CONFIG_FILES]
                       [-C OPT_CONFIG_FILES] [--dump-config]
                       [--dump-config-hierarchy]
                       [--log-level {debug,info,warning,error,critical,fatal}]
                       [-q] [--append-to-log] [--multi-log] [--simple-log]
                       [--list-actions] [--add-action ACTIONS]
                       [--no-action ACTIONS] [--clobber] [--no-clobber]
                       [--read-buildbot-config] [--no-read-buildbot-config]
                       [--download-and-extract] [--no-download-and-extract]
                       [--populate-webroot] [--no-populate-webroot]
                       [--create-virtualenv] [--no-create-virtualenv]
                       [--install] [--no-install] [--setup-mitmproxy]
                       [--no-setup-mitmproxy] [--run-tests] [--no-run-tests]
                       [--use-talos-json] [--suite SUITE]
                       [--branch-name BRANCH] [--system-bits {32,64}]
                       [--add-option TALOS_EXTRA_OPTIONS] [--geckoProfile]
                       [--geckoProfileInterval GECKO_PROFILE_INTERVAL]
                       [--enable-stylo] [--disable-stylo] [--enable-webrender]
                       [--installer-url INSTALLER_URL]
                       [--installer-path INSTALLER_PATH]
                       [--binary-path BINARY_PATH] [--exe-suffix EXE_SUFFIX]
                       [--test-url TEST_URL]
                       [--test-packages-url TEST_PACKAGES_URL]
                       [--jsshell-url JSSHELL_URL]
                       [--download-symbols {ondemand,true}]
                       [--virtualenv-path VIRTUALENV_PATH]
                       [--find-links FIND_LINKS] [--pip-index]
                       [--no-pip-index] [--try-message TRY_MESSAGE] [--verify]
                       [--blob-upload-branch BLOB_UPLOAD_BRANCH]
                       [--blob-upload-server BLOB_UPLOAD_SERVERS]
                       [--code-coverage] [--disable-ccov-upload]
talos_script.py: error: argument --add-option: expected one argument
program finished with exit code 2

Comment 10

[Andrew Halberstadt [:ahal]]

There's something up with the custom argparse 'extend' action. I'm not sure why it's working for builds and tests but not talos though. I'll have to dig in further.

Comment 11

[Andrew Halberstadt [:ahal]]

Oh, it's because talos is passing in:
--add-option "--webserver,default"

Looks like argparse is treating the '--webserver' as a brand new argument instead of as the value of --add-option.

Comment 12

[Andrew Halberstadt [:ahal]]

So the only way I can figure out how to fix this is to change the command line to:
--add-option="--webserver,default"

Unfortunately most talos jobs are still scheduled over BBB, so this will need to be fixed in buildbotcustom. (Or I could block on talos tasks migrating off of buildbot). The bug this one is blocking, is also blocked on the buildbot migration.. so maybe I should just wait it out.

Comment 13

[Andrew Halberstadt [:ahal]]

The talos issue shouldn't exist anymore, now that it is on Taskcluster. Unfortunately I don't have time atm to rebase this (and do the requisite testing to be reasonably confident it won't break something).

If anyone wants to commandeer this feel free. It might not be terribly difficult.