[PATCH 1/2] keytable: Add source information in generated keymaps

[PATCH 1/2] keytable: Add source information in generated keymaps

[Bastien Nocera]

Add comments to mention that keymap files are generated, and that
they shouldn't be modified by hand. Also list which tool was used
to generate them and the kernel source filename.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 utils/keytable/gen_keytables.pl | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/utils/keytable/gen_keytables.pl b/utils/keytable/gen_keytables.pl
index 4124e366..3dc74ba6 100755
--- a/utils/keytable/gen_keytables.pl
+++ b/utils/keytable/gen_keytables.pl
@@ -36,10 +36,17 @@ sub flush($$)
 	my $filename = shift;
 	my $legacy = shift;
 	my $defined;
+	my $relative_filename = $filename;
 
 	return if (!$keyname || !$out);
-	print "Creating $dir/$keyname.toml\n";
+	$relative_filename =~ s/^$kernel_dir//;
+	$relative_filename =~ s/^\///;
+	print "Creating $dir/$keyname.toml from $relative_filename\n";
 	open OUT, ">$dir/$keyname.toml";
+	print OUT "# This file is a generated data file, do not modify manually\n";
+	print OUT "#\n";
+	print OUT "# Generated with gen_keytables.pl in v4l-utils\n";
+	print OUT "# using $relative_filename as a source file\n";
 	print OUT "[[protocols]]\n";
 	print OUT "name = \"$keyname\"\n";
 	print OUT "protocol = \"$type\"\n";
-- 
2.21.0

[PATCH 2/2] keytable: Remove comments before processing keymaps

[Bastien Nocera]

Do our best to remove comments from each line we process from the keymap
sources, so as to avoid commented duplicates and false positives
sneaking in to the keymap definitions.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 utils/keytable/gen_keytables.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils/keytable/gen_keytables.pl b/utils/keytable/gen_keytables.pl
index 3dc74ba6..d73daf58 100755
--- a/utils/keytable/gen_keytables.pl
+++ b/utils/keytable/gen_keytables.pl
@@ -138,6 +138,9 @@ sub parse_file($$)
 		}
 
 		if ($read) {
+			# Remove comments
+			~ s#/\*.*?\*/##sg;
+			~ s#.*\*/##sg;
 			if (m/(0x[\dA-Fa-f]+)[\s\,]+(KEY|BTN)(\_[^\s\,\}]+)/) {
 				$out .= "$1 = \"$2$3\"\n";
 				next;
-- 
2.21.0

Re: [PATCH 2/2] keytable: Remove comments before processing keymaps

[Sean Young]

On Mon, Jul 01, 2019 at 06:38:13PM +0200, Bastien Nocera wrote:
> Do our best to remove comments from each line we process from the keymap
> sources, so as to avoid commented duplicates and false positives
> sneaking in to the keymap definitions.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>  utils/keytable/gen_keytables.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/utils/keytable/gen_keytables.pl b/utils/keytable/gen_keytables.pl
> index 3dc74ba6..d73daf58 100755
> --- a/utils/keytable/gen_keytables.pl
> +++ b/utils/keytable/gen_keytables.pl
> @@ -138,6 +138,9 @@ sub parse_file($$)
>  		}
>  
>  		if ($read) {
> +			# Remove comments
> +			~ s#/\*.*?\*/##sg;
> +			~ s#.*\*/##sg;

This doesn't solve the

	/* { 0x800ff40b, KEY_ENTER },
	   not used */

case. Or // comments.

>  			if (m/(0x[\dA-Fa-f]+)[\s\,]+(KEY|BTN)(\_[^\s\,\}]+)/) {
>  				$out .= "$1 = \"$2$3\"\n";
>  				next;
> -- 
> 2.21.0

Re: [PATCH 2/2] keytable: Remove comments before processing keymaps

[Bastien Nocera]

On Tue, 2019-07-02 at 10:08 +0100, Sean Young wrote:
> On Mon, Jul 01, 2019 at 06:38:13PM +0200, Bastien Nocera wrote:
> > Do our best to remove comments from each line we process from the
> > keymap
> > sources, so as to avoid commented duplicates and false positives
> > sneaking in to the keymap definitions.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  utils/keytable/gen_keytables.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/utils/keytable/gen_keytables.pl
> > b/utils/keytable/gen_keytables.pl
> > index 3dc74ba6..d73daf58 100755
> > --- a/utils/keytable/gen_keytables.pl
> > +++ b/utils/keytable/gen_keytables.pl
> > @@ -138,6 +138,9 @@ sub parse_file($$)
> >  		}
> >  
> >  		if ($read) {
> > +			# Remove comments
> > +			~ s#/\*.*?\*/##sg;
> > +			~ s#.*\*/##sg;
> 
> This doesn't solve the
> 
> 	/* { 0x800ff40b, KEY_ENTER },
> 	   not used */
> 
> case. Or // comments.

Those weren't problems I was aware of, or that I was trying to solve...

The presence of a "0x800ff40b = KEY_ENTER" won't stop the remote
definition from loading, or the remote to work correctly, and //
comments usually don't get merged into the kernel sources.

> >  			if (m/(0x[\dA-Fa-
> > f]+)[\s\,]+(KEY|BTN)(\_[^\s\,\}]+)/) {
> >  				$out .= "$1 = \"$2$3\"\n";
> >  				next;
> > -- 
> > 2.21.0

Re: [PATCH 2/2] keytable: Remove comments before processing keymaps

[Bastien Nocera]

On Tue, 2019-07-02 at 10:08 +0100, Sean Young wrote:
> On Mon, Jul 01, 2019 at 06:38:13PM +0200, Bastien Nocera wrote:
> > Do our best to remove comments from each line we process from the
> > keymap
> > sources, so as to avoid commented duplicates and false positives
> > sneaking in to the keymap definitions.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  utils/keytable/gen_keytables.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/utils/keytable/gen_keytables.pl
> > b/utils/keytable/gen_keytables.pl
> > index 3dc74ba6..d73daf58 100755
> > --- a/utils/keytable/gen_keytables.pl
> > +++ b/utils/keytable/gen_keytables.pl
> > @@ -138,6 +138,9 @@ sub parse_file($$)
> >  		}
> >  
> >  		if ($read) {
> > +			# Remove comments
> > +			~ s#/\*.*?\*/##sg;
> > +			~ s#.*\*/##sg;
> 
> This doesn't solve the
> 
> 	/* { 0x800ff40b, KEY_ENTER },
> 	   not used */

This isn't in the current Linus kernel.

> case. Or // comments.

And this isn't used anywhere in the kernel either.

Those are the 2 lines that would remove those comments but I can't test
it. Note that if this was done properly, instead of fixing the bugs we
encounter, the whole parsing would need to be redone.

+                       ~ s#/\*.*?##sg;
+                       ~ s#//.*##sg;

> >  			if (m/(0x[\dA-Fa-
> > f]+)[\s\,]+(KEY|BTN)(\_[^\s\,\}]+)/) {
> >  				$out .= "$1 = \"$2$3\"\n";
> >  				next;
> > -- 
> > 2.21.0

Re: [PATCH 2/2] keytable: Remove comments before processing keymaps

[Sean Young]

On Tue, Jul 02, 2019 at 11:43:39AM +0200, Bastien Nocera wrote:
> On Tue, 2019-07-02 at 10:08 +0100, Sean Young wrote:
> > On Mon, Jul 01, 2019 at 06:38:13PM +0200, Bastien Nocera wrote:
> > > Do our best to remove comments from each line we process from the
> > > keymap
> > > sources, so as to avoid commented duplicates and false positives
> > > sneaking in to the keymap definitions.
> > > 
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > ---
> > >  utils/keytable/gen_keytables.pl | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/utils/keytable/gen_keytables.pl
> > > b/utils/keytable/gen_keytables.pl
> > > index 3dc74ba6..d73daf58 100755
> > > --- a/utils/keytable/gen_keytables.pl
> > > +++ b/utils/keytable/gen_keytables.pl
> > > @@ -138,6 +138,9 @@ sub parse_file($$)
> > >  		}
> > >  
> > >  		if ($read) {
> > > +			# Remove comments
> > > +			~ s#/\*.*?\*/##sg;
> > > +			~ s#.*\*/##sg;
> > 
> > This doesn't solve the
> > 
> > 	/* { 0x800ff40b, KEY_ENTER },
> > 	   not used */
> 
> This isn't in the current Linus kernel.

We might as well solve this properly and handle all cases.

> > case. Or // comments.
> 
> And this isn't used anywhere in the kernel either.

That's wrong. They are used.

> Those are the 2 lines that would remove those comments but I can't test
> it. Note that if this was done properly, instead of fixing the bugs we
> encounter, the whole parsing would need to be redone.
> 
> +                       ~ s#/\*.*?##sg;
> +                       ~ s#//.*##sg;

If you are unable to test it, I will fix it.

> 
> > >  			if (m/(0x[\dA-Fa-
> > > f]+)[\s\,]+(KEY|BTN)(\_[^\s\,\}]+)/) {
> > >  				$out .= "$1 = \"$2$3\"\n";
> > >  				next;
> > > -- 
> > > 2.21.0


Sean

Re: [PATCH 2/2] keytable: Remove comments before processing keymaps

[Bastien Nocera]

On Tue, 2019-07-02 at 12:44 +0100, Sean Young wrote:
> On Tue, Jul 02, 2019 at 11:43:39AM +0200, Bastien Nocera wrote:
> > On Tue, 2019-07-02 at 10:08 +0100, Sean Young wrote:
> > > On Mon, Jul 01, 2019 at 06:38:13PM +0200, Bastien Nocera wrote:
> > > > Do our best to remove comments from each line we process from
> > > > the
> > > > keymap
> > > > sources, so as to avoid commented duplicates and false
> > > > positives
> > > > sneaking in to the keymap definitions.
> > > > 
> > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > ---
> > > >  utils/keytable/gen_keytables.pl | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/utils/keytable/gen_keytables.pl
> > > > b/utils/keytable/gen_keytables.pl
> > > > index 3dc74ba6..d73daf58 100755
> > > > --- a/utils/keytable/gen_keytables.pl
> > > > +++ b/utils/keytable/gen_keytables.pl
> > > > @@ -138,6 +138,9 @@ sub parse_file($$)
> > > >  		}
> > > >  
> > > >  		if ($read) {
> > > > +			# Remove comments
> > > > +			~ s#/\*.*?\*/##sg;
> > > > +			~ s#.*\*/##sg;
> > > 
> > > This doesn't solve the
> > > 
> > > 	/* { 0x800ff40b, KEY_ENTER },
> > > 	   not used */
> > 
> > This isn't in the current Linus kernel.
> 
> We might as well solve this properly and handle all cases.
> 
> > > case. Or // comments.
> > 
> > And this isn't used anywhere in the kernel either.
> 
> That's wrong. They are used.

Use in the keymap definitions? I don't think so. Either that or the 2
additional lines underneath here don't work. I think they do.

> > Those are the 2 lines that would remove those comments but I can't
> > test
> > it. Note that if this was done properly, instead of fixing the bugs
> > we
> > encounter, the whole parsing would need to be redone.
> > 
> > +                       ~ s#/\*.*?##sg;
> > +                       ~ s#//.*##sg;
> 
> If you are unable to test it, I will fix it.

Re: [PATCH 1/2] keytable: Add source information in generated keymaps

[Sean Young]

On Mon, Jul 01, 2019 at 06:38:12PM +0200, Bastien Nocera wrote:
> Add comments to mention that keymap files are generated, and that
> they shouldn't be modified by hand. Also list which tool was used
> to generate them and the kernel source filename.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>  utils/keytable/gen_keytables.pl | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/keytable/gen_keytables.pl b/utils/keytable/gen_keytables.pl
> index 4124e366..3dc74ba6 100755
> --- a/utils/keytable/gen_keytables.pl
> +++ b/utils/keytable/gen_keytables.pl
> @@ -36,10 +36,17 @@ sub flush($$)
>  	my $filename = shift;
>  	my $legacy = shift;
>  	my $defined;
> +	my $relative_filename = $filename;
>  
>  	return if (!$keyname || !$out);
> -	print "Creating $dir/$keyname.toml\n";
> +	$relative_filename =~ s/^$kernel_dir//;
> +	$relative_filename =~ s/^\///;
> +	print "Creating $dir/$keyname.toml from $relative_filename\n";
>  	open OUT, ">$dir/$keyname.toml";
> +	print OUT "# This file is a generated data file, do not modify manually\n";
> +	print OUT "#\n";
> +	print OUT "# Generated with gen_keytables.pl in v4l-utils\n";
> +	print OUT "# using $relative_filename as a source file\n";

This is only relevant for a developer who is wanting to upstream their
keymap changes to v4l-utils. If it should be documented somewhere it
should be in the v4l-utils repo itself, so that anyone generating
patches will notice before spending too much time making a patch on the
keymaps directory.

Also some remotes do not have definitions in the kernel, only in v4l-utils.

For an end user who is trying to patch their keymap this is confusing. Someone
who just wants to patch their keymap for their remote might be put off
by this message; can they not simply edit the toml file?

NAK, I'm afraid.

>  	print OUT "[[protocols]]\n";
>  	print OUT "name = \"$keyname\"\n";
>  	print OUT "protocol = \"$type\"\n";
> -- 
> 2.21.0


Sean

Re: [PATCH 1/2] keytable: Add source information in generated keymaps

[Bastien Nocera]

On Wed, 2019-07-03 at 16:33 +0100, Sean Young wrote:
> On Mon, Jul 01, 2019 at 06:38:12PM +0200, Bastien Nocera wrote:
> > Add comments to mention that keymap files are generated, and that
> > they shouldn't be modified by hand. Also list which tool was used
> > to generate them and the kernel source filename.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  utils/keytable/gen_keytables.pl | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/utils/keytable/gen_keytables.pl
> > b/utils/keytable/gen_keytables.pl
> > index 4124e366..3dc74ba6 100755
> > --- a/utils/keytable/gen_keytables.pl
> > +++ b/utils/keytable/gen_keytables.pl
> > @@ -36,10 +36,17 @@ sub flush($$)
> >  	my $filename = shift;
> >  	my $legacy = shift;
> >  	my $defined;
> > +	my $relative_filename = $filename;
> >  
> >  	return if (!$keyname || !$out);
> > -	print "Creating $dir/$keyname.toml\n";
> > +	$relative_filename =~ s/^$kernel_dir//;
> > +	$relative_filename =~ s/^\///;
> > +	print "Creating $dir/$keyname.toml from $relative_filename\n";
> >  	open OUT, ">$dir/$keyname.toml";
> > +	print OUT "# This file is a generated data file, do not modify
> > manually\n";
> > +	print OUT "#\n";
> > +	print OUT "# Generated with gen_keytables.pl in v4l-utils\n";
> > +	print OUT "# using $relative_filename as a source file\n";
> 
> This is only relevant for a developer who is wanting to upstream
> their
> keymap changes to v4l-utils.

It's only relevant to you. Or the chump that tried to do things well
(me). I wouldn't have wasted this much time writing tests, or making
patches if there had been a bit of guidance on how you wanted those
problems fixed. I'll let you create a README for that instead.

I'll send the "check" patch again.