Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 10 years ago

in talos we have a standalone script xtalos/xtalos.py which takes commandline arguments.  It would be really nice to convert the script to use argparse instead of optparse.  Here is some information about converting that:
https://argparse.googlecode.com/svn/trunk/doc/argparse-vs-optparse.html

	Vidya Assignee
	Comment 2 • 10 years ago

Hey Joel,

Can you test this in Windows 7 and let me know if there is any error?

Thank you so much.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 3 • 10 years ago

Comment on attachment 8545365 [details] [diff] [review]
Switch from optparse module to argparse

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

this looks great, I am testing it out now!

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 4 • 10 years ago

It looks like we have a slight issue:
12:10:17    ERROR -  Traceback (most recent call last):
12:10:17     INFO -    File "C:\slave\test\build\venv\lib\site-packages\talos/xtalos/start_xperf.py", line 78, in <module>
12:10:17     INFO -      main()
12:10:17     INFO -    File "C:\slave\test\build\venv\lib\site-packages\talos/xtalos/start_xperf.py", line 66, in main
12:10:17     INFO -      parser = xtalos.XtalosOptions()
12:10:17     INFO -    File "C:\slave\test\build\venv\lib\site-packages\talos\xtalos\xtalos.py", line 99, in __init__
12:10:17     INFO -      self.set_usage('')
12:10:17     INFO -  AttributeError: 'XtalosOptions' object has no attribute 'set_usage'

This should be fairly easy to sort out.

	Vidya Assignee
	Comment 5 • 10 years ago

Hi Joel,

Sorry for the error. Please check now. 

Thanks.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 6 • 10 years ago

ok, we have another issue here:
06:05:26     INFO -  -----------------------------------------------------------------------------
06:05:26     INFO -  output of setup command:
06:05:26     INFO -  usage:
06:05:26     INFO -  start_xperf.py: error: unrecognized arguments: C:\slave\test\build\venv\lib\site-packages\talos/bcontroller.yml
06:05:26     INFO -  end of setup command output
06:05:26     INFO -  -----------------------------------------------------------------------------


Quite possibly you could run these locally just to make sure the command line arguments are accurate.

We could have it take the input config file via a cli argument.  make sure the sys.argv references here are resolved:
http://hg.mozilla.org/build/talos/file/3921f6afbd84/talos/xtalos/start_xperf.py#l63

	Vidya Assignee
	Comment 7 • 10 years ago

Hi Joel,

Thanks for testing this. It's affecting 2 more files apart from start_xperf.py:
etlparser.py
parse_xperf.py

So I tried running: python start_xperf.py --help

Before modifying the code it shows:

Options:
  -h, --help            show this help message and exit
  --pid=PROCESSID       process ID of the process we launch
  -x XPERF_PATH, --xperf=XPERF_PATH
                        location of xperf tool, defaults to 'xperf.exe'
  -e ETL_FILENAME, --etl_filename=ETL_FILENAME
                        Name of the .etl file to work with. Defaults to
                        'output.etl'

   .
   .
   .

After modifying the code I get(by adding self.usage=''):
usage: 

optional arguments:
  -h, --help            show this help message and exit
  --pid PROCESSID       process ID of the process we launch
  -x XPERF_PATH, --xperf XPERF_PATH
                        location of xperf tool, defaults to 'xperf.exe'
  -e ETL_FILENAME, --etl_filename ETL_FILENAME
                        Name of the .etl file to work with. Defaults to
                        'output.etl'
 .
 .
 .

 I also tried to modify the code by removing line: http://hg.mozilla.org/build/talos/file/3921f6afbd84/talos/xtalos/xtalos.py#99

This is the output:

usage: start_xperf.py [-h] [--pid PROCESSID] [-x XPERF_PATH] [-e ETL_FILENAME]
                      [-d DEBUG_LEVEL] [-o OUTPUTFILE] [-r XPERF_PROVIDERS]
                      [--user-providers XPERF_USER_PROVIDERS]
                      [-s XPERF_STACKWALK] [-c CONFIGFILE] [-w WHITELIST_FILE]
                      [-i] [-t] [-a APPROOT] [--error-filename ERROR_FILENAME]

optional arguments:
  -h, --help            show this help message and exit
  --pid PROCESSID       process ID of the process we launch
  -x XPERF_PATH, --xperf XPERF_PATH
                        location of xperf tool, defaults to 'xperf.exe'
  -e ETL_FILENAME, --etl_filename ETL_FILENAME
                        Name of the .etl file to work with. Defaults to
                        'output.etl'
   .
   .
   .
   .

It's better if I remove the line 99. 

Let me know your opinion.

	Vidya Assignee
	Comment 8 • 10 years ago

This modifies files start_cperf.py,xtalos,py,etl_parser.py and parse_xperf.py

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 9 • 10 years ago

Comment on attachment 8546151 [details] [diff] [review]
Switch from optparse module to argparse

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

this is still failing.  How about we add a --config option to the xtalos available options, then modify test.py where we call it:
http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l410

	Vidya Assignee
	Comment 10 • 10 years ago

