[X,B,C,D,unstable] UBUNTU: [Packaging] config-check: allow overlay annotations files

Message ID 20190131154617.24363-1-marcelo.cerri@canonical.com
State New
Headers show
Series
  • [X,B,C,D,unstable] UBUNTU: [Packaging] config-check: allow overlay annotations files
Related show

Commit Message

Marcelo Henrique Cerri Jan. 31, 2019, 3:46 p.m.
BugLink: http://bugs.launchpad.net/bugs/1752072

Update the config-check script to parse additional annotations files
that can overlay the original annotations on a config basis. The overlay
files are read from "$DEBIAN/config/*.annotations".

Since the overlay works on a config basis, any mention to a config in
the overlay file will cause the entry for that config on the original
file to be ignore.

For instance, the following line in the overlay file:

CONFIG_X note<some note>

Will cause any line related to CONFIG_X in the original annotations file
to be ignored.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
---

I intend to use this patch to better handle config annotations for
custom, and specially cloud, kernels. That way we can keep
"debian.$BRANCH/config/annotations" as a symlink to the base kernel
(usually "debian.master/config/annotations") and keep the
customizations for the custom kernel on a separate file (ie:
"debian.$BRANCH/config/$BRANCH.annotations").

With this patch applied I'm planning to extract all the configs
requirements for the cloud kernel on a separate file. That should help
us to have a clear perspect of the the customizations made to a kernel.

---
 debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 20 deletions(-)

Comments

Kamal Mostafa Jan. 31, 2019, 3:51 p.m. | #1
Great idea!

Acked-by: Kamal Mostafa <kamal@canonical.com>

 -Kamal

