[PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Alexander Kanavin]

Specifically, 'devtool upgrade' will now do these things:

1) determine if any of the license checksums need updating; if so,
write the new checksums into the LIC_FILES_CHKSUM value in the recipe
that is written to the workspace;

2) print a notice to the standard output:

NOTE: New recipe is /home/ak/development/poky/build/workspace/recipes/puzzles/puzzles_git.bb
NOTE: License checksums have been updated in the new recipe; please refer to it for the difference between the old and the new license texts.

3) and the cool part: devtool will create a diff of the old and new licenses,
and write the diff into the workspace recipe as a comment, like this:

======
 FIXME: the LIC_FILES_CHKSUM values have been updated by 'devtool upgrade'.
 The following is the difference between the old and the new license text.
 Please update the LICENSE value if needed, and summarize the changes in
 the commit message via 'License-checksum-change:' tag.
 (example: 'License-checksum-change: copyright years updated.')

 The changes:

 --- LICENCE
 +++ LICENCE
 @@ -1,8 +1,9 @@
  This software is copyright (c) 2004-2014 Simon Tatham.

  Portions copyright Richard Boulton, James Harvey, Mike Pinna, Jonas
 -Klker, Dariusz Olszewski, Michael Schierl, Lambros Lambrou, Bernd
 -Schmidt, Steffen Bauer, Lennard Sprong and Rogier Goossens.
 +Kölker, Dariusz Olszewski, Michael Schierl, Lambros Lambrou, Bernd
 +Schmidt, Steffen Bauer, Lennard Sprong, Rogier Goossens and Michael
 +Quevillon.

  Permission is hereby granted, free of charge, to any person
  obtaining a copy of this software and associated documentation files

======

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 scripts/lib/devtool/upgrade.py | 75 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/scripts/lib/devtool/upgrade.py b/scripts/lib/devtool/upgrade.py
index 28fbdaee357..e153d0c2edf 100644
--- a/scripts/lib/devtool/upgrade.py
+++ b/scripts/lib/devtool/upgrade.py
@@ -291,7 +291,26 @@ def _extract_new_source(newpv, srctree, no_patch, srcrev, srcbranch, branch, kee
 
     return (rev, md5, sha256, srcbranch, srcsubdir_rel)
 
-def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, srcsubdir_new, workspace, tinfoil, rd):
+def _add_license_diff_to_recipe(path, diff):
+    notice_text = """# FIXME: the LIC_FILES_CHKSUM values have been updated by 'devtool upgrade'.
+# The following is the difference between the old and the new license text.
+# Please update the LICENSE value if needed, and summarize the changes in
+# the commit message via 'License-checksum-change:' tag.
+# (example: 'License-checksum-change: copyright years updated.')
+#
+# The changes:
+#
+"""
+    commented_diff = "\n".join(["# {}".format(l) for l in diff.split('\n')])
+    with open(path, 'rb') as f:
+        orig_content = f.read()
+    with open(path, 'wb') as f:
+        f.write(notice_text.encode())
+        f.write(commented_diff.encode())
+        f.write("\n#\n".encode())
+        f.write(orig_content)
+
+def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, srcsubdir_new, workspace, tinfoil, rd, license_diff, new_licenses):
     """Creates the new recipe under workspace"""
 
     bpn = rd.getVar('BPN')
@@ -400,6 +419,11 @@ def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, src
             else:
                 logger.info('Source subdirectory has changed, updating S value')
 
+    if license_diff:
+        newlicchksum = " ".join(["file://{};md5={}".format(l["path"], l["actual_md5"]) + (";beginline={}".format(l["beginline"]) if l["beginline"] else "") + (";endline={}".format(l["endline"]) if l["endline"] else "") for l in new_licenses])
+        newvalues["LIC_FILES_CHKSUM"] = newlicchksum
+        _add_license_diff_to_recipe(fullpath, license_diff)
+
     rd = tinfoil.parse_recipe_file(fullpath, False)
     oe.recipeutils.patch_recipe(rd, fullpath, newvalues)
 
@@ -427,6 +451,47 @@ def _check_git_config():
     if configerr:
         raise DevtoolError('Your git configuration is incomplete which will prevent rebases from working:\n' + '\n'.join(configerr))
 