There already exists one in xtalos.py

self.add_argument("-c", "--config-file", dest="configFile",
                        help="Name of the yaml config file with test run and browser information")
        defaults["configFile"] = None

Isn't this the same thing?

Can you provide me with error message?

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 11 • 10 years ago

14:21:28     INFO -  -----------------------------------------------------------------------------
14:21:28     INFO -  output of setup command:
14:21:28     INFO -  usage: start_xperf.py [-h] [--pid PROCESSID] [-x XPERF_PATH] [-e ETL_FILENAME]
14:21:28     INFO -                        [-d DEBUG_LEVEL] [-o OUTPUTFILE] [-r XPERF_PROVIDERS]
14:21:28     INFO -                        [--user-providers XPERF_USER_PROVIDERS]
14:21:28     INFO -                        [-s XPERF_STACKWALK] [-c CONFIGFILE] [-w WHITELIST_FILE]
14:21:28     INFO -                        [-i] [-t] [-a APPROOT] [--error-filename ERROR_FILENAME]
14:21:28     INFO -  start_xperf.py: error: unrecognized arguments: C:\slave\test\build\venv\lib\site-packages\talos/bcontroller.yml
14:21:28     INFO -  end of setup command output
14:21:28     INFO -  -----------------------------------------------------------------------------

I think just modifying test.py to use th existing -c would make it work!

	Vidya Assignee
	Comment 12 • 9 years ago

Hi Joel,

Apologies for the delay.

Is this how it (http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l410) should be modified:

setup = '${talos}/xtalos/start_xperf.py -c ${talos}/bcontroller.yml'

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 13 • 9 years ago

yes, change (http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l418):
setup = '${talos}/xtalos/start_xperf.py ${talos}/bcontroller.yml'

to:
setup = '${talos}/xtalos/start_xperf.py -c ${talos}/bcontroller.yml'

there is both setup/cleanup tasks :)

	Vidya Assignee
	Comment 14 • 9 years ago

Have modified test.py to include -c in lines 418 and 419

Let me know if this still produces any error.

Thanks Joel.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 15 • 9 years ago

Comment on attachment 8554694 [details] [diff] [review]
Convert xtalos.py to have argparse instead of optparse module

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

This still fails on the try server with this error:
19:17:54     INFO -  -------- Summary: end --------
19:17:54     INFO -  extending with xperf!
19:17:54    ERROR -  Traceback (most recent call last):
19:17:54     INFO -    File "C:\slave\test\build\venv\lib\site-packages\talos/xtalos/parse_xperf.py", line 71, in <module>
19:17:54     INFO -      main()
19:17:54     INFO -    File "C:\slave\test\build\venv\lib\site-packages\talos/xtalos/parse_xperf.py", line 64, in main
19:17:54     INFO -      stop_from_config(config_file=args[0],

Just a few small areas overlooked.

::: talos/xtalos/etlparser.py
@@ +429,5 @@
>  
>      # call etlparser
>      etlparser(**args)
>  
>  def main(args=sys.argv[1:]):

here we use sys.argv[1:]

::: talos/xtalos/parse_xperf.py
@@ +61,4 @@
>  
>      # start xperf
>      try:
>          stop_from_config(config_file=args[0],

we also use args[0] here

::: talos/xtalos/start_xperf.py
@@ +68,4 @@
>  
>      # start xperf
>      try:
>          start_from_config(config_file=args[0],

see the args[0] here, we need to use args.config or something named

	Vidya Assignee
	Comment 16 • 9 years ago

Sorry Joel I missed that.

If I change parse_xperf and start_xperf to have 
       config_file = args.configFile,

That would clear away the errors, right?

Or am I missing anything else?

Also there won't be any change in etlparser.py

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 17 • 9 years ago

we still reference sys.argv and argv[0] in places.  I don't believe we can realistically do that.  You should be able to run the scripts locally with made up arguments and see if you get an error outside of the commandline parsing :)

I don't believe you need to change anything in etlparser.py.

	Vidya Assignee
	Comment 18 • 9 years ago

Hi Joel,

As you suggested I tested this locally with the following made up arguments:

    $  python start_xperf.py -c ts_desktop.yml -s blah --user-providers vidya -r yeah -x /Desktop/blah.exe 

Traceback (most recent call last):
  File "start_xperf.py", line 78, in <module>
    main()
  File "start_xperf.py", line 73, in main
    **args.__dict__)
  File "start_xperf.py", line 61, in start_from_config
    start(**args)
  File "start_xperf.py", line 28, in start
    subprocess.call(xperf_cmd)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 522, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied

Returns an OS error. 

Does that mean there is no error related to command line parsing. Should I submit a patch?

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 19 • 9 years ago

hey, this looks like it made it paste the commandline parsing, lets look at the patch!

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 21 • 9 years ago

Comment on attachment 8558074 [details] [diff] [review]
Convert xtalos.py to have argparse instead of optparse module

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

thanks!

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 22 • 9 years ago

https://hg.mozilla.org/build/talos/rev/60d6980dec28