On Thu, Jan 31, 2019 at 01:46:17PM -0200, Marcelo Henrique Cerri wrote:
> BugLink: http://bugs.launchpad.net/bugs/1752072
> 
> Update the config-check script to parse additional annotations files
> that can overlay the original annotations on a config basis. The overlay
> files are read from "$DEBIAN/config/*.annotations".
> 
> Since the overlay works on a config basis, any mention to a config in
> the overlay file will cause the entry for that config on the original
> file to be ignore.
> 
> For instance, the following line in the overlay file:
> 
> CONFIG_X note<some note>
> 
> Will cause any line related to CONFIG_X in the original annotations file
> to be ignored.
> 
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> ---
> 
> I intend to use this patch to better handle config annotations for
> custom, and specially cloud, kernels. That way we can keep
> "debian.$BRANCH/config/annotations" as a symlink to the base kernel
> (usually "debian.master/config/annotations") and keep the
> customizations for the custom kernel on a separate file (ie:
> "debian.$BRANCH/config/$BRANCH.annotations").
> 
> With this patch applied I'm planning to extract all the configs
> requirements for the cloud kernel on a separate file. That should help
> us to have a clear perspect of the the customizations made to a kernel.
> 
> ---
>  debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> index 07958556e58e..e28d28153c4b 100755
> --- a/debian/scripts/config-check
> +++ b/debian/scripts/config-check
> @@ -41,33 +41,38 @@ close(CONFIG);
>  # ANNOTATIONS: check any annotations marked for enforcement
>  my $pass = 0;
>  my $total = 0;
> -my $annotations = "$commonconfig/annotations";
>  my ($config, $value, $options, $option, $value, $check, $policy);
> -print "$P: $annotations loading annotations\n";
> +
>  my %annot;
> -my $form = 1;
> -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> -while (<ANNOTATIONS>) {
> -	if (/^# FORMAT: (\S+)/) {
> -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> -		$form = $1;
> -	}
> +my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));
> +for my $annotations (@annotations_list) {
> +	print "$P: $annotations loading annotations\n";
> +	my %current_annot;
> +	my $form = 1;
> +	open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> +	while (<ANNOTATIONS>) {
> +		if (/^# FORMAT: (\S+)/) {
> +			die "$P: $1: unknown annotations format\n" if ($1 != 2);
> +			$form = $1;
> +		}
>  
> -	/^#/ && next;
> -	chomp;
> -	/^$/ && next;
> +		/^#/ && next;
> +		chomp;
> +		/^$/ && next;
>  
> -	/^CONFIG_/ || next;
> +		/^CONFIG_/ || next;
>  
> -	if ($form == 1) {
> -		($config, $value, $options) = split(' ', $_, 3);
> -	} elsif ($form == 2) {
> -		($config, $options) = split(' ', $_, 2);
> -	}
> +		if ($form == 1) {
> +			($config, $value, $options) = split(' ', $_, 3);
> +		} elsif ($form == 2) {
> +			($config, $options) = split(' ', $_, 2);
> +		}
>  
> -	$annot{$config} = $annot{$config} . ' ' . $options;
> +		$current_annot{$config} = $current_annot{$config} . ' ' . $options;
> +	}
> +	close(ANNOTATIONS);
> +	%annot = ( %annot, %current_annot );
>  }
> -close(ANNOTATIONS);
>  
>  my $config;
>  for $config (keys %annot) {
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Seth Forshee Jan. 31, 2019, 4:17 p.m. | #2
On Thu, Jan 31, 2019 at 01:46:17PM -0200, Marcelo Henrique Cerri wrote:
> BugLink: http://bugs.launchpad.net/bugs/1752072
> 
> Update the config-check script to parse additional annotations files
> that can overlay the original annotations on a config basis. The overlay
> files are read from "$DEBIAN/config/*.annotations".
> 
> Since the overlay works on a config basis, any mention to a config in
> the overlay file will cause the entry for that config on the original
> file to be ignore.
> 
> For instance, the following line in the overlay file:
> 
> CONFIG_X note<some note>
> 
> Will cause any line related to CONFIG_X in the original annotations file
> to be ignored.
> 
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> ---
> 
> I intend to use this patch to better handle config annotations for
> custom, and specially cloud, kernels. That way we can keep
> "debian.$BRANCH/config/annotations" as a symlink to the base kernel
> (usually "debian.master/config/annotations") and keep the
> customizations for the custom kernel on a separate file (ie:
> "debian.$BRANCH/config/$BRANCH.annotations").
> 
> With this patch applied I'm planning to extract all the configs
> requirements for the cloud kernel on a separate file. That should help
> us to have a clear perspect of the the customizations made to a kernel.
> 
> ---
>  debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> index 07958556e58e..e28d28153c4b 100755
> --- a/debian/scripts/config-check
> +++ b/debian/scripts/config-check
> @@ -41,33 +41,38 @@ close(CONFIG);
>  # ANNOTATIONS: check any annotations marked for enforcement
>  my $pass = 0;
>  my $total = 0;
> -my $annotations = "$commonconfig/annotations";
>  my ($config, $value, $options, $option, $value, $check, $policy);
> -print "$P: $annotations loading annotations\n";
> +
>  my %annot;
> -my $form = 1;
> -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> -while (<ANNOTATIONS>) {
> -	if (/^# FORMAT: (\S+)/) {
> -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> -		$form = $1;
> -	}
> +my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));

I don't think we should glob here, then we're creating the ability to
layer multiple annotations overlays but without some good criteria for
which ones take precedence (I assume it would turn out to be ascii sort
order).

I'd prefer if this were maybe $branch.annotations, or even just
something like annotations.overlay. Otherwise I think this looks fine.

