Refactor landmines.py so that it can be used downstream.

Refactor landmines.py so that it can be used downstream.

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

[iannucci, 2013-08-15 21:07:53]

lgtm

[commit-bot: I haz the power, 2013-08-15 21:24:04]

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

[Siva Chandra, 2013-08-15 22:55:18]

PTaL at patch set 2. Modified based on offline discussion. Tested on my
workstation.

[Siva Chandra, 2013-08-16 19:11:50]

On 2013/08/15 22:55:18, Siva Chandra wrote:
> PTaL at patch set 2. Modified based on offline discussion. Tested on my
> workstation.

ping?

[iannucci, 2013-08-16 20:26:39]

On 2013/08/16 19:11:50, Siva Chandra wrote:
> On 2013/08/15 22:55:18, Siva Chandra wrote:
> > PTaL at patch set 2. Modified based on offline discussion. Tested on my
> > workstation.
> 
> ping?

Looks like landmines.py is missing from patch 2, try a re-upload?

[iannucci, 2013-08-16 20:28:33]

https://chromiumcodereview.appspot.com/22935010/diff/30001/build/get_landmine...
File build/get_landmines.py (right):

https://chromiumcodereview.appspot.com/22935010/diff/30001/build/get_landmine...
build/get_landmines.py:27: target is 'Release' or 'Debug'
maybe update this comment while you're at it (target can be {Release,Debug}_x64
on windows)

[Siva Chandra, 2013-08-16 20:57:38]

PTAL

[iannucci, 2013-08-16 22:10:27]

looking good

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py
File build/landmines.py (right):

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py...
build/landmines.py:68: f.writelines(new_landmines)
Since writelines interposes newlines, you don't want new_landmines to contain
newlines.

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py...
build/landmines.py:89: default=[SRC_DIR + '/build/get_landmines.py'],
os.path.join . '/' is wrong on windows.

Also maybe -s, --landmine-scripts

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py...
build/landmines.py:90: help='Path to the script which emits landmines to
stdout')
Mention that the target is passed in with -t

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py...
build/landmines.py:110: print s
print 'Getting landmines from `%s -t %s`' % (s, target)

https://chromiumcodereview.appspot.com/22935010/diff/27002/build/landmines.py...
build/landmines.py:113: landmines.extend([('%s\n' % l) for l in
output.split('\n')])
landmines.extend(x.strip() for x in output.splitlines())

[Siva Chandra, 2013-08-16 22:36:15]

PTaL at the latest patchset.

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py
File build/landmines.py (right):

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py#newcode68
build/landmines.py:68: f.writelines(new_landmines)
On 2013/08/16 22:10:28, iannucci wrote:
> Since writelines interposes newlines, you don't want new_landmines to contain
> newlines.

Discussed offline.

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py#newcode89
build/landmines.py:89: default=[SRC_DIR + '/build/get_landmines.py'],
On 2013/08/16 22:10:28, iannucci wrote:
> os.path.join . '/' is wrong on windows.
> 
> Also maybe -s, --landmine-scripts

Done.

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py#newcode90
build/landmines.py:90: help='Path to the script which emits landmines to
stdout')
On 2013/08/16 22:10:28, iannucci wrote:
> Mention that the target is passed in with -t

Done.

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py#newcod...
build/landmines.py:110: print s
On 2013/08/16 22:10:28, iannucci wrote:
> print 'Getting landmines from `%s -t %s`' % (s, target)

Ah! That was a leftover statement from my testing. Did the way you suggest now.

https://codereview.chromium.org/22935010/diff/27002/build/landmines.py#newcod...
build/landmines.py:113: landmines.extend([('%s\n' % l) for l in
output.split('\n')])
On 2013/08/16 22:10:28, iannucci wrote:
> landmines.extend(x.strip() for x in output.splitlines())

Done differently now.

[Siva Chandra, 2013-08-19 19:01:45]

ping?

[iannucci, 2013-08-19 22:00:35]

lgtm

[commit-bot: I haz the power, 2013-08-20 16:44:45]

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

[commit-bot: I haz the power, 2013-08-20 17:46:37]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
net_unittests, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-20 19:21:42]

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

[commit-bot: I haz the power, 2013-08-21 02:45:01]

Change committed as 218605

[jamesr, 2013-08-22 20:37:32]

https://chromiumcodereview.appspot.com/22935010/diff/76001/build/landmines.py
File build/landmines.py (right):

https://chromiumcodereview.appspot.com/22935010/diff/76001/build/landmines.py...
build/landmines.py:112: print 'Getting landmines from `%s -t %s`' % (s, target)
Please revert this change.  It's spewing useless console output for all
developers.  Only output something to the console if there's an error condition
the user needs to deal with.