diff mbox series

[v2] checkpatch: allow build files to reference other build files

Message ID 20240112221947.1950503-1-willmcvicker@google.com
State New
Headers
Series [v2] checkpatch: allow build files to reference other build files |

Commit Message

William McVicker Jan. 12, 2024, 10:19 p.m. UTC 
  Add an exception to the EMBEDDED_FILENAME warning for build files. This
fixes the below warnings where the Kconfig and Makefile files reference
other similarly named build files.

  WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
  #24: FILE: Kconfig:34:
  +source "drivers/willmcvicker/Kconfig"

  WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
  #36: FILE: Makefile:667:
  +	} > Makefile

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

v2:
- Unwrap commit message lines
- Align and update regex



base-commit: 70d201a40823acba23899342d62bc2644051ad2e

Comments

Nicolas Schier Jan. 29, 2024, 9:22 a.m. UTC | #1 
On Fri, Jan 12, 2024 at 02:19:46PM -0800, Will McVicker wrote:
> Add an exception to the EMBEDDED_FILENAME warning for build files. This

As far as I can see, your patch fixes only the checkpatch warnings for
top-level Makefile and Kconfig (and leaving out top-level Kbuild).
Other build files are not affected, right?

Kind regards,
Nicolas


> fixes the below warnings where the Kconfig and Makefile files reference
> other similarly named build files.
> 
>   WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
>   #24: FILE: Kconfig:34:
>   +source "drivers/willmcvicker/Kconfig"
> 
>   WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
>   #36: FILE: Makefile:667:
>   +	} > Makefile
> 
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> v2:
> - Unwrap commit message lines
> - Align and update regex
> 
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f8343b34a28b..c2869803e545 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3785,7 +3785,8 @@ sub process {
>  		}
>  
>  # check for embedded filenames
> -		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/) {
> +		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/ &&
> +		    $realfile !~ /(?:Kconfig|Makefile)/) {
>  			WARN("EMBEDDED_FILENAME",
>  			     "It's generally not useful to have the filename in the file\n" . $herecurr);
>  		}
> 
> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
> -- 
> 2.43.0.275.g3460e3d667-goog
>
William McVicker Jan. 30, 2024, 10:19 p.m. UTC | #2 
On 01/29/2024, Nicolas Schier wrote:
> On Fri, Jan 12, 2024 at 02:19:46PM -0800, Will McVicker wrote:
> > Add an exception to the EMBEDDED_FILENAME warning for build files. This
> 
> As far as I can see, your patch fixes only the checkpatch warnings for
> top-level Makefile and Kconfig (and leaving out top-level Kbuild).
> Other build files are not affected, right?

Since $realfile includes the full path, I wasn't able to find a case where this
issue happens outside of the top-level build files. The same goes for Kbuild
files -- the top-level Kbuild file doesn't include other Kbuild files and the
other Kbuild files don't include other Kbuild files within the same directory.
If you prefer to protect against this warning in the future, I can include
Kbuild as well if you want.

Thanks,
Will

> 
> Kind regards,
> Nicolas
> 
> 
> > fixes the below warnings where the Kconfig and Makefile files reference
> > other similarly named build files.
> > 
> >   WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
> >   #24: FILE: Kconfig:34:
> >   +source "drivers/willmcvicker/Kconfig"
> > 
> >   WARNING:EMBEDDED_FILENAME: It's generally not useful to have the filename in the file
> >   #36: FILE: Makefile:667:
> >   +	} > Makefile
> > 
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  scripts/checkpatch.pl | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > v2:
> > - Unwrap commit message lines
> > - Align and update regex
> > 
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f8343b34a28b..c2869803e545 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3785,7 +3785,8 @@ sub process {
> >  		}
> >  
> >  # check for embedded filenames
> > -		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/) {
> > +		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/ &&
> > +		    $realfile !~ /(?:Kconfig|Makefile)/) {
> >  			WARN("EMBEDDED_FILENAME",
> >  			     "It's generally not useful to have the filename in the file\n" . $herecurr);
> >  		}
> > 
> > base-commit: 70d201a40823acba23899342d62bc2644051ad2e
> > -- 
> > 2.43.0.275.g3460e3d667-goog
> >
Nicolas Schier Feb. 8, 2024, 7:11 a.m. UTC | #3 
On Tue, Jan 30, 2024 at 02:19:23PM -0800, William McVicker wrote:
> On 01/29/2024, Nicolas Schier wrote:
> > On Fri, Jan 12, 2024 at 02:19:46PM -0800, Will McVicker wrote:
> > > Add an exception to the EMBEDDED_FILENAME warning for build files. This
> > 
> > As far as I can see, your patch fixes only the checkpatch warnings for
> > top-level Makefile and Kconfig (and leaving out top-level Kbuild).
> > Other build files are not affected, right?
> 
> Since $realfile includes the full path, I wasn't able to find a case where this
> issue happens outside of the top-level build files. The same goes for Kbuild
> files -- the top-level Kbuild file doesn't include other Kbuild files and the
> other Kbuild files don't include other Kbuild files within the same directory.
> If you prefer to protect against this warning in the future, I can include
> Kbuild as well if you want.

yes, I think it would be more complete if top-level Kbuild is also
included.  Could you also mention 'top-level' somewhere in the commit
message?

Thanks and kind regards,
Nicolas
diff mbox series

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f8343b34a28b..c2869803e545 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3785,7 +3785,8 @@  sub process {
 		}
 
 # check for embedded filenames
-		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/) {
+		if ($rawline =~ /^\+.*\b\Q$realfile\E\b/ &&
+		    $realfile !~ /(?:Kconfig|Makefile)/) {
 			WARN("EMBEDDED_FILENAME",
 			     "It's generally not useful to have the filename in the file\n" . $herecurr);
 		}