[Android WebView] Make 3rd party licenses checker to turn bot red

[Android WebView] Make 3rd party licenses checker to turn bot red

We have substantially improved the licenses checker to avoid
false positives in finding copyrighted code in non-third_party dirs.

Also, this change makes the presence of stale entries in the
whitelist file to be a warning, not an error.

BUG=161461
NOTRY=true

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

[mnaganov (inactive), 2013-02-06 11:37:35]

Marcin, please take a look.

bulach@ - please look at the buildbot changes.

[mkosiba (inactive), 2013-02-06 15:40:48]

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:79: SCAN_ERRORS = 1 << 1
It seems these are exclusive, not flags, so how about:

  class ScanResult: # or ScanStatus
      Ok, Warnings, Errors = range(3) # or [0] + [1 << i for i in range(2)]

Based on
http://stackoverflow.com/questions/702834/whats-the-common-practice-for-enums...

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:88: SCAN_OK if all files with
non-standard license headers are whitelisted and
if you go with the suggestion above you could say Returns: ScanResult and move
the documentation to the ScanResult class (and then you wouldn't need to repeat
all of the status code descriptions in every function)

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:150: return SCAN_ERRORS if unknown
else (SCAN_WARNINGS if stale else SCAN_OK)
I think it would be slightly clearer to make this an old-fashioned if/elif:

if unknown:
    return SCAN_ERRORS
elif stale:
    return SCAN_WARNINGS
else:
    return SCAN_OK

[mnaganov (inactive), 2013-02-06 16:42:16]

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:79: SCAN_ERRORS = 1 << 1
On 2013/02/06 15:40:49, Martin Kosiba wrote:
> It seems these are exclusive, not flags, so how about:
> 
>   class ScanResult: # or ScanStatus
>       Ok, Warnings, Errors = range(3) # or [0] + [1 << i for i in range(2)]
> 
> Based on
>
http://stackoverflow.com/questions/702834/whats-the-common-practice-for-enums...

Fixed, thanks!

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:88: SCAN_OK if all files with
non-standard license headers are whitelisted and
On 2013/02/06 15:40:49, Martin Kosiba wrote:
> if you go with the suggestion above you could say Returns: ScanResult and move
> the documentation to the ScanResult class (and then you wouldn't need to
repeat
> all of the status code descriptions in every function)

The interpretation of results are method-dependent. _CheckLicenseHeaders only
deals with non-third party code copyrights status, while _Scan also deals with
third-party code licensing metadata. I think it's worth to have explicit
descriptions.

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:150: return SCAN_ERRORS if unknown
else (SCAN_WARNINGS if stale else SCAN_OK)
On 2013/02/06 15:40:49, Martin Kosiba wrote:
> I think it would be slightly clearer to make this an old-fashioned if/elif:
> 
> if unknown:
>     return SCAN_ERRORS
> elif stale:
>     return SCAN_WARNINGS
> else:
>     return SCAN_OK

Done.

[mkosiba (inactive), 2013-02-06 16:54:51]

lgtm

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

https://codereview.chromium.org/12209034/diff/2001/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:88: SCAN_OK if all files with
non-standard license headers are whitelisted and
On 2013/02/06 16:42:17, Mikhail Naganov (Chromium) wrote:
> On 2013/02/06 15:40:49, Martin Kosiba wrote:
> > if you go with the suggestion above you could say Returns: ScanResult and
move
> > the documentation to the ScanResult class (and then you wouldn't need to
> repeat
> > all of the status code descriptions in every function)
> 
> The interpretation of results are method-dependent. _CheckLicenseHeaders only
> deals with non-third party code copyrights status, while _Scan also deals with
> third-party code licensing metadata. I think it's worth to have explicit
> descriptions.

ah, ok then.

[mnaganov (inactive), 2013-02-07 11:52:12]

Marcus, ping!

[mnaganov (inactive), 2013-02-07 11:52:12]

Marcus, ping!

[bulach, 2013-02-07 11:57:14]

lgtm, tiny nit below. sorry about the delay! :)

https://codereview.chromium.org/12209034/diff/1004/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

https://codereview.chromium.org/12209034/diff/1004/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:77: class ScanResult:
nit: derive from object,
class ScanResult(object):

[Isaac (away), 2013-02-07 12:08:47]

https://codereview.chromium.org/12209034/diff/1004/build/android/buildbot/bui...
File build/android/buildbot/buildbot_functions.sh (right):