+def _extract_licenses(srcpath, recipe_licenses):
+    licenses = []
+    for url in recipe_licenses.split():
+        license = {}
+        (type, host, path, user, pswd, parm) = bb.fetch.decodeurl(url)
+        license['path'] = path
+        license['md5'] = parm.get('md5', '')
+        license['beginline'], license['endline'] = 0, 0
+        if 'beginline' in parm:
+            license['beginline'] = int(parm['beginline'])
+        if 'endline' in parm:
+            license['endline'] = int(parm['endline'])
+        license['text'] = []
+        with open(os.path.join(srcpath, path), 'rb') as f:
+            import hashlib
+            actual_md5 = hashlib.md5()
+            lineno = 0
+            for line in f:
+                lineno += 1
+                if (lineno >= license['beginline']) and ((lineno <= license['endline']) or not license['endline']):
+                    license['text'].append(line.decode(errors='ignore'))
+                    actual_md5.update(line)
+        license['actual_md5'] = actual_md5.hexdigest()
+        licenses.append(license)
+    return licenses
+
+def _generate_license_diff(old_licenses, new_licenses):
+    need_diff = False
+    for l in new_licenses:
+        if l['md5'] != l['actual_md5']:
+            need_diff = True
+            break
+    if need_diff == False:
+        return None
+
+    import difflib
+    diff = ''
+    for old, new in zip(old_licenses, new_licenses):
+        for line in difflib.unified_diff(old['text'], new['text'], old['path'], new['path']):
+            diff = diff + line
+    return diff
 
 def upgrade(args, config, basepath, workspace):
     """Entry point for the devtool 'upgrade' subcommand"""
@@ -480,14 +545,18 @@ def upgrade(args, config, basepath, workspace):
             check_prerelease_version(args.version, 'devtool upgrade')
 
         rf = None
+        license_diff = None
         try:
             logger.info('Extracting current version source...')
             rev1, srcsubdir1 = standard._extract_source(srctree, False, 'devtool-orig', False, config, basepath, workspace, args.fixed_setup, rd, tinfoil, no_overrides=args.no_overrides)
+            old_licenses = _extract_licenses(srctree, rd.getVar('LIC_FILES_CHKSUM'))
             logger.info('Extracting upgraded version source...')
             rev2, md5, sha256, srcbranch, srcsubdir2 = _extract_new_source(args.version, srctree, args.no_patch,
                                                     args.srcrev, args.srcbranch, args.branch, args.keep_temp,
                                                     tinfoil, rd)
-            rf, copied = _create_new_recipe(args.version, md5, sha256, args.srcrev, srcbranch, srcsubdir1, srcsubdir2, config.workspace_path, tinfoil, rd)
+            new_licenses = _extract_licenses(srctree, rd.getVar('LIC_FILES_CHKSUM'))
+            license_diff = _generate_license_diff(old_licenses, new_licenses)
+            rf, copied = _create_new_recipe(args.version, md5, sha256, args.srcrev, srcbranch, srcsubdir1, srcsubdir2, config.workspace_path, tinfoil, rd, license_diff, new_licenses)
         except bb.process.CmdError as e:
             _upgrade_error(e, rf, srctree)
         except DevtoolError as e:
@@ -502,6 +571,8 @@ def upgrade(args, config, basepath, workspace):
 
         logger.info('Upgraded source extracted to %s' % srctree)
         logger.info('New recipe is %s' % rf)
+        if license_diff:
+            logger.info('License checksums have been updated in the new recipe; please refer to it for the difference between the old and the new license texts.')
     finally:
         tinfoil.shutdown()
     return 0
-- 
2.15.0

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Khem Raj]

On 11/27/17 6:54 AM, Alexander Kanavin wrote:
> Specifically, 'devtool upgrade' will now do these things:
> 
> 1) determine if any of the license checksums need updating; if so,
> write the new checksums into the LIC_FILES_CHKSUM value in the recipe
> that is written to the workspace;
> 
> 2) print a notice to the standard output:
> 
> NOTE: New recipe is /home/ak/development/poky/build/workspace/recipes/puzzles/puzzles_git.bb
> NOTE: License checksums have been updated in the new recipe; please refer to it for the difference between the old and the new license texts.
> 
> 3) and the cool part: devtool will create a diff of the old and new licenses,
> and write the diff into the workspace recipe as a comment, like this:
> 

adding a comment in recipe itself might start folks sending patches with 
this text in the recipe changes. It should just be in commit message.

