Make landmines.py take an extra script via an env var.

Make landmines.py take an extra script via an env var.

The extra Python script to run can be specified via env var
EXTRA_LANDMINES_SCRIPT.

BUG=223636
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220846

[iannucci, 2013-08-30 22:07:51]

https://codereview.chromium.org/23604016/diff/1/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/23604016/diff/1/build/landmines.py#newcode99
build/landmines.py:99: options, args = parser.parse_args()
Just do:

extra_script = os.environ.get('EXTRA_LANDMINES_SCRIPT')
if extra_script:
  options.landmine_scripts.append(extra_script)

[Siva Chandra, 2013-08-30 22:17:06]

PTaL at patch set 3.

https://codereview.chromium.org/23604016/diff/1/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/23604016/diff/1/build/landmines.py#newcode99
build/landmines.py:99: options, args = parser.parse_args()
On 2013/08/30 22:07:51, iannucci wrote:
> Just do:
> 
> extra_script = os.environ.get('EXTRA_LANDMINES_SCRIPT')
> if extra_script:
>   options.landmine_scripts.append(extra_script)

Done. Some reviewers do not like changing cmd line args.

[iannucci, 2013-08-31 00:54:20]

certainly modifying options far away from the parsing is bad.  I would add a
note to the helptext for the scripts arg noting that it can be modified by this
variable too.

https://codereview.chromium.org/23604016/diff/10001/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/23604016/diff/10001/build/landmines.py#newcod...
build/landmines.py:111: options.landmine_scripts.append(extra_script)
I would put this above the apply_chromium_gyp_env. In fact, I would pull all the
argument parsing (including this) into its own function which just returns
landmine_scripts.

[iannucci, 2013-08-31 01:18:59]

lgtm % whitespace

https://codereview.chromium.org/23604016/diff/15001/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/23604016/diff/15001/build/landmines.py#newcod...
build/landmines.py:102: if args:
I'd keep this whitespace line

https://codereview.chromium.org/23604016/diff/15001/build/landmines.py#newcod...
build/landmines.py:126: 
and remove this one

[commit-bot: I haz the power, 2013-08-31 01:37:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/23604016/15002

[commit-bot: I haz the power, 2013-08-31 05:08:11]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-31 12:49:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/23604016/15002

[commit-bot: I haz the power, 2013-08-31 16:05:18]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-02 13:51:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/23604016/15002

[commit-bot: I haz the power, 2013-09-02 14:59:28]

Change committed as 220846