https://codereview.chromium.org/12209034/diff/1004/build/android/buildbot/bui...
build/android/buildbot/buildbot_functions.sh:277: return $licenses_exit_code
This syntax will not work - the bot while halt if this step goes red instead of
running the other tests.  You could use bb_run_step instead.

However unless you have a plan to put this on the main waterfall, it should not
turn the fyi bots red.  And the last time we discussed this I thought it was
going to be converted to a presubmit check?

[mnaganov (inactive), 2013-02-07 13:48:51]

https://codereview.chromium.org/12209034/diff/1004/android_webview/tools/webv...
File android_webview/tools/webview_licenses.py (right):

https://codereview.chromium.org/12209034/diff/1004/android_webview/tools/webv...
android_webview/tools/webview_licenses.py:77: class ScanResult:
On 2013/02/07 11:57:15, bulach wrote:
> nit: derive from object,
> class ScanResult(object):

Fixed, thanks!

https://codereview.chromium.org/12209034/diff/1004/build/android/buildbot/bui...
File build/android/buildbot/buildbot_functions.sh (right):

https://codereview.chromium.org/12209034/diff/1004/build/android/buildbot/bui...
build/android/buildbot/buildbot_functions.sh:277: return $licenses_exit_code
On 2013/02/07 12:08:47, Isaac wrote:
> This syntax will not work - the bot while halt if this step goes red instead
of
> running the other tests.  You could use bb_run_step instead.
>

Thanks for pointing this out, Isaac! I think I can't use bb_run_step directly
because it can only report a step failure, not a warning condition. But I
changed the code to avoid halting the build.
 
> However unless you have a plan to put this on the main waterfall, it should
not
> turn the fyi bots red.  And the last time we discussed this I thought it was
> going to be converted to a presubmit check?

I remember about that goal, this change is a step towards it. We have improved
the copyright scanner to have substantially less false positives, so it should
only become red on real issues, not when somebody uses 'copyright' as a variable
name or in a string.

The plan is to evaluate the new version on our FYI bot first, then promote it
into a presubmit check.

[Isaac (away), 2013-02-07 16:48:55]

The other thing discussed on the bug is extending the curret license check
script rather than running this on android bot.  People do not always send
tryjobs to android - I don't think this check is well suited for android bot /
tryjobs.  Given that; it should not turn the android fyi bot red.

[mnaganov (inactive), 2013-02-07 17:03:33]

On 2013/02/07 16:48:55, Isaac wrote:
> The other thing discussed on the bug is extending the curret license check
> script rather than running this on android bot.  People do not always send
> tryjobs to android - I don't think this check is well suited for android bot /
> tryjobs.  Given that; it should not turn the android fyi bot red.

Correct. I have tried fitting licensecheck.pl to the needs of finding 3rd-party
code outside of third_party dirs, but the changes we have to make were too
project-specific, while phajdan@ strives to only make upstreamable changes
(remember, this is a script from Debian). So we have ended up having our own
specialized script for finding 3rd-party code.