> ======
>   FIXME: the LIC_FILES_CHKSUM values have been updated by 'devtool upgrade'.
>   The following is the difference between the old and the new license text.
>   Please update the LICENSE value if needed, and summarize the changes in
>   the commit message via 'License-checksum-change:' tag.
>   (example: 'License-checksum-change: copyright years updated.')
> 
>   The changes:
> 
>   --- LICENCE
>   +++ LICENCE
>   @@ -1,8 +1,9 @@
>    This software is copyright (c) 2004-2014 Simon Tatham.
> 
>    Portions copyright Richard Boulton, James Harvey, Mike Pinna, Jonas
>   -Klker, Dariusz Olszewski, Michael Schierl, Lambros Lambrou, Bernd
>   -Schmidt, Steffen Bauer, Lennard Sprong and Rogier Goossens.
>   +Kölker, Dariusz Olszewski, Michael Schierl, Lambros Lambrou, Bernd
>   +Schmidt, Steffen Bauer, Lennard Sprong, Rogier Goossens and Michael
>   +Quevillon.
> 
>    Permission is hereby granted, free of charge, to any person
>    obtaining a copy of this software and associated documentation files
> 
> ======
> 
> Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
> ---
>   scripts/lib/devtool/upgrade.py | 75 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/lib/devtool/upgrade.py b/scripts/lib/devtool/upgrade.py
> index 28fbdaee357..e153d0c2edf 100644
> --- a/scripts/lib/devtool/upgrade.py
> +++ b/scripts/lib/devtool/upgrade.py
> @@ -291,7 +291,26 @@ def _extract_new_source(newpv, srctree, no_patch, srcrev, srcbranch, branch, kee
>   
>       return (rev, md5, sha256, srcbranch, srcsubdir_rel)
>   
> -def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, srcsubdir_new, workspace, tinfoil, rd):
> +def _add_license_diff_to_recipe(path, diff):
> +    notice_text = """# FIXME: the LIC_FILES_CHKSUM values have been updated by 'devtool upgrade'.
> +# The following is the difference between the old and the new license text.
> +# Please update the LICENSE value if needed, and summarize the changes in
> +# the commit message via 'License-checksum-change:' tag.
> +# (example: 'License-checksum-change: copyright years updated.')
> +#
> +# The changes:
> +#
> +"""
> +    commented_diff = "\n".join(["# {}".format(l) for l in diff.split('\n')])
> +    with open(path, 'rb') as f:
> +        orig_content = f.read()
> +    with open(path, 'wb') as f:
> +        f.write(notice_text.encode())
> +        f.write(commented_diff.encode())
> +        f.write("\n#\n".encode())
> +        f.write(orig_content)
> +
> +def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, srcsubdir_new, workspace, tinfoil, rd, license_diff, new_licenses):
>       """Creates the new recipe under workspace"""
>   
>       bpn = rd.getVar('BPN')
> @@ -400,6 +419,11 @@ def _create_new_recipe(newpv, md5, sha256, srcrev, srcbranch, srcsubdir_old, src
>               else:
>                   logger.info('Source subdirectory has changed, updating S value')
>   
> +    if license_diff:
> +        newlicchksum = " ".join(["file://{};md5={}".format(l["path"], l["actual_md5"]) + (";beginline={}".format(l["beginline"]) if l["beginline"] else "") + (";endline={}".format(l["endline"]) if l["endline"] else "") for l in new_licenses])
> +        newvalues["LIC_FILES_CHKSUM"] = newlicchksum
> +        _add_license_diff_to_recipe(fullpath, license_diff)
> +
>       rd = tinfoil.parse_recipe_file(fullpath, False)
>       oe.recipeutils.patch_recipe(rd, fullpath, newvalues)
>   
> @@ -427,6 +451,47 @@ def _check_git_config():
>       if configerr:
>           raise DevtoolError('Your git configuration is incomplete which will prevent rebases from working:\n' + '\n'.join(configerr))
>   
> +def _extract_licenses(srcpath, recipe_licenses):
> +    licenses = []
> +    for url in recipe_licenses.split():
> +        license = {}
> +        (type, host, path, user, pswd, parm) = bb.fetch.decodeurl(url)
> +        license['path'] = path
> +        license['md5'] = parm.get('md5', '')
> +        license['beginline'], license['endline'] = 0, 0
> +        if 'beginline' in parm:
> +            license['beginline'] = int(parm['beginline'])
> +        if 'endline' in parm:
> +            license['endline'] = int(parm['endline'])
> +        license['text'] = []
> +        with open(os.path.join(srcpath, path), 'rb') as f:
> +            import hashlib
> +            actual_md5 = hashlib.md5()
> +            lineno = 0
> +            for line in f:
> +                lineno += 1
> +                if (lineno >= license['beginline']) and ((lineno <= license['endline']) or not license['endline']):
> +                    license['text'].append(line.decode(errors='ignore'))
> +                    actual_md5.update(line)
> +        license['actual_md5'] = actual_md5.hexdigest()
> +        licenses.append(license)
> +    return licenses
> +
> +def _generate_license_diff(old_licenses, new_licenses):
> +    need_diff = False
> +    for l in new_licenses:
> +        if l['md5'] != l['actual_md5']:
> +            need_diff = True
> +            break
> +    if need_diff == False:
> +        return None
> +
> +    import difflib
> +    diff = ''
> +    for old, new in zip(old_licenses, new_licenses):
> +        for line in difflib.unified_diff(old['text'], new['text'], old['path'], new['path']):
> +            diff = diff + line
> +    return diff
>   
>   def upgrade(args, config, basepath, workspace):
>       """Entry point for the devtool 'upgrade' subcommand"""
> @@ -480,14 +545,18 @@ def upgrade(args, config, basepath, workspace):
>               check_prerelease_version(args.version, 'devtool upgrade')
>   
>           rf = None
> +        license_diff = None
>           try:
>               logger.info('Extracting current version source...')
>               rev1, srcsubdir1 = standard._extract_source(srctree, False, 'devtool-orig', False, config, basepath, workspace, args.fixed_setup, rd, tinfoil, no_overrides=args.no_overrides)
> +            old_licenses = _extract_licenses(srctree, rd.getVar('LIC_FILES_CHKSUM'))
>               logger.info('Extracting upgraded version source...')
>               rev2, md5, sha256, srcbranch, srcsubdir2 = _extract_new_source(args.version, srctree, args.no_patch,
>                                                       args.srcrev, args.srcbranch, args.branch, args.keep_temp,
>                                                       tinfoil, rd)
> -            rf, copied = _create_new_recipe(args.version, md5, sha256, args.srcrev, srcbranch, srcsubdir1, srcsubdir2, config.workspace_path, tinfoil, rd)
> +            new_licenses = _extract_licenses(srctree, rd.getVar('LIC_FILES_CHKSUM'))
> +            license_diff = _generate_license_diff(old_licenses, new_licenses)
> +            rf, copied = _create_new_recipe(args.version, md5, sha256, args.srcrev, srcbranch, srcsubdir1, srcsubdir2, config.workspace_path, tinfoil, rd, license_diff, new_licenses)
>           except bb.process.CmdError as e:
>               _upgrade_error(e, rf, srctree)
>           except DevtoolError as e:
> @@ -502,6 +571,8 @@ def upgrade(args, config, basepath, workspace):
>   
>           logger.info('Upgraded source extracted to %s' % srctree)
>           logger.info('New recipe is %s' % rf)
> +        if license_diff:
> +            logger.info('License checksums have been updated in the new recipe; please refer to it for the difference between the old and the new license texts.')
>       finally:
>           tinfoil.shutdown()
>       return 0
> 

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Alexander Kanavin]

