NACK: [Unstable][PATCH] UBUNTU: [Packaging] Rewrite debian/scripts/misc/insert-changes.pl in Python

[Juerg Haefliger]

On Mon, 1 Nov 2021 15:04:17 -0300
Thadeu Lima de Souza Cascardo <cascardo at canonical.com> wrote:

> On Mon, Nov 01, 2021 at 10:40:05AM +0100, Juerg Haefliger wrote:
> > Rewrite the insert-changes.pl script in Python to get us one step closer
> > to dropping Perl as an Ubuntu kernel build dependency.
> > 
> > Signed-off-by: Juerg Haefliger <juergh at canonical.com>
> > ---
> >  debian/rules.d/1-maintainer.mk        |  2 +-
> >  debian/scripts/misc/insert-changes    | 35 ++++++++++++++++++++++
> >  debian/scripts/misc/insert-changes.pl | 43 ---------------------------
> >  3 files changed, 36 insertions(+), 44 deletions(-)
> >  create mode 100755 debian/scripts/misc/insert-changes
> >  delete mode 100755 debian/scripts/misc/insert-changes.pl
> > 
> > diff --git a/debian/rules.d/1-maintainer.mk b/debian/rules.d/1-maintainer.mk
> > index 7c13fcdaa98a..5917b89bbd49 100644
> > --- a/debian/rules.d/1-maintainer.mk
> > +++ b/debian/rules.d/1-maintainer.mk
> > @@ -113,7 +113,7 @@ printchanges:
> >  	$(DROOT)/scripts/misc/git-ubuntu-log $(ubuntu_log_opts)
> >  
> >  insertchanges: autoreconstruct finalchecks
> > -	@perl -w -f $(DROOT)/scripts/misc/insert-changes.pl $(DROOT) $(DEBIAN) 
> > +	$(DROOT)/scripts/misc/insert-changes $(DROOT) $(DEBIAN)
> >  
> >  autoreconstruct:
> >  	# No need for reconstruct for -rc kernels since we don't upload an
> > diff --git a/debian/scripts/misc/insert-changes b/debian/scripts/misc/insert-changes
> > new file mode 100755
> > index 000000000000..284b6788cc13
> > --- /dev/null
> > +++ b/debian/scripts/misc/insert-changes
> > @@ -0,0 +1,35 @@
> > +#!/usr/bin/python3
> > +
> > +import os
> > +import sys
> > +
> > +from subprocess import check_output
> > +
> > +droot = 'debian'
> > +if len(sys.argv) > 1:
> > +    droot = sys.argv[1]
> > +
> > +debian = 'debian'  
> 
> Should be
> debian = 'debian.master'

Indeed.

 
> > +if len(sys.argv) > 2:
> > +    debian = sys.argv[2]
> > +
> > +rules = os.path.join(droot, 'rules')
> > +changelog = os.path.join(debian, 'changelog')
> > +changelog_new = os.path.join(debian, 'changelog.new')
> > +
> > +# Generate the list of new changes
> > +changes = check_output(['make', '-s', '-f', rules, 'printchanges']).decode('UTF-8')
> > +  
> 
> Once again, I wonder if we can make this better: make calling a script
> calling make calling a script. Make is good when you have a dependency
> tree, but that is not the case here. I am just thinking that we should
> consider the option where we keep these tools in tree, and still allow them
> to be run from make if we really care, but have cranky start calling them
> directly.

Good point. Will look into it.

 
> > +# Insert the new changes into the changelog
> > +with open(changelog) as orig, open(changelog_new, 'w') as new:
> > +    printed = False
> > +    for line in orig:
> > +        if line.startswith('  CHANGELOG: '):
> > +            if not printed:
> > +                printed = True
> > +                new.write(changes)
> > +        else:
> > +            new.write(line)
> > +  
> 
> The newline flag is missing here. It has certainly caused a regression, after I
> gave it a try.

What regression is that? The output looks just fine to me. No double empty
lines in the modified changelog entry.

Oh, if debian.master/changes is empty? When would that happen?

...Juerg


> Cascardo.
> 
> > +# Replace the original changelog with the new one
> > +os.rename(changelog_new, changelog)
> > diff --git a/debian/scripts/misc/insert-changes.pl b/debian/scripts/misc/insert-changes.pl
> > deleted file mode 100755
> > index 4eed4e28f9d3..000000000000
> > --- a/debian/scripts/misc/insert-changes.pl
> > +++ /dev/null
> > @@ -1,43 +0,0 @@
> > -#!/usr/bin/perl -w
> > -
> > -my $debian;
> > -$droot = $ARGV[0] if (defined $ARGV[0]);
> > -$droot = 'debian' if (!defined $droot);
> > -$debian = $ARGV[1] if (defined $ARGV[1]);
> > -$debian = 'debian.master' if (!defined $debian);
> > -
> > -system("make -s -f $droot/rules printchanges > $debian/changes");
> > -
> > -open(CHANGELOG, "< $debian/changelog") or die "Cannot open changelog";
> > -open(CHANGES, "< $debian/changes") or die "Cannot open new changes";
> > -open(NEW, "> $debian/changelog.new") or die "Cannot open new changelog";
> > -
> > -$printed = 0;
> > -my $skip_newline = 0;
> > -
> > -while (<CHANGELOG>) {
> > -	if (/^  CHANGELOG: /) {
> > -		next if $printed;
> > -
> > -		$skip_newline = 1;
> > -		while (<CHANGES>) {
> > -			$skip_newline = 0;
> > -			print NEW;
> > -		}
> > -
> > -		$printed = 1;
> > -	} else {
> > -		if (/^$/ && $skip_newline == 1) {
> > -			$skip_newline = 0;
> > -			next;
> > -		}
> > -		print NEW;
> > -	}
> > -}
> > -
> > -close(NEW);
> > -close(CHANGES);
> > -close(CHANGELOG);
> > -
> > -rename("$debian/changelog.new", "$debian/changelog");
> > -unlink("$debian/changes");
> > -- 
> > 2.30.2
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20211102/da71ae0f/attachment-0001.sig>