Linux: add script to remove bundled libraries.

Linux: add script to remove bundled libraries.

BUG=none
R=thestig@chromium.org

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

[Lei Zhang, 2013-08-20 23:41:10]

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:7: Removes bundled libraries to
make sure they are not used.
Can you elaborate on what this means exactly?

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:21: for exclusion in argv[1:]:
Document what argv[1:] is?

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:22: status_dict[exclusion] =
{'exists': False}
Why do the values in |status_dict| have to be a dict? Why can't the value just
be a boolean?

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:31: if 'third_party' not in
relpath.split(os.sep):
You don't have to do this on a per-file basis.

for root, dirs, files in os.walk(source_tree_root):
  relpath = os.path.relpath(root, source_tree_root)
  if 'third_party' not in relpath.split(os.sep):
    continue
  for f in files:
    relpath = os.path.join(relpath, f)
    ...

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:35: for exclusion in argv:
Shouldn't you skip argv[0] ?

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:50: os.remove(path)
No dry run mode? Seems dangerous.

[Paweł Hajdan Jr., 2013-08-21 00:42:40]

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:7: Removes bundled libraries to
make sure they are not used.
On 2013/08/20 23:41:10, Lei Zhang wrote:
> Can you elaborate on what this means exactly?

See README updates. Further feedback is welcome.

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:22: status_dict[exclusion] =
{'exists': False}
On 2013/08/20 23:41:10, Lei Zhang wrote:
> Why do the values in |status_dict| have to be a dict? Why can't the value just
> be a boolean?

Done.

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:31: if 'third_party' not in
relpath.split(os.sep):
On 2013/08/20 23:41:10, Lei Zhang wrote:
> You don't have to do this on a per-file basis.
> 
> for root, dirs, files in os.walk(source_tree_root):
>   relpath = os.path.relpath(root, source_tree_root)
>   if 'third_party' not in relpath.split(os.sep):
>     continue
>   for f in files:
>     relpath = os.path.join(relpath, f)
>     ...

Done.

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:35: for exclusion in argv:
On 2013/08/20 23:41:10, Lei Zhang wrote:
> Shouldn't you skip argv[0] ?

Done.

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:50: os.remove(path)
On 2013/08/20 23:41:10, Lei Zhang wrote:
> No dry run mode? Seems dangerous.

I don't think dry run would be useful given how this is supposed to be used. :)

[Lei Zhang, 2013-08-21 01:27:03]

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:7: Removes bundled libraries to
make sure they are not used.
On 2013/08/21 00:42:40, Paweł Hajdan Jr. wrote:
> On 2013/08/20 23:41:10, Lei Zhang wrote:
> > Can you elaborate on what this means exactly?
> 
> See README updates. Further feedback is welcome.

Can you write "See README for more details." here?

https://codereview.chromium.org/23037005/diff/1/build/linux/unbundle/remove_b...
build/linux/unbundle/remove_bundled_libraries.py:50: os.remove(path)
On 2013/08/21 00:42:40, Paweł Hajdan Jr. wrote:
> On 2013/08/20 23:41:10, Lei Zhang wrote:
> > No dry run mode? Seems dangerous.
> 
> I don't think dry run would be useful given how this is supposed to be used.
:)

How about dry run by default (print what is to be deleted) and only delete with
a '--yes-please-delete' flag? Someone is going to shoot themselves in the foot,
and if they do, you can at least point and say "you passed that flag".

https://codereview.chromium.org/23037005/diff/4001/build/linux/unbundle/remov...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/4001/build/linux/unbundle/remov...
build/linux/unbundle/remove_bundled_libraries.py:17: source_tree_root =
os.path.abspath(
Please add some sanity checks to prevent this script from running amok:
- make sure os.path.join(source_tree_root, 'build', 'linux') == my_dirname
- make sure |source_tree_root| doesn't have a '.svn' or '.git' directory.

[Paweł Hajdan Jr., 2013-08-21 18:57:02]

PTAL

I think a sanity check for .svn/.git would be a bit of an overkill given the
command-line switch... and usually people who even look into
build/linux/unbundle know what they're doing.

[Lei Zhang, 2013-08-21 19:10:09]

lgtm

https://codereview.chromium.org/23037005/diff/11001/build/linux/unbundle/remo...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/11001/build/linux/unbundle/remo...
build/linux/unbundle/remove_bundled_libraries.py:31: options, args =
parser.parse_args(argv)
nit: parse_args() defaults to sys.argv[1:], so you can omit |argv| altogether.

[Paweł Hajdan Jr., 2013-08-21 20:14:25]

https://codereview.chromium.org/23037005/diff/11001/build/linux/unbundle/remo...
File build/linux/unbundle/remove_bundled_libraries.py (right):

https://codereview.chromium.org/23037005/diff/11001/build/linux/unbundle/remo...
build/linux/unbundle/remove_bundled_libraries.py:31: options, args =
parser.parse_args(argv)
On 2013/08/21 19:10:10, Lei Zhang wrote:
> nit: parse_args() defaults to sys.argv[1:], so you can omit |argv| altogether.

I like the ability to have a function I can pass args to (instead of global
sys.argv). This is also consistent with replace_gyp_files.py.

I can change it in a follow-up if really needed.

[Paweł Hajdan Jr., 2013-08-21 20:17:46]

Committed patchset #3 manually as r218816 (presubmit successful).