On 11/27/2017 08:11 PM, Khem Raj wrote:
>> 3) and the cool part: devtool will create a diff of the old and new 
>> licenses,
>> and write the diff into the workspace recipe as a comment, like this:
>>
> 
> adding a comment in recipe itself might start folks sending patches with 
> this text in the recipe changes. It should just be in commit message.

I'm not sure if this was clear, but the auto-generated license diff is 
meant as a starting point for a commit summary written by a human, and 
once the summary is written, should be deleted, regardless of whether it 
was placed in a recipe, commit message or a separate file. Devtool does 
not create or manage commits from the changes it makes to recipes, and 
so placing this into the recipe felt the most appropriate to me 
(separate files are more cumbersome to manage and printing to stdout can 
be forgotten). We can have an automatic patch qa check that it's not 
accidentally left over, similar to the existing check that license 
changes have an explanation.


Alex

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Khem Raj]

On Mon, Nov 27, 2017 at 11:23 AM, Alexander Kanavin
<alexander.kanavin@linux.intel.com> wrote:
> On 11/27/2017 08:11 PM, Khem Raj wrote:
>>>
>>> 3) and the cool part: devtool will create a diff of the old and new
>>> licenses,
>>> and write the diff into the workspace recipe as a comment, like this:
>>>
>>
>> adding a comment in recipe itself might start folks sending patches with
>> this text in the recipe changes. It should just be in commit message.
>
>
> I'm not sure if this was clear, but the auto-generated license diff is meant
> as a starting point for a commit summary written by a human, and once the
> summary is written, should be deleted, regardless of whether it was placed
> in a recipe, commit message or a separate file. Devtool does not create or
> manage commits from the changes it makes to recipes, and so placing this
> into the recipe felt the most appropriate to me (separate files are more
> cumbersome to manage and printing to stdout can be forgotten). We can have
> an automatic patch qa check that it's not accidentally left over, similar to
> the existing check that license changes have an explanation.

