| 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 | expand |
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
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
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
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.
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
I will submit a new version.
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) {
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(-)