> +for my $annotations (@annotations_list) {
> +	print "$P: $annotations loading annotations\n";
> +	my %current_annot;
> +	my $form = 1;
> +	open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> +	while (<ANNOTATIONS>) {
> +		if (/^# FORMAT: (\S+)/) {
> +			die "$P: $1: unknown annotations format\n" if ($1 != 2);
> +			$form = $1;
> +		}
>  
> -	/^#/ && next;
> -	chomp;
> -	/^$/ && next;
> +		/^#/ && next;
> +		chomp;
> +		/^$/ && next;
>  
> -	/^CONFIG_/ || next;
> +		/^CONFIG_/ || next;
>  
> -	if ($form == 1) {
> -		($config, $value, $options) = split(' ', $_, 3);
> -	} elsif ($form == 2) {
> -		($config, $options) = split(' ', $_, 2);
> -	}
> +		if ($form == 1) {
> +			($config, $value, $options) = split(' ', $_, 3);
> +		} elsif ($form == 2) {
> +			($config, $options) = split(' ', $_, 2);
> +		}
>  
> -	$annot{$config} = $annot{$config} . ' ' . $options;
> +		$current_annot{$config} = $current_annot{$config} . ' ' . $options;
> +	}
> +	close(ANNOTATIONS);
> +	%annot = ( %annot, %current_annot );
>  }
> -close(ANNOTATIONS);
>  
>  my $config;
>  for $config (keys %annot) {
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Andy Whitcroft Jan. 31, 2019, 5:10 p.m. | #3
On Thu, Jan 31, 2019 at 10:17:36AM -0600, Seth Forshee wrote:
> On Thu, Jan 31, 2019 at 01:46:17PM -0200, Marcelo Henrique Cerri wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1752072
> > 
> > Update the config-check script to parse additional annotations files
> > that can overlay the original annotations on a config basis. The overlay
> > files are read from "$DEBIAN/config/*.annotations".
> > 
> > Since the overlay works on a config basis, any mention to a config in
> > the overlay file will cause the entry for that config on the original
> > file to be ignore.
> > 
> > For instance, the following line in the overlay file:
> > 
> > CONFIG_X note<some note>
> > 
> > Will cause any line related to CONFIG_X in the original annotations file
> > to be ignored.
> > 
> > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > ---
> > 
> > I intend to use this patch to better handle config annotations for
> > custom, and specially cloud, kernels. That way we can keep
> > "debian.$BRANCH/config/annotations" as a symlink to the base kernel
> > (usually "debian.master/config/annotations") and keep the
> > customizations for the custom kernel on a separate file (ie:
> > "debian.$BRANCH/config/$BRANCH.annotations").
> > 
> > With this patch applied I'm planning to extract all the configs
> > requirements for the cloud kernel on a separate file. That should help
> > us to have a clear perspect of the the customizations made to a kernel.
> > 
> > ---
> >  debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> > 
> > diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> > index 07958556e58e..e28d28153c4b 100755
> > --- a/debian/scripts/config-check
> > +++ b/debian/scripts/config-check
> > @@ -41,33 +41,38 @@ close(CONFIG);
> >  # ANNOTATIONS: check any annotations marked for enforcement
> >  my $pass = 0;
> >  my $total = 0;
> > -my $annotations = "$commonconfig/annotations";
> >  my ($config, $value, $options, $option, $value, $check, $policy);
> > -print "$P: $annotations loading annotations\n";
> > +
> >  my %annot;
> > -my $form = 1;
> > -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> > -while (<ANNOTATIONS>) {
> > -	if (/^# FORMAT: (\S+)/) {
> > -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> > -		$form = $1;
> > -	}
> > +my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));
> 
> I don't think we should glob here, then we're creating the ability to
> layer multiple annotations overlays but without some good criteria for
> which ones take precedence (I assume it would turn out to be ascii sort
> order).

Which is likely an order affected by LANG= settings; which sounds all
rather unreliable.

> 
> I'd prefer if this were maybe $branch.annotations, or even just
> something like annotations.overlay. Otherwise I think this looks fine.

Is there some reason it could not be in $DEBIAN so it is branch
specific?  Or are we hoping to check it into the master branch?

-apw
Thadeu Lima de Souza Cascardo Jan. 31, 2019, 5:47 p.m. | #4
On Thu, Jan 31, 2019 at 05:10:25PM +0000, Andy Whitcroft wrote:
> On Thu, Jan 31, 2019 at 10:17:36AM -0600, Seth Forshee wrote:
> > On Thu, Jan 31, 2019 at 01:46:17PM -0200, Marcelo Henrique Cerri wrote:
> > > BugLink: http://bugs.launchpad.net/bugs/1752072
> > > 
> > > Update the config-check script to parse additional annotations files
> > > that can overlay the original annotations on a config basis. The overlay
> > > files are read from "$DEBIAN/config/*.annotations".
> > > 
> > > Since the overlay works on a config basis, any mention to a config in
> > > the overlay file will cause the entry for that config on the original
> > > file to be ignore.
> > > 
> > > For instance, the following line in the overlay file:
> > > 
> > > CONFIG_X note<some note>
> > > 
> > > Will cause any line related to CONFIG_X in the original annotations file
> > > to be ignored.
> > > 
> > > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > > ---
> > > 
> > > I intend to use this patch to better handle config annotations for
> > > custom, and specially cloud, kernels. That way we can keep
> > > "debian.$BRANCH/config/annotations" as a symlink to the base kernel
> > > (usually "debian.master/config/annotations") and keep the
> > > customizations for the custom kernel on a separate file (ie:
> > > "debian.$BRANCH/config/$BRANCH.annotations").
> > > 
> > > With this patch applied I'm planning to extract all the configs
> > > requirements for the cloud kernel on a separate file. That should help
> > > us to have a clear perspect of the the customizations made to a kernel.
> > > 
> > > ---
> > >  debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
> > >  1 file changed, 25 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> > > index 07958556e58e..e28d28153c4b 100755
> > > --- a/debian/scripts/config-check
> > > +++ b/debian/scripts/config-check
> > > @@ -41,33 +41,38 @@ close(CONFIG);
> > >  # ANNOTATIONS: check any annotations marked for enforcement
> > >  my $pass = 0;
> > >  my $total = 0;
> > > -my $annotations = "$commonconfig/annotations";
> > >  my ($config, $value, $options, $option, $value, $check, $policy);
> > > -print "$P: $annotations loading annotations\n";
> > > +
> > >  my %annot;
> > > -my $form = 1;
> > > -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> > > -while (<ANNOTATIONS>) {
> > > -	if (/^# FORMAT: (\S+)/) {
> > > -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> > > -		$form = $1;
> > > -	}
> > > +my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));
> > 
> > I don't think we should glob here, then we're creating the ability to
> > layer multiple annotations overlays but without some good criteria for
> > which ones take precedence (I assume it would turn out to be ascii sort
> > order).
> 
> Which is likely an order affected by LANG= settings; which sounds all
> rather unreliable.
> 
> > 
> > I'd prefer if this were maybe $branch.annotations, or even just
> > something like annotations.overlay. Otherwise I think this looks fine.
> 
> Is there some reason it could not be in $DEBIAN so it is branch
> specific?  Or are we hoping to check it into the master branch?
> 
> -apw

Or if he is planning on always using the debian.master version of it, why not
have an "include" directive and include it, maybe with a format=3?

Cascardo.
Marcelo Henrique Cerri Feb. 1, 2019, 3:32 p.m. | #5
On Thu, Jan 31, 2019 at 05:10:25PM +0000, Andy Whitcroft wrote:
> On Thu, Jan 31, 2019 at 10:17:36AM -0600, Seth Forshee wrote:
> > On Thu, Jan 31, 2019 at 01:46:17PM -0200, Marcelo Henrique Cerri wrote:
> > > BugLink: http://bugs.launchpad.net/bugs/1752072
> > > 
> > > Update the config-check script to parse additional annotations files
> > > that can overlay the original annotations on a config basis. The overlay
> > > files are read from "$DEBIAN/config/*.annotations".
> > > 
> > > Since the overlay works on a config basis, any mention to a config in
> > > the overlay file will cause the entry for that config on the original
> > > file to be ignore.
> > > 
> > > For instance, the following line in the overlay file:
> > > 
> > > CONFIG_X note<some note>
> > > 
> > > Will cause any line related to CONFIG_X in the original annotations file
> > > to be ignored.
> > > 
> > > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > > ---
> > > 
> > > I intend to use this patch to better handle config annotations for
> > > custom, and specially cloud, kernels. That way we can keep
> > > "debian.$BRANCH/config/annotations" as a symlink to the base kernel
> > > (usually "debian.master/config/annotations") and keep the
> > > customizations for the custom kernel on a separate file (ie:
> > > "debian.$BRANCH/config/$BRANCH.annotations").
> > > 
> > > With this patch applied I'm planning to extract all the configs
> > > requirements for the cloud kernel on a separate file. That should help
> > > us to have a clear perspect of the the customizations made to a kernel.
> > > 
> > > ---
> > >  debian/scripts/config-check | 45 ++++++++++++++++++++-----------------
> > >  1 file changed, 25 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/debian/scripts/config-check b/debian/scripts/config-check
> > > index 07958556e58e..e28d28153c4b 100755
> > > --- a/debian/scripts/config-check
> > > +++ b/debian/scripts/config-check
> > > @@ -41,33 +41,38 @@ close(CONFIG);
> > >  # ANNOTATIONS: check any annotations marked for enforcement
> > >  my $pass = 0;
> > >  my $total = 0;
> > > -my $annotations = "$commonconfig/annotations";
> > >  my ($config, $value, $options, $option, $value, $check, $policy);
> > > -print "$P: $annotations loading annotations\n";
> > > +
> > >  my %annot;
> > > -my $form = 1;
> > > -open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
> > > -while (<ANNOTATIONS>) {
> > > -	if (/^# FORMAT: (\S+)/) {
> > > -		die "$P: $1: unknown annotations format\n" if ($1 != 2);
> > > -		$form = $1;
> > > -	}
> > > +my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));
> > 
> > I don't think we should glob here, then we're creating the ability to
> > layer multiple annotations overlays but without some good criteria for
> > which ones take precedence (I assume it would turn out to be ascii sort
> > order).
> 
> Which is likely an order affected by LANG= settings; which sounds all
> rather unreliable.
>

Perl's glob sorts by default in ascii order. But I do prefer a fixed
name to avoid unnecessary complications. I just used a glob to give it
some flexibility and to avoid pinning a name. Which, I think, was a
mistake.

I've written that patch a long time ago as a prove of concept and I
used bionic/linux-gcp as a test for it. In there I used
"overlay.annotations", so I think I stick with it to avoid issues with
gcp.


> > 
> > I'd prefer if this were maybe $branch.annotations, or even just
> > something like annotations.overlay. Otherwise I think this looks fine.
> 
> Is there some reason it could not be in $DEBIAN so it is branch
> specific?  Or are we hoping to check it into the master branch?

It is under $DEBIAN.

> 
> -apw

--
Regards,
Marcelo
Marcelo Henrique Cerri Feb. 1, 2019, 7:25 p.m. | #6
I will submit a new version.

Patch

diff --git a/debian/scripts/config-check b/debian/scripts/config-check
index 07958556e58e..e28d28153c4b 100755
--- a/debian/scripts/config-check
+++ b/debian/scripts/config-check
@@ -41,33 +41,38 @@  close(CONFIG);
 # ANNOTATIONS: check any annotations marked for enforcement
 my $pass = 0;
 my $total = 0;
-my $annotations = "$commonconfig/annotations";
 my ($config, $value, $options, $option, $value, $check, $policy);
-print "$P: $annotations loading annotations\n";
+
 my %annot;
-my $form = 1;
-open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
-while (<ANNOTATIONS>) {
-	if (/^# FORMAT: (\S+)/) {
-		die "$P: $1: unknown annotations format\n" if ($1 != 2);
-		$form = $1;
-	}
+my @annotations_list = ("$commonconfig/annotations", glob("$commonconfig/*.annotations"));
+for my $annotations (@annotations_list) {
+	print "$P: $annotations loading annotations\n";
+	my %current_annot;
+	my $form = 1;
+	open(ANNOTATIONS, "<$annotations") || die "$P: $annotations: open failed -- $! -- aborting\n";
+	while (<ANNOTATIONS>) {
+		if (/^# FORMAT: (\S+)/) {
+			die "$P: $1: unknown annotations format\n" if ($1 != 2);
+			$form = $1;
+		}
 
-	/^#/ && next;
-	chomp;
-	/^$/ && next;
+		/^#/ && next;
+		chomp;
+		/^$/ && next;
 
-	/^CONFIG_/ || next;
+		/^CONFIG_/ || next;
 
-	if ($form == 1) {
-		($config, $value, $options) = split(' ', $_, 3);
-	} elsif ($form == 2) {
-		($config, $options) = split(' ', $_, 2);
-	}
+		if ($form == 1) {
+			($config, $value, $options) = split(' ', $_, 3);
+		} elsif ($form == 2) {
+			($config, $options) = split(' ', $_, 2);
+		}
 
-	$annot{$config} = $annot{$config} . ' ' . $options;
+		$current_annot{$config} = $current_annot{$config} . ' ' . $options;
+	}
+	close(ANNOTATIONS);
+	%annot = ( %annot, %current_annot );
 }
-close(ANNOTATIONS);
 
 my $config;
 for $config (keys %annot) {