| Message ID | 20230606105706.60807b85ff79.I21ab3b54eeebd638676bead3b2f87417944e44f3@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] kernel-doc: don't let V=1 change outcome | expand |
On Tue, Jun 6, 2023 at 5:57 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > The kernel-doc script currently reports a number of issues > only in "verbose" mode, but that's initialized from V=1 > (via KBUILD_VERBOSE), so if you use KDOC_WERROR=1 then > adding V=1 might actually break the build. This is rather > unexpected. > > Change kernel-doc to not change its behaviour wrt. errors > (or warnings) when verbose mode is enabled, but rather add > separate warning flags (and -Wall) for it. Allow enabling > those flags via environment/make variables in the kernel's > build system for easier user use, but to not have to parse > them in the script itself. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > v2: - parse environment variables in build system rather than > the script itself, as suggested by Masahiro Yamada > - fix indentation > --- > scripts/Makefile.build | 7 ++++++- > scripts/kernel-doc | 28 +++++++++++++++++++++++----- > 2 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 9f94fc83f086..90bb5badb0e9 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -101,7 +101,12 @@ else ifeq ($(KBUILD_CHECKSRC),2) > endif > > ifneq ($(KBUILD_EXTRA_WARN),) > - cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $< > + cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > + $(if $(KDOC_WALL), -Wall) \ > + $(if $(KDOC_WRETURN), -Wreturn) \ > + $(if $(KDOC_WSHORT_DESC), -Wshort-desc) \ > + $(if $(KDOC_WSHORT_DESC), -Wcontents-before-sections) \ Sorry, I misunderstood your intention. (I just thought existing env variables would be moved to Makefile) I do not want to proliferate env variables any more. If you need per-flag control, maybe we can do like this? cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ $(KDOCFLAGS) Then, users can do $ make KDOCFLAGS=-Wall $ make KDOCFLAGS=-Wreturn etc. > + $< > endif > > # Compile C sources (.c) > diff --git a/scripts/kernel-doc b/scripts/kernel-doc > index 2486689ffc7b..8f8440870a0f 100755 > --- a/scripts/kernel-doc > +++ b/scripts/kernel-doc > @@ -23,7 +23,7 @@ kernel-doc - Print formatted kernel documentation to stdout > > =head1 SYNOPSIS > > - kernel-doc [-h] [-v] [-Werror] > + kernel-doc [-h] [-v] [-Werror] [-Wall] [-Wreturn] [-Wshort-description] [-Wcontents-before-sections] > [ -man | > -rst [-sphinx-version VERSION] [-enable-lineno] | > -none > @@ -133,6 +133,9 @@ my $dohighlight = ""; > > my $verbose = 0; > my $Werror = 0; > +my $Wreturn = 0; > +my $Wshort_desc = 0; > +my $Wcontents_before_sections = 0; > my $output_mode = "rst"; > my $output_preformatted = 0; > my $no_doc_sections = 0; > @@ -187,9 +190,14 @@ if (defined($ENV{'KCFLAGS'})) { > } > } > > +# reading this variable is for backwards compat just in case > +# someone was calling it with the variable from outside the > +# kernel's build system > if (defined($ENV{'KDOC_WERROR'})) { > $Werror = "$ENV{'KDOC_WERROR'}"; > } > +# other environment variables are converted to command-line > +# arguments in cmd_checkdoc in the build system > > # Generated docbook code is inserted in a template at a point where > # docbook v3.1 requires a non-zero sequence of RefEntry's; see: > @@ -318,6 +326,16 @@ while ($ARGV[0] =~ m/^--?(.*)/) { > $verbose = 1; > } elsif ($cmd eq "Werror") { > $Werror = 1; > + } elsif ($cmd eq "Wreturn") { > + $Wreturn = 1; > + } elsif ($cmd eq "Wshort-desc") { > + $Wshort_desc = 1; > + } elsif ($cmd eq "Wcontents-before-sections") { > + $Wcontents_before_sections = 1; > + } elsif ($cmd eq "Wall") { > + $Wreturn = 1; > + $Wshort_desc = 1; > + $Wcontents_before_sections = 1; > } elsif (($cmd eq "h") || ($cmd eq "help")) { > pod2usage(-exitval => 0, -verbose => 2); > } elsif ($cmd eq 'no-doc-sections') { > @@ -1748,9 +1766,9 @@ sub dump_function($$) { > # This check emits a lot of warnings at the moment, because many > # functions don't have a 'Return' doc section. So until the number > # of warnings goes sufficiently down, the check is only performed in > - # verbose mode. > + # -Wreturn mode. > # TODO: always perform the check. > - if ($verbose && !$noret) { > + if ($Wreturn && !$noret) { > check_return_section($file, $declaration_name, $return_type); > } > > @@ -2054,7 +2072,7 @@ sub process_name($$) { > $state = STATE_NORMAL; > } > > - if (($declaration_purpose eq "") && $verbose) { > + if (($declaration_purpose eq "") && $Wshort_desc) { > emit_warning("${file}:$.", "missing initial short description on line:\n$_"); > } > > @@ -2103,7 +2121,7 @@ sub process_body($$) { > } > > if (($contents ne "") && ($contents ne "\n")) { > - if (!$in_doc_sect && $verbose) { > + if (!$in_doc_sect && $Wcontents_before_sections) { > emit_warning("${file}:$.", "contents before sections\n"); > } > dump_section($file, $section, $contents); > -- > 2.40.1 > -- Best Regards Masahiro Yamada
On Tue, 2023-06-06 at 20:15 +0900, Masahiro Yamada wrote: > > > > ifneq ($(KBUILD_EXTRA_WARN),) > > - cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $< > > + cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > > + $(if $(KDOC_WALL), -Wall) \ > > + $(if $(KDOC_WRETURN), -Wreturn) \ > > + $(if $(KDOC_WSHORT_DESC), -Wshort-desc) \ > > + $(if $(KDOC_WSHORT_DESC), -Wcontents-before-sections) \ > > > > Sorry, I misunderstood your intention. > (I just thought existing env variables would be moved to Makefile) > > > I do not want to proliferate env variables any more. Oh, ok, sure. > If you need per-flag control, maybe we can do like this? Well honestly, I myself just want to pass -Wall, but not necessarily W=2 since that adds more stuff from the C compiler. > cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > $(KDOCFLAGS) > > > Then, users can do > > $ make KDOCFLAGS=-Wall > $ make KDOCFLAGS=-Wreturn I'd rather call it KDOC_FLAGS if you don't mind to align with KDOC_WERROR which we have already, but sure, can do. johannes
On Wed, Jun 7, 2023 at 6:07 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2023-06-06 at 20:15 +0900, Masahiro Yamada wrote: > > > > > > ifneq ($(KBUILD_EXTRA_WARN),) > > > - cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $< > > > + cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > > > + $(if $(KDOC_WALL), -Wall) \ > > > + $(if $(KDOC_WRETURN), -Wreturn) \ > > > + $(if $(KDOC_WSHORT_DESC), -Wshort-desc) \ > > > + $(if $(KDOC_WSHORT_DESC), -Wcontents-before-sections) \ > > > > > > > > Sorry, I misunderstood your intention. > > (I just thought existing env variables would be moved to Makefile) > > > > > > I do not want to proliferate env variables any more. > > Oh, ok, sure. > > > If you need per-flag control, maybe we can do like this? > > Well honestly, I myself just want to pass -Wall, but not necessarily W=2 > since that adds more stuff from the C compiler. > > > cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > > $(KDOCFLAGS) > > > > > > Then, users can do > > > > $ make KDOCFLAGS=-Wall > > $ make KDOCFLAGS=-Wreturn > > I'd rather call it KDOC_FLAGS if you don't mind to align with > KDOC_WERROR which we have already, but sure, can do. I just tried to be consistent with CPPFLAGS, CFLAGS, AFLAGS, CHECKFLAGS etc. (CHECKFLAGS is for sparse) because you apparently mimick compiler flags in kernel-doc. BTW, kernel-doc is invoked from Documentation/Makefile too. Do we need to pass the same flags to both of them? > johannes -- Best Regards Masahiro Yamada
Johannes Berg <johannes@sipsolutions.net> writes: > From: Johannes Berg <johannes.berg@intel.com> > > The kernel-doc script currently reports a number of issues > only in "verbose" mode, but that's initialized from V=1 > (via KBUILD_VERBOSE), so if you use KDOC_WERROR=1 then > adding V=1 might actually break the build. This is rather > unexpected. > > Change kernel-doc to not change its behaviour wrt. errors > (or warnings) when verbose mode is enabled, but rather add > separate warning flags (and -Wall) for it. Allow enabling > those flags via environment/make variables in the kernel's > build system for easier user use, but to not have to parse > them in the script itself. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > v2: - parse environment variables in build system rather than > the script itself, as suggested by Masahiro Yamada > - fix indentation So this change seems fine to me; Masahiro, if you want to take these, feel free to add: Acked-by: Jonathan Corbet <corbet@lwn.net> Thanks, jon
On Thu, 2023-06-08 at 08:51 +0900, Masahiro Yamada wrote: > > > > > cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ > > > $(KDOCFLAGS) > > > > > > > > > Then, users can do > > > > > > $ make KDOCFLAGS=-Wall > > > $ make KDOCFLAGS=-Wreturn > > > > I'd rather call it KDOC_FLAGS if you don't mind to align with > > KDOC_WERROR which we have already, but sure, can do. > > > I just tried to be consistent with > CPPFLAGS, CFLAGS, AFLAGS, CHECKFLAGS etc. > (CHECKFLAGS is for sparse) because > you apparently mimick compiler flags in kernel-doc. OK, sure, works for me. I made it mimic compiler flags because it was already doing it for -Werror :) > BTW, kernel-doc is invoked from Documentation/Makefile too. > > Do we need to pass the same flags to both of them? It doesn't look like we _can_ in that case, at least not from the build system, it'd have to be in kerneldoc.py or something. I also didn't think it was that important, so I left it. johannes
diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 9f94fc83f086..90bb5badb0e9 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -101,7 +101,12 @@ else ifeq ($(KBUILD_CHECKSRC),2) endif ifneq ($(KBUILD_EXTRA_WARN),) - cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $< + cmd_checkdoc = $(srctree)/scripts/kernel-doc -none \ + $(if $(KDOC_WALL), -Wall) \ + $(if $(KDOC_WRETURN), -Wreturn) \ + $(if $(KDOC_WSHORT_DESC), -Wshort-desc) \ + $(if $(KDOC_WSHORT_DESC), -Wcontents-before-sections) \ + $< endif # Compile C sources (.c) diff --git a/scripts/kernel-doc b/scripts/kernel-doc index 2486689ffc7b..8f8440870a0f 100755 --- a/scripts/kernel-doc +++ b/scripts/kernel-doc @@ -23,7 +23,7 @@ kernel-doc - Print formatted kernel documentation to stdout =head1 SYNOPSIS - kernel-doc [-h] [-v] [-Werror] + kernel-doc [-h] [-v] [-Werror] [-Wall] [-Wreturn] [-Wshort-description] [-Wcontents-before-sections] [ -man | -rst [-sphinx-version VERSION] [-enable-lineno] | -none @@ -133,6 +133,9 @@ my $dohighlight = ""; my $verbose = 0; my $Werror = 0; +my $Wreturn = 0; +my $Wshort_desc = 0; +my $Wcontents_before_sections = 0; my $output_mode = "rst"; my $output_preformatted = 0; my $no_doc_sections = 0; @@ -187,9 +190,14 @@ if (defined($ENV{'KCFLAGS'})) { } } +# reading this variable is for backwards compat just in case +# someone was calling it with the variable from outside the +# kernel's build system if (defined($ENV{'KDOC_WERROR'})) { $Werror = "$ENV{'KDOC_WERROR'}"; } +# other environment variables are converted to command-line +# arguments in cmd_checkdoc in the build system # Generated docbook code is inserted in a template at a point where # docbook v3.1 requires a non-zero sequence of RefEntry's; see: @@ -318,6 +326,16 @@ while ($ARGV[0] =~ m/^--?(.*)/) { $verbose = 1; } elsif ($cmd eq "Werror") { $Werror = 1; + } elsif ($cmd eq "Wreturn") { + $Wreturn = 1; + } elsif ($cmd eq "Wshort-desc") { + $Wshort_desc = 1; + } elsif ($cmd eq "Wcontents-before-sections") { + $Wcontents_before_sections = 1; + } elsif ($cmd eq "Wall") { + $Wreturn = 1; + $Wshort_desc = 1; + $Wcontents_before_sections = 1; } elsif (($cmd eq "h") || ($cmd eq "help")) { pod2usage(-exitval => 0, -verbose => 2); } elsif ($cmd eq 'no-doc-sections') { @@ -1748,9 +1766,9 @@ sub dump_function($$) { # This check emits a lot of warnings at the moment, because many # functions don't have a 'Return' doc section. So until the number # of warnings goes sufficiently down, the check is only performed in - # verbose mode. + # -Wreturn mode. # TODO: always perform the check. - if ($verbose && !$noret) { + if ($Wreturn && !$noret) { check_return_section($file, $declaration_name, $return_type); } @@ -2054,7 +2072,7 @@ sub process_name($$) { $state = STATE_NORMAL; } - if (($declaration_purpose eq "") && $verbose) { + if (($declaration_purpose eq "") && $Wshort_desc) { emit_warning("${file}:$.", "missing initial short description on line:\n$_"); } @@ -2103,7 +2121,7 @@ sub process_body($$) { } if (($contents ne "") && ($contents ne "\n")) { - if (!$in_doc_sect && $verbose) { + if (!$in_doc_sect && $Wcontents_before_sections) { emit_warning("${file}:$.", "contents before sections\n"); } dump_section($file, $section, $contents);