Add a tool to check license compatibility with Android

Add a tool to check license compatibility with Android

This tool checks all third-party code used on Android, for the purposes of the Android WebView build.

BUG=138921
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150904

[Steve Block, 2012-07-24 18:47:00]

Open to discussion regarding naming of android_webview. Torne, you said you were
going to/had already started a discussion on this?

[Evan Martin, 2012-07-24 19:27:55]

I think you may underestimate how rapidly the Chrome tree churns.  It would be
wise to colocate metadata with the code you're annotating unless you want this
to be a perpetual source of busywork.

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/third...
File android_webview/tools/third_party_directories.txt (right):

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/third...
android_webview/tools/third_party_directories.txt:10: # individual files.
This is gonna regress every time someone moves a directory, which is pretty
common.  Why not add the below metadata to the README.chromium in each
directory?  It would be useful for non-WebView ports of Chrome too.

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/webvi...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/webvi...
android_webview/tools/webview_licenses.py:93: '(?<!([Cc]opyright|opyright
))\([Cc]\) (19|20)[0-9][0-9]'
Why not fix the case on the code rather than this complicated regex?

It looks like any instance of "the chromium authors" in the Chrome tree (with
that casing) is a typo.

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/webvi...
android_webview/tools/webview_licenses.py:309: 'snapshot.\n\n' \
I think this long string should be the docstring of the module, which you can
retrieve with __doc__.  I.e.:

parser.description = (__doc__ +
   'Commands:\n'
   '  check ... [as before]')

[Evan Martin, 2012-07-24 19:41:28]

two other small comments

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/webvi...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/2006/android_webview/tools/webvi...
android_webview/tools/webview_licenses.py:205: return lines
"lines" is a confusing name, as it is a single string of the contents of the
file.  (You can just "return f.read()" in the with statement to eliminate this
anywya.)

http://codereview.chromium.org/10816041/diff/2006/tools/licenses.py
File tools/licenses.py (right):

http://codereview.chromium.org/10816041/diff/2006/tools/licenses.py#newcode288
tools/licenses.py:288: metadata = ParseDir(path, True)
Consider something like:
  ParseDir(path, raise_on_error=True)
to make the code more self-describing.  (here and below) 


But if the calling code really wants to swallow all LicenseErrors, why not just
wrap the call to ParseDir() with a "try" block that does so rather than passing
this bool?

[Steve Block, 2012-07-24 21:20:58]

Thanks for the quick review Evan.

We have a pretty good idea of the churn here, as we've been maintaing this tool
downstream for about 6 months now, and it hasn't been too bad. My intent was to
limit the impact on existing code, but if you're happy with adding
Android-specific meta-data to the README.chromium files, that sounds good to me.

I think I can eliminate the directory list by moving the data to README.chromium
files, but we'll need to keep the file list, at least for now, as this is by
definition for code not in a third-party directory. Fortunately the churn to the
file list has proved to be significantly less than that to the directory list.

I guess I'll start off by fixing 'licenses.py scan' for the directories I'm
interested in, then add the Android-specific data to these files. I can then
re-work the tool to make use of this.

http://codereview.chromium.org/10816041/diff/2006/tools/licenses.py
File tools/licenses.py (right):

http://codereview.chromium.org/10816041/diff/2006/tools/licenses.py#newcode288
tools/licenses.py:288: metadata = ParseDir(path, True)
Regarding a try block, this approach doesn't work because I need to method to
complete, even if one of the early phases fails and would otherwise raise.

[Evan Martin, 2012-07-24 21:27:09]

On 2012/07/24 21:20:58, Steve Block wrote:
> Thanks for the quick review Evan.

FYI, I don't work on Chrome anymore and ultimately you will need to find a new
reviewer.  I think you can look at the blamelists of various README.chromiums in
the tree to find someone else who has fiddled with this stuff last; I think the
last person added security metadata.

[Steve Block, 2012-07-25 09:27:24]

Thanks Evan

Filed https://code.google.com/p/chromium/issues/detail?id=138921 to track this.

[Steve Block, 2012-08-07 18:41:29]

Ready for another look.

[Nico, 2012-08-07 19:18:36]

http://codereview.chromium.org/10816041/diff/14001/android_webview/tools/thir...
File android_webview/tools/third_party_files_whitelist.txt (right):

http://codereview.chromium.org/10816041/diff/14001/android_webview/tools/thir...
android_webview/tools/third_party_files_whitelist.txt:11: #
http://www.chromium.org/developers/adding-3rd-party-libraries.
Will this list be used on a bot? Will moving a 3rd-party library without
updating this file turn some bot red? Who's on the hook for keeping it up to
date?

[Steve Block, 2012-08-09 12:53:32]

> Will this list be used on a bot?
Yes, we plan to the run Android license tool on a bot.

> Will moving a 3rd-party library without
> updating this file turn some bot red?
No, this whitelist tracks only those files which contain third-party code but
which aren't in a third-party directory. So these aren't complete modules, but
small imported snippets. It's true that moving one of these file will require
this whitelist to be updated, but
http://www.chromium.org/developers/adding-3rd-party-libraries requires all new
third-party code to be in a directory named third-party, so this list shouldn't
grow.

> Who's on the hook for keeping it up to date?
Initially it will be the Chromium-Android team.

[Nico, 2012-08-09 15:02:29]