I think emitting this info to stdout should be enough.

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Alexander Kanavin]

On 11/29/2017 09:43 AM, Khem Raj wrote:
>> I'm not sure if this was clear, but the auto-generated license diff is meant
>> as a starting point for a commit summary written by a human, and once the
>> summary is written, should be deleted, regardless of whether it was placed
>> in a recipe, commit message or a separate file. Devtool does not create or
>> manage commits from the changes it makes to recipes, and so placing this
>> into the recipe felt the most appropriate to me (separate files are more
>> cumbersome to manage and printing to stdout can be forgotten). We can have
>> an automatic patch qa check that it's not accidentally left over, similar to
>> the existing check that license changes have an explanation.
> 
> I think emitting this info to stdout should be enough.

Between issuing 'devtool upgrade' and issuing 'git commit' with the 
upgrade a lot of things can happen. Patches need to be rebased by hand, 
build failures need to be dealt with, fixes need to be submitted 
upstream. Oh, and the changes need to be tested on target sometimes. 
Between all that, what the original command printed can be easily lost 
or forgottten. Much better to stash it somewhere where it will be safe.

Alex

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

I just tested this new behaviour and I for one like how it worked.  It's
sufficiently verbose that it obviously needs to be deleted and is stashed
somewhere useful.

A new patchtest test to automatically tell people if they left it in the
recipe would be good too.

Ross

On 29 November 2017 at 08:07, Alexander Kanavin <
alexander.kanavin@linux.intel.com> wrote:

> On 11/29/2017 09:43 AM, Khem Raj wrote:
>
>> I'm not sure if this was clear, but the auto-generated license diff is
>>> meant
>>> as a starting point for a commit summary written by a human, and once the
>>> summary is written, should be deleted, regardless of whether it was
>>> placed
>>> in a recipe, commit message or a separate file. Devtool does not create
>>> or
>>> manage commits from the changes it makes to recipes, and so placing this
>>> into the recipe felt the most appropriate to me (separate files are more
>>> cumbersome to manage and printing to stdout can be forgotten). We can
>>> have
>>> an automatic patch qa check that it's not accidentally left over,
>>> similar to
>>> the existing check that license changes have an explanation.
>>>
>>
>> I think emitting this info to stdout should be enough.
>>
>
> Between issuing 'devtool upgrade' and issuing 'git commit' with the
> upgrade a lot of things can happen. Patches need to be rebased by hand,
> build failures need to be dealt with, fixes need to be submitted upstream.
> Oh, and the changes need to be tested on target sometimes. Between all
> that, what the original command printed can be easily lost or forgottten.
> Much better to stash it somewhere where it will be safe.
>
> Alex
>
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>

[-- Attachment #2: Type: text/html, Size: 2745 bytes --]

Re: [PATCH] devtool: add license checksum change handling to 'devtool upgrade'

[Alexander Kanavin]

On 11/29/2017 11:10 AM, Burton, Ross wrote:
> I just tested this new behaviour and I for one like how it worked.  It's 
> sufficiently verbose that it obviously needs to be deleted and is 
> stashed somewhere useful.

FWIW, I will modify AUH so that it will move the info from recipe into 
commit, when creating one. So then a human will only need to amend the 
commit message. The broader goal is to automate the upgrade process as 
much as possible, give the commits directly to autobuilder and post them 
to oe-core list, and if they pass and don't need amending, I think it's 
fine to push them to straight to master with the 'From' still containing 
a robot name :)

So that I (and other recipe maintainers) can spend time on more 
interesting things.

> A new patchtest test to automatically tell people if they left it in the 
> recipe would be good too.

Yes; will be done after I'm happy with the way AUH works :)

Alex