We will reuse licensecheck.pl and checklicenses.py for verifying that the
third-party code we use is under acceptable licenses (since Android's list is
narrower than Chromium's), but this isn't done yet, it's a separate task.

I intend to keep this change on Android FYI bots only for a month. If it will
not bring any hassle to our sheriffs during the trial period, it will be safe to
roll it Chromium-wide. Ideally, if no one will check in copyrighted code into
non-third_party dir, the bot will never go red due to the license checker. While
if that will happen, it's actually a bad thing for the project, and it will be
great if we will catch it early. Currently no one takes care of the license
checking step just because it doesn't affect the bot state and because it is
known to cause troubles in the past.

[joth, 2013-02-07 18:53:49]

On 7 February 2013 09:03, <mnaganov@chromium.org> wrote:

> On 2013/02/07 16:48:55, Isaac wrote:
>
>> The other thing discussed on the bug is extending the curret license check
>> script rather than running this on android bot.  People do not always send
>> tryjobs to android - I don't think this check is well suited for android
>> bot /
>> tryjobs.  Given that; it should not turn the android fyi bot red.
>>
>
> Correct. I have tried fitting licensecheck.pl to the needs of finding
> 3rd-party
> code outside of third_party dirs, but the changes we have to make were too
> project-specific, while phajdan@ strives to only make upstreamable changes
> (remember, this is a script from Debian). So we have ended up having our
> own
> specialized script for finding 3rd-party code.
>
>
CL, for reference: https://codereview.chromium.org/12047113/


> We will reuse licensecheck.pl and checklicenses.py for verifying that the
> third-party code we use is under acceptable licenses (since Android's list
> is
> narrower than Chromium's), but this isn't done yet, it's a separate task.
>
> I intend to keep this change on Android FYI bots only for a month. If it
> will
> not bring any hassle to our sheriffs during the trial period, it will be
> safe to
> roll it Chromium-wide. Ideally, if no one will check in copyrighted code
> into
> non-third_party dir, the bot will never go red due to the license checker.
> While
> if that will happen, it's actually a bad thing for the project, and it
> will be
> great if we will catch it early. Currently no one takes care of the license
> checking step just because it doesn't affect the bot state and because it
> is
> known to cause troubles in the past.
>
>
https://codereview.chromium.**org/12209034/<https://codereview.chromium.org/1...
>

[mnaganov (inactive), 2013-02-08 10:07:33]

So, Isaac, what do you think about the plan?

The check anyway currently runs on Android Builder. The only difference that
this change introduces is to attract sheriff's attention on real
licensing-related issues. It's a preliminary step before rolling it out
Chromium-wide.

[Isaac (away), 2013-02-09 22:46:09]

After looking at the background to this change, I'm on board.

The last time we turned this on on the FYI bots, it generated a lot of extra
work for the android sheriffs.  To avoid that, let's also move this step to CQ &
main waterfall at the same time (add the step to main-builder-dbg in
bb_run_bot.py, goo.gl/5knJ4).  It may cause the android bot to go red more
frequently, but the chromium sheriffs will be able to assist.

I still like the idea of a single license check, but after looking at the
licensecheck.pl code review, I'm not sure the best way forward.  Perhaps after
we add a presubmit check, it will not matter that we run two license check
scripts, because only people who bypass hooks will get caught.

[mnaganov (inactive), 2013-02-11 10:18:46]

On 2013/02/09 22:46:09, Isaac wrote:
> After looking at the background to this change, I'm on board.
> 
> The last time we turned this on on the FYI bots, it generated a lot of extra
> work for the android sheriffs.  To avoid that, let's also move this step to CQ
&
> main waterfall at the same time (add the step to main-builder-dbg in
> bb_run_bot.py, goo.gl/5knJ4).  It may cause the android bot to go red more
> frequently, but the chromium sheriffs will be able to assist.
> 

Thanks. Please look at the change--did I get your idea right?

> I still like the idea of a single license check, but after looking at the
> licensecheck.pl code review, I'm not sure the best way forward.  Perhaps after
> we add a presubmit check, it will not matter that we run two license check
> scripts, because only people who bypass hooks will get caught.

Just to recap. The check currently performed by the Linux bot (check_licenses)
scans the first 100 lines of every code source file (that includes third_party
dirs) and checks, if they have a Chromium-compatible license. This is
good for us, except that Android's list of accepted licenses is narrower
than Chromium's.

The copyright scanning we are doing is not done by Chromium bots (as you can
see, the corresponding code is even removed from the Chromium version of
licensecheck.pl), but it is also valuable for the project, as no one at
Chromium wants to have copyrighted code with a wrong license to creep into
the codebase.

So these two check are not overlapping much in fact.

[Isaac (away), 2013-02-11 10:56:42]

On 2013/02/11 10:18:46, Mikhail Naganov (Chromium) wrote:
> The copyright scanning we are doing is not done by Chromium bots (as you can
> see, the corresponding code is even removed from the Chromium version of
> licensecheck.pl), but it is also valuable for the project, as no one at
> Chromium wants to have copyrighted code with a wrong license to creep into
> the codebase.
> 
> So these two check are not overlapping much in fact.

Yes, but it's confusing to have license checks on two different bots.  So we
should try to combine them.

We should keep this step on the fyi bots for now.  That's because I'm trying to
have the fyi bots remain a superset of the tests on main waterfall.  It's a
little redundant but it lets people only send jobs to android_fyi_dbg. 
Otherwise, the buildbot changes lgtm (w/ a green android_dbg tryjob).  I'm
expecting this to add 60-90sec per tryjob run, so we should consider this
temporary.  There are plans to add a presubmit-ish trybot soon -- this would be
a good candidate to move there.

[Isaac (away), 2013-02-11 10:57:43]

By 'keep this step on fyi' I mean in addition to adding to main waterfall
config.

[commit-bot: I haz the power, 2013-02-11 11:52:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/12209034/1007

[commit-bot: I haz the power, 2013-02-11 12:10:33]

Change committed as 181679