Ok, looks fine. I'm assuming this will only run on bots.

The grep command might take a while. Maybe you'll need to have something that
prints some progress marker so that buildbot doesn't let a build time out while
that command is running. (But no need to do that yet, maybe it's not an issue
after all.)

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:36: and which do not use the standard
Chromium license, are listed.
s/are listed/are whitelisted/?

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:50: regex = '[Cc]opyright(?!( \(c\))?
20[0-9][0-9](-20[01][0189])? ' \
Why not 20[0-9][0-9] for the second year in a range too? Since we changed
policies, new files shouldn't have ranges, but we've changes this policy often
enough that I wouldn't be surprised if we change it again.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:61: '.']
Do you set / check the cwd anywhere?

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:65: # Exclude files under listed
directories and some known offendors.
"offenders" I think

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:82: and not
x.startswith('chrome/app/resources/')):
nit: Could you instead say

  directory_list = directory_list + ['android_webview/tools/', ...]

at the top of the function / above the loop? That's a bit more table-driven and
a bit easier to understand.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:83: offending_files += [x]
.append

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:85: result = True
s/result/all_files_valid/

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:97: ' remove the following files from
the whitelist list.\n%s' % \
s/whitelist list/whitelist/

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:113: lines = f.read()
fyi: if you write

  return open(...path...).read()

python will clean up the file at the end of the statement too. Maybe a bit
shorter.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:126: result = True
s/result/all_licenses_valid/

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:140: if match and not
len(match.group(1)) == 0:
if you say + instead of *, you don't need the second part of the conditional

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:141: whitelisted_files +=
[match.group(1)]
append

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:145: def _FindThirdPartyDirs():
Move above _Scan

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:196: return result
Do you need this? If so, adjust coding style (it kinda looks like third-party
code, maybe it should be in a third_party subdirectory with a LICENSE file :-)
). But we can probably just live without it.

[Steve Block, 2012-08-09 17:24:59]

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:36: and which do not use the standard
Chromium license, are listed.
On 2012/08/09 15:02:29, Nico wrote:
> s/are listed/are whitelisted/?

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:50: regex = '[Cc]opyright(?!( \(c\))?
20[0-9][0-9](-20[01][0189])? ' \
On 2012/08/09 15:02:29, Nico wrote:
> Why not 20[0-9][0-9] for the second year in a range too? Since we changed
> policies, new files shouldn't have ranges, but we've changes this policy often
> enough that I wouldn't be surprised if we change it again.

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:61: '.']
I set the cwd on the line below. But it turns out that the existing license tool
assumes it's run from the repository root, so I've added a check for that too.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:65: # Exclude files under listed
directories and some known offendors.
On 2012/08/09 15:02:29, Nico wrote:
> "offenders" I think

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:82: and not
x.startswith('chrome/app/resources/')):
On 2012/08/09 15:02:29, Nico wrote:
> nit: Could you instead say
> 
>   directory_list = directory_list + ['android_webview/tools/', ...]
> 
> at the top of the function / above the loop? That's a bit more table-driven
and
> a bit easier to understand.

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:83: offending_files += [x]
On 2012/08/09 15:02:29, Nico wrote:
> .append

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:85: result = True
On 2012/08/09 15:02:29, Nico wrote:
> s/result/all_files_valid/

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:97: ' remove the following files from
the whitelist list.\n%s' % \
On 2012/08/09 15:02:29, Nico wrote:
> s/whitelist list/whitelist/

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:113: lines = f.read()
On 2012/08/09 15:02:29, Nico wrote:
> fyi: if you write
> 
>   return open(...path...).read()
> 
> python will clean up the file at the end of the statement too. Maybe a bit
> shorter.

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:126: result = True
On 2012/08/09 15:02:29, Nico wrote:
> s/result/all_licenses_valid/

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:140: if match and not
len(match.group(1)) == 0:
On 2012/08/09 15:02:29, Nico wrote:
> if you say + instead of *, you don't need the second part of the conditional

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:141: whitelisted_files +=
[match.group(1)]
On 2012/08/09 15:02:29, Nico wrote:
> append

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:145: def _FindThirdPartyDirs():
On 2012/08/09 15:02:29, Nico wrote:
> Move above _Scan

Done.

http://codereview.chromium.org/10816041/diff/21001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:196: return result
I found the snippet on a forum. I meant to clean it up - now done.

[Nico, 2012-08-09 17:29:04]

lgtm

http://codereview.chromium.org/10816041/diff/15005/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/15005/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:65: # Ignore these tools.
nit: If you do append() instead of `list = list + [...]`, then you're modifying
the input list object, which is generally not desirable. If you want to keep the
append() style, you probably want to add `directory_list = directory_list[:]`
above this.

[Steve Block, 2012-08-09 20:03:19]

http://codereview.chromium.org/10816041/diff/15005/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

http://codereview.chromium.org/10816041/diff/15005/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:65: # Ignore these tools.
On 2012/08/09 17:29:04, Nico wrote:
> nit: If you do append() instead of `list = list + [...]`, then you're
modifying
> the input list object, which is generally not desirable. If you want to keep
the
> append() style, you probably want to add `directory_list = directory_list[:]`
> above this.

Done.

[commit-bot: I haz the power, 2012-08-09 20:05:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/10816041/26001

[commit-bot: I haz the power, 2012-08-09 21:46:18]

Change committed as 150904