[PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Mauro Carvalho Chehab]

When kernel-doc is called via kerneldoc.py, there's no need to
auto-detect the Sphinx version, as the Sphinx module already
knows it. So, add an optional parameter to allow changing the
Sphinx dialect.

As kernel-doc can also be manually called, keep the auto-detection
logic if the parameter was not specified. On such case, emit
a warning if sphinx-build can't be found at PATH.

Suggested-by: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 Documentation/sphinx/kerneldoc.py |  5 ++++
 scripts/kernel-doc                | 40 ++++++++++++++++++++++++-------
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index 233f610539f0..e9857ab904f1 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -71,6 +71,11 @@ class KernelDocDirective(Directive):
         env = self.state.document.settings.env
         cmd = [env.config.kerneldoc_bin, '-rst', '-enable-lineno']
 
+	# Pass the version string to kernel-doc, as it needs to use a different
+	# dialect, depending what the C domain supports for each specific
+	# Sphinx versions
+        cmd += ['-sphinx-version', sphinx.__version__]
+
         filename = env.config.kerneldoc_srctree + '/' + self.arguments[0]
         export_file_patterns = []
 
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 5ac3749905e5..01efb0afb8c2 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -56,6 +56,13 @@ Output format selection (mutually exclusive):
   -rst			Output reStructuredText format.
   -none			Do not output documentation, only warnings.
 
+Output format selection modifier (affects only ReST output):
+
+  -sphinx-version	Use the ReST C domain dialect compatible with an
+			specific Sphinx Version.
+			If not specified, kernel-doc will auto-detect using
+			the sphinx-build version found on PATH.
+
 Output selection (mutually exclusive):
   -export		Only output documentation for symbols that have been
 			exported using EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL()
@@ -270,7 +277,7 @@ if ($#ARGV == -1) {
 }
 
 my $kernelversion;
-my $sphinx_major;
+my ($sphinx_major, $sphinx_minor, $sphinx_patch);
 
 my $dohighlight = "";
 
@@ -457,6 +464,15 @@ while ($ARGV[0] =~ m/^--?(.*)/) {
 	    $enable_lineno = 1;
     } elsif ($cmd eq 'show-not-found') {
 	$show_not_found = 1;  # A no-op but don't fail
+    } elsif ($cmd eq "sphinx-version") {
+	my $ver_string = shift @ARGV;
+	if ($ver_string =~ m/^(\d+)\.(\d+)\.(\d+)/) {
+	    $sphinx_major = $1;
+	    $sphinx_minor = $2;
+	    $sphinx_patch = $3;
+	} else {
+	    die "Sphinx version should be at major.minor.patch format\n";
+	}
     } else {
 	# Unknown argument
         usage();
@@ -477,29 +493,37 @@ sub findprog($)
 sub get_sphinx_version()
 {
 	my $ver;
-	my $major = 1;
 
 	my $cmd = "sphinx-build";
 	if (!findprog($cmd)) {
 		my $cmd = "sphinx-build3";
-		return $major if (!findprog($cmd));
+		if (!findprog($cmd)) {
+			$sphinx_major = 1;
+			$sphinx_minor = 2;
+			$sphinx_patch = 0;
+			printf STDERR "Warning: Sphinx version not found. Using default (Sphinx version %d.%d.%d)\n",
+			       $sphinx_major, $sphinx_minor, $sphinx_patch;
+			return;
+		}
 	}
 
 	open IN, "$cmd --version 2>&1 |";
 	while (<IN>) {
 		if (m/^\s*sphinx-build\s+([\d]+)\.([\d\.]+)(\+\/[\da-f]+)?$/) {
-			$major=$1;
+			$sphinx_major = $1;
+			$sphinx_minor = $2;
+			$sphinx_patch = $3;
 			last;
 		}
 		# Sphinx 1.2.x uses a different format
 		if (m/^\s*Sphinx.*\s+([\d]+)\.([\d\.]+)$/) {
-			$major=$1;
+			$sphinx_major = $1;
+			$sphinx_minor = $2;
+			$sphinx_patch = $3;
 			last;
 		}
 	}
 	close IN;
-
-	return $major;
 }
 
 # get kernel version from env
@@ -2338,7 +2362,7 @@ sub process_file($) {
 }
 
 
-$sphinx_major = get_sphinx_version();
+get_sphinx_version() if (!$sphinx_major);
 $kernelversion = get_kernel_version();
 
 # generate a sequence of code that will splice in highlighting information
-- 
2.26.2

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Jonathan Corbet]

On Sun,  4 Oct 2020 10:02:03 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> When kernel-doc is called via kerneldoc.py, there's no need to
> auto-detect the Sphinx version, as the Sphinx module already
> knows it. So, add an optional parameter to allow changing the
> Sphinx dialect.
> 
> As kernel-doc can also be manually called, keep the auto-detection
> logic if the parameter was not specified. On such case, emit
> a warning if sphinx-build can't be found at PATH.
> 
> Suggested-by: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  Documentation/sphinx/kerneldoc.py |  5 ++++
>  scripts/kernel-doc                | 40 ++++++++++++++++++++++++-------
>  2 files changed, 37 insertions(+), 8 deletions(-)

So I'm glad to see this.  Still not fully sold on the autodetection, but if
we don't actually use it, maybe I can live with it :)

One little nit:

> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index 233f610539f0..e9857ab904f1 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> +    } elsif ($cmd eq "sphinx-version") {
> +	my $ver_string = shift @ARGV;
> +	if ($ver_string =~ m/^(\d+)\.(\d+)\.(\d+)/) {
> +	    $sphinx_major = $1;
> +	    $sphinx_minor = $2;
> +	    $sphinx_patch = $3;
> +	} else {
> +	    die "Sphinx version should be at major.minor.patch format\n";
> +	}

Can we allow just major.minor, with patch defaulting to zero?  People
passing this by hand may not want to look up their patch version every
time, and I doubt it will ever matter...

Thanks,

jon

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Mauro Carvalho Chehab]

Em Mon, 5 Oct 2020 10:17:36 -0600
Jonathan Corbet <corbet@lwn.net> escreveu:

> On Sun,  4 Oct 2020 10:02:03 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > When kernel-doc is called via kerneldoc.py, there's no need to
> > auto-detect the Sphinx version, as the Sphinx module already
> > knows it. So, add an optional parameter to allow changing the
> > Sphinx dialect.
> > 
> > As kernel-doc can also be manually called, keep the auto-detection
> > logic if the parameter was not specified. On such case, emit
> > a warning if sphinx-build can't be found at PATH.
> > 
> > Suggested-by: Jonathan Corbet <corbet@lwn.net>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  Documentation/sphinx/kerneldoc.py |  5 ++++
> >  scripts/kernel-doc                | 40 ++++++++++++++++++++++++-------
> >  2 files changed, 37 insertions(+), 8 deletions(-)  
> 
> So I'm glad to see this.  Still not fully sold on the autodetection, but if
> we don't actually use it, maybe I can live with it :)
> 
> One little nit:
> 
> > diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> > index 233f610539f0..e9857ab904f1 100644
> > --- a/Documentation/sphinx/kerneldoc.py
> > +++ b/Documentation/sphinx/kerneldoc.py
> > +    } elsif ($cmd eq "sphinx-version") {
> > +	my $ver_string = shift @ARGV;
> > +	if ($ver_string =~ m/^(\d+)\.(\d+)\.(\d+)/) {
> > +	    $sphinx_major = $1;
> > +	    $sphinx_minor = $2;
> > +	    $sphinx_patch = $3;
> > +	} else {
> > +	    die "Sphinx version should be at major.minor.patch format\n";
> > +	}  
> 
> Can we allow just major.minor, with patch defaulting to zero?  People
> passing this by hand may not want to look up their patch version every
> time, and I doubt it will ever matter...

Sure. It should be easy to make the third argument optional, although
the regex will be a little more harder to understand.

Something like this should do the trick:

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 01efb0afb8c2..104d79949a8a 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -466,12 +466,16 @@ while ($ARGV[0] =~ m/^--?(.*)/) {
 	$show_not_found = 1;  # A no-op but don't fail
     } elsif ($cmd eq "sphinx-version") {
 	my $ver_string = shift @ARGV;
-	if ($ver_string =~ m/^(\d+)\.(\d+)\.(\d+)/) {
+	if ($ver_string =~ m/^(\d+)\.(\d+)(?:\.?(\d+)?)/) {
 	    $sphinx_major = $1;
 	    $sphinx_minor = $2;
-	    $sphinx_patch = $3;
+	    if ($3) {
+		$sphinx_patch = $3;
+	    } else {
+		$sphinx_patch = 0;
+	    }
 	} else {
-	    die "Sphinx version should be at major.minor.patch format\n";
+	    die "Sphinx version should either major.minor or major.minor.patch format\n";
 	}
     } else {
 	# Unknown argument

As right now we don't support Sphinx version 3.0[1], we're actually using just
$sphinx_major. So, I'm wonder if it would make sense to also make <minor>
optional.

The change would be trivial, although the regex will become even more
harder to read ;-)

[1] not sure how valuable would be adding support for Sphinx 3.0. While
I didn't make any tests, I'm strongly suspecting that, with the approach
we took for backward/forward compatibility, adding support for it
would mean to just do a trivial change at cdomain.py by applying a
patch that Markus did replacing a regex function that doesn't exist
anymore at Sphinx API and emulating C namespace with the logic I
already implemented. 

I guess I'll give it a try anyway, as it seems weird to have a gap
in the middle of the supported versions.


Thanks,
Mauro

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Joe Perches]

On Tue, 2020-10-06 at 08:42 +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Oct 2020 10:17:36 -0600
> Jonathan Corbet <corbet@lwn.net> escreveu:
[]
> Sure. It should be easy to make the third argument optional, although
> the regex will be a little more harder to understand.
> 
> Something like this should do the trick:
> 
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
[]
> @@ -466,12 +466,16 @@ while ($ARGV[0] =~ m/^--?(.*)/) {
>  	$show_not_found = 1;  # A no-op but don't fail
>      } elsif ($cmd eq "sphinx-version") {
>  	my $ver_string = shift @ARGV;
> -	if ($ver_string =~ m/^(\d+)\.(\d+)\.(\d+)/) {
> +	if ($ver_string =~ m/^(\d+)\.(\d+)(?:\.?(\d+)?)/) {

trivia: perhaps more readable as

	if ($ver_string =~ m/^(\d+)\.(\d+)(\.\d+)?/) {
	    $sphinx_major = $1;
	    $sphinx_minor = $2;
	    if (defined($3)) {
		$sphinx_patch = substr($3,1);
	    } else {
		$sphinx_patch = 0;
	    }

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Jonathan Corbet]

On Tue, 6 Oct 2020 08:42:07 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> As right now we don't support Sphinx version 3.0[1], we're actually using just
> $sphinx_major. So, I'm wonder if it would make sense to also make <minor>
> optional.

Maybe...someday we may need it, knowing how the Sphinx folks approach
compatibility, but I guess we can always add it then if so.

> The change would be trivial, although the regex will become even more
> harder to read ;-)

	^(\d+)(\.(\d+)){,2}

?  (untested, of course)

> [1] not sure how valuable would be adding support for Sphinx 3.0. While
> I didn't make any tests, I'm strongly suspecting that, with the approach
> we took for backward/forward compatibility, adding support for it
> would mean to just do a trivial change at cdomain.py by applying a
> patch that Markus did replacing a regex function that doesn't exist
> anymore at Sphinx API and emulating C namespace with the logic I
> already implemented. 

3.0 might just be skippable at this point, methinks.  

jon

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Joe Perches]

On Tue, 2020-10-06 at 08:01 -0600, Jonathan Corbet wrote:
> On Tue, 6 Oct 2020 08:42:07 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > As right now we don't support Sphinx version 3.0[1], we're actually using just
> > $sphinx_major. So, I'm wonder if it would make sense to also make <minor>
> > optional.
> 
> Maybe...someday we may need it, knowing how the Sphinx folks approach
> compatibility, but I guess we can always add it then if so.
> 
> > The change would be trivial, although the regex will become even more
> > harder to read ;-)
> 
> 	^(\d+)(\.(\d+)){,2}
> 
> ?  (untested, of course)

This doesn't work as capture groups don't nest using {m, n}

Re: [PATCH] scripts: kernel-doc: allow passing desired Sphinx C domain dialect

[Mauro Carvalho Chehab]

Em Tue, 6 Oct 2020 08:01:34 -0600
Jonathan Corbet <corbet@lwn.net> escreveu:

> On Tue, 6 Oct 2020 08:42:07 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > As right now we don't support Sphinx version 3.0[1], we're actually using just
> > $sphinx_major. So, I'm wonder if it would make sense to also make <minor>
> > optional.  
> 
> Maybe...someday we may need it, knowing how the Sphinx folks approach
> compatibility, but I guess we can always add it then if so.
> 
> > The change would be trivial, although the regex will become even more
> > harder to read ;-)  
> 
> 	^(\d+)(\.(\d+)){,2}
> 
> ?  (untested, of course)

Didn't work (perl complains about its syntax), and neither:

	^(\d+)(\.(\d+)){0,2}

I also tried this:

	^(\d+)(?:\.(\d+)){0,2}

But both ends misplacing the second group information when
x.y.z is used.

On the tests I did, this is the one that worked fine:

	if ($ver_string =~ m/^(\d+)(?:\.(\d+))?(?:\.(\d+))?/) {
	    $sphinx_major = $1;
	    if (defined($2)) {
		$sphinx_minor = $2;
	    } else {
		$sphinx_minor = 0;
	    }
	    if (defined($3)) {
		$sphinx_patch = $3
	    } else {
		$sphinx_patch = 0;
	    }

Or the alternative one, using substrings:

	if ($ver_string =~ m/^(\d+)(\.\d+)?(\.\d+)?/) {
	    $sphinx_major = $1;
	    if (defined($2)) {
		$sphinx_minor = substr($2,1);
	    } else {
		$sphinx_minor = 0;
	    }
	    if (defined($3)) {
		$sphinx_patch = substr($3,1)
	    } else {
		$sphinx_patch = 0;
	    }
	}

I'll keep the last one, as it is likely simpler to understand.

> 
> > [1] not sure how valuable would be adding support for Sphinx 3.0. While
> > I didn't make any tests, I'm strongly suspecting that, with the approach
> > we took for backward/forward compatibility, adding support for it
> > would mean to just do a trivial change at cdomain.py by applying a
> > patch that Markus did replacing a regex function that doesn't exist
> > anymore at Sphinx API and emulating C namespace with the logic I
> > already implemented.   
> 
> 3.0 might just be skippable at this point, methinks.  

Yeah, agreed. 

I'll fold the latest version of this patch with the following diff.

Thanks,
Mauro

index 297312824d26..c8f6b11d5da1 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -466,11 +466,15 @@ while ($ARGV[0] =~ m/^--?(.*)/) {
 	$show_not_found = 1;  # A no-op but don't fail
     } elsif ($cmd eq "sphinx-version") {
 	my $ver_string = shift @ARGV;
-	if ($ver_string =~ m/^(\d+)\.(\d+)(\.\d+)?/) {
+	if ($ver_string =~ m/^(\d+)(\.\d+)?(\.\d+)?/) {
 	    $sphinx_major = $1;
-	    $sphinx_minor = $2;
+	    if (defined($2)) {
+		$sphinx_minor = substr($2,1);
+	    } else {
+		$sphinx_minor = 0;
+	    }
 	    if (defined($3)) {
-		$sphinx_patch = substr($3,1);
+		$sphinx_patch = substr($3,1)
 	    } else {
 		$sphinx_patch = 0;
 	    }
@@ -2368,7 +2372,10 @@ sub process_file($) {
 }
 
 
-get_sphinx_version() if (!$sphinx_major);
+if ($output_mode eq "rst") {
+	get_sphinx_version() if (!$sphinx_major);
+}
+
 $kernelversion = get_kernel_version();
 
 # generate a sequence of code that will splice in highlighting information