Collect license version as an optional_key in licenses.py

Collect license version as an optional_key in licenses.py

This changes licenses.py to collect the license version information
during ParseDir, as an optional field.  This is for scripts that
import this script as a module, that may need this information.

Additionally, this removes the "Required Text" support from
licenses.py, because it was only used by SwiftShader, which no
longer needs this functionality.

BUG=424424
Committed: https://crrev.com/95468c6efabd1ec13b9926c762f2021400a14192
Cr-Commit-Position: refs/heads/master@{#302213}

[mckev, 2014-10-17 00:28:02]

mckev@amazon.com changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-10-24 19:34:27]

Can't you just patch your local version of licenses.py for use in your project?

[mckev, 2014-10-24 23:04:59]

On 2014/10/24 19:34:27, Lei Zhang wrote:
> Can't you just patch your local version of licenses.py for use in your
project?

Hi Lei!

Thanks for the review.  Currently we don't have a local version of this script,
we just import it as a module (as suggested in the header). We call ParseDir
from our reporting logic, much like GenerateCredits.  We could include an
altered version of ParseDir in our script, however it would be less than ideal,
since it does contain a lot of logic that seems to make sense to reuse and stay
up to date with.

Please feel free to let me know if you have another alternative in mind, or a
different way to factor in our customization, I'd be happy to give it a shot.

Thanks!
Kevin

[Lei Zhang, 2014-10-25 01:15:35]

Maybe add an optional "optional_keys" parameter to ParseDir() ?

i.e.

def ParseDir(path, root, require_license_file=True, optional_keys=[]):
  ...
  if not optional_keys:
    optional_keys = ["Required Text", "License Android Compatible"]


BTW, all your emails have been going to my spam folder for some reason. :(
Fortunately I've been able to spot it while emptying the spam folder.

[Lei Zhang, 2014-10-25 01:17:23]

On 2014/10/25 01:15:35, Lei Zhang wrote:
> Maybe add an optional "optional_keys" parameter to ParseDir() ?
> 
> i.e.
> 
> def ParseDir(path, root, require_license_file=True, optional_keys=[]):
>   ...
>   if not optional_keys:
>     optional_keys = ["Required Text", "License Android Compatible"]
> 
> 
> BTW, all your emails have been going to my spam folder for some reason. :(
> Fortunately I've been able to spot it while emptying the spam folder.

Actually, optional_keys=None, not =[], per
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Defau...

[mckev, 2014-10-28 00:49:03]

On 2014/10/25 01:17:23, Lei Zhang wrote:
> On 2014/10/25 01:15:35, Lei Zhang wrote:
> > Maybe add an optional "optional_keys" parameter to ParseDir() ?
> > 
> > i.e.
> > 
> > def ParseDir(path, root, require_license_file=True, optional_keys=[]):
> >   ...
> >   if not optional_keys:
> >     optional_keys = ["Required Text", "License Android Compatible"]
> > 
> > 
> > BTW, all your emails have been going to my spam folder for some reason. :(
> > Fortunately I've been able to spot it while emptying the spam folder.
> 
> Actually, optional_keys=None, not =[], per
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Defau...

> On 2014/10/25 01:15:35, Lei Zhang wrote:
> > Maybe add an optional "optional_keys" parameter to ParseDir() ?
> > 
> > i.e.
> > 
> > def ParseDir(path, root, require_license_file=True, optional_keys=[]):
> >   ...
> >   if not optional_keys:
> >     optional_keys = ["Required Text", "License Android Compatible"]
> > 
> > 
> > BTW, all your emails have been going to my spam folder for some reason. :(
> > Fortunately I've been able to spot it while emptying the spam folder.
> 
> Actually, optional_keys=None, not =[], per
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Defau...

Thanks for the suggestion; I think this is a good alternative.  In fact, we
might not need a default value at all.  I did more research into the two
existing values.  Like this patch, "License Android Compatible" is actually only
used by webview_licenses.py, and probably doesn't need to be here (see
http://crrev.com/756b04e1e06ba31f280515049358d20720fa2ca9e).  Likewise,
"Required Text" is an implementation detail of GenerateCredits, so I should be
able to extract that out of ParseDir as well.  I'll attempt a little
refactoring, and have a new patchset soon.

(Also, thanks for the heads up on the email issue.  I've opened an issue both
internally and with Rietveld to see if I can get that resolved separately.  If
you have any other ideas on how to resolve this or who to contact, feel free to
contact me offline with suggestions.)

[mckev, 2014-10-30 01:35:03]

mckev@amazon.com changed reviewers:
+ mkosiba@chromium.org

[mckev, 2014-10-30 01:35:04]

+mkosiba for android_webview version of licenses.py

PTAL, thanks!

[mkosiba (inactive), 2014-10-30 08:54:43]

lgtm

[Lei Zhang, 2014-10-30 09:14:12]

+jbauman FYI for "required text" key.

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:432: # 'Required Text' is a relative path to a file containing
some html
Fun fact, nobody uses this anymore. It was added solely for swiftshader, which
no longer needs it.

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:439: # TODO(phajdan.jr): Convert to fatal error
(http://crbug.com/39240).
BTW, 79 chars / line in general with Python. See PEP-8.

[mckev, 2014-10-30 16:45:13]

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:432: # 'Required Text' is a relative path to a file containing
some html
On 2014/10/30 09:14:11, Lei Zhang wrote:
> Fun fact, nobody uses this anymore. It was added solely for swiftshader, which
> no longer needs it.

Interesting.  Would you advocate removing it now, or doing so in a separate CL?

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:439: # TODO(phajdan.jr): Convert to fatal error
(http://crbug.com/39240).
On 2014/10/30 09:14:11, Lei Zhang wrote:
> BTW, 79 chars / line in general with Python. See PEP-8.

Whoops, sorry about that.  I'll fix it right away.  Weird that presubmit didn't
catch this one.

[Lei Zhang, 2014-10-30 19:06:13]

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:432: # 'Required Text' is a relative path to a file containing
some html
On 2014/10/30 16:45:13, mckev wrote:
> On 2014/10/30 09:14:11, Lei Zhang wrote:
> > Fun fact, nobody uses this anymore. It was added solely for swiftshader,
which
> > no longer needs it.
> 
> Interesting.  Would you advocate removing it now, or doing so in a separate
CL?

I'd just remove it and mention it in the CL description.

[jbauman, 2014-10-30 20:22:28]

On 2014/10/30 19:06:13, Lei Zhang wrote:
> https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py
> File tools/licenses.py (right):
> 
>
https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
> tools/licenses.py:432: # 'Required Text' is a relative path to a file
containing
> some html
> On 2014/10/30 16:45:13, mckev wrote:
> > On 2014/10/30 09:14:11, Lei Zhang wrote:
> > > Fun fact, nobody uses this anymore. It was added solely for swiftshader,
> which
> > > no longer needs it.
> > 
> > Interesting.  Would you advocate removing it now, or doing so in a separate
> CL?
> 
> I'd just remove it and mention it in the CL description.

Yeah, might as well just remove it now.

[mckev, 2014-10-30 20:45:54]

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/660163002/diff/20001/tools/licenses.py#newcod...
tools/licenses.py:432: # 'Required Text' is a relative path to a file containing
some html
On 2014/10/30 19:06:13, Lei Zhang wrote:
> On 2014/10/30 16:45:13, mckev wrote:
> > On 2014/10/30 09:14:11, Lei Zhang wrote:
> > > Fun fact, nobody uses this anymore. It was added solely for swiftshader,
> which
> > > no longer needs it.
> > 
> > Interesting.  Would you advocate removing it now, or doing so in a separate
> CL?
> 
> I'd just remove it and mention it in the CL description.

Sounds good to me!

[Lei Zhang, 2014-10-31 01:19:32]

lgtm, thanks

[mckev, 2014-10-31 01:53:56]

The CQ bit was checked by mckev@amazon.com

[commit-bot: I haz the power, 2014-10-31 01:54:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660163002/60001

[mckev, 2014-10-31 02:01:23]

On 2014/10/31 01:54:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/660163002/60001

Thanks for your help on this review, Lei!

[commit-bot: I haz the power, 2014-10-31 02:30:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 02:31:04]

Patchset 4 (id:??) landed as
https://crrev.com/95468c6efabd1ec13b9926c762f2021400a14192
Cr-Commit-Position: refs/heads/master@{#302213}