linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] keytable: Add source information in generated keymaps
@ 2019-07-01 16:38 Bastien Nocera
  2019-07-01 16:38 ` [PATCH 2/2] keytable: Remove comments before processing keymaps Bastien Nocera
  2019-07-03 15:33 ` [PATCH 1/2] keytable: Add source information in generated keymaps Sean Young
  0 siblings, 2 replies; 9+ messages in thread
From: Bastien Nocera @ 2019-07-01 16:38 UTC (permalink / raw)
  To: linux-media

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-01 16:38 [PATCH 1/2] keytable: Add source information in generated keymaps Bastien Nocera
@ 2019-07-01 16:38 ` Bastien Nocera
  2019-07-02  9:08   ` Sean Young
  2019-07-03 15:33 ` [PATCH 1/2] keytable: Add source information in generated keymaps Sean Young
  1 sibling, 1 reply; 9+ messages in thread
From: Bastien Nocera @ 2019-07-01 16:38 UTC (permalink / raw)
  To: linux-media

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-01 16:38 ` [PATCH 2/2] keytable: Remove comments before processing keymaps Bastien Nocera
@ 2019-07-02  9:08   ` Sean Young
  2019-07-02  9:29     ` Bastien Nocera
  2019-07-02  9:43     ` Bastien Nocera
  0 siblings, 2 replies; 9+ messages in thread
From: Sean Young @ 2019-07-02  9:08 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-media

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-02  9:08   ` Sean Young
@ 2019-07-02  9:29     ` Bastien Nocera
  2019-07-02  9:43     ` Bastien Nocera
  1 sibling, 0 replies; 9+ messages in thread
From: Bastien Nocera @ 2019-07-02  9:29 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-02  9:08   ` Sean Young
  2019-07-02  9:29     ` Bastien Nocera
@ 2019-07-02  9:43     ` Bastien Nocera
  2019-07-02 11:44       ` Sean Young
  1 sibling, 1 reply; 9+ messages in thread
From: Bastien Nocera @ 2019-07-02  9:43 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-02  9:43     ` Bastien Nocera
@ 2019-07-02 11:44       ` Sean Young
  2019-07-02 13:20         ` Bastien Nocera
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Young @ 2019-07-02 11:44 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-media

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] keytable: Remove comments before processing keymaps
  2019-07-02 11:44       ` Sean Young
@ 2019-07-02 13:20         ` Bastien Nocera
  0 siblings, 0 replies; 9+ messages in thread
From: Bastien Nocera @ 2019-07-02 13:20 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] keytable: Add source information in generated keymaps
  2019-07-01 16:38 [PATCH 1/2] keytable: Add source information in generated keymaps Bastien Nocera
  2019-07-01 16:38 ` [PATCH 2/2] keytable: Remove comments before processing keymaps Bastien Nocera
@ 2019-07-03 15:33 ` Sean Young
  2019-07-04 13:22   ` Bastien Nocera
  1 sibling, 1 reply; 9+ messages in thread
From: Sean Young @ 2019-07-03 15:33 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-media

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] keytable: Add source information in generated keymaps
  2019-07-03 15:33 ` [PATCH 1/2] keytable: Add source information in generated keymaps Sean Young
@ 2019-07-04 13:22   ` Bastien Nocera
  0 siblings, 0 replies; 9+ messages in thread
From: Bastien Nocera @ 2019-07-04 13:22 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-07-04 13:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-01 16:38 [PATCH 1/2] keytable: Add source information in generated keymaps Bastien Nocera
2019-07-01 16:38 ` [PATCH 2/2] keytable: Remove comments before processing keymaps Bastien Nocera
2019-07-02  9:08   ` Sean Young
2019-07-02  9:29     ` Bastien Nocera
2019-07-02  9:43     ` Bastien Nocera
2019-07-02 11:44       ` Sean Young
2019-07-02 13:20         ` Bastien Nocera
2019-07-03 15:33 ` [PATCH 1/2] keytable: Add source information in generated keymaps Sean Young
2019-07-04 13:22   ` Bastien Nocera

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).