[PATCH 0/5] More changes for sphinx-pre-install script

[PATCH 0/5] More changes for sphinx-pre-install script

[Mauro Carvalho Chehab]

Hi Jon,

As asked, I changed the sphinx-pre-install script to print a different message
for the PDF minimal recommended version. This change itself was easy,
but, while testing the patch, I noticed some new weird behaviors when python
venv is used.

Basically, when using python venv, the venv environment will contain python
itself. So, an attempt to create a new virtual environment to upgrade a version
fails (at least here with Fedora 31). As I didn't notice this behavior before,
maybe the problem was due to some Fedora upgrade.

In any case, the approach I took should be generic enough to work past eventual
distro packaging differences.

-

At the end,  instead of a single patch, I ended needing to fix some other stuff, 
for this to work better. Oh well...

The good news is that, at the cost of a slicely more complex logic, the script
should now detect if a virtual environment works and to recommend activating
a newer environment if it exists (instead of recommending to reinstall a
venv using the name of an already-existing directory).

Mauro Carvalho Chehab (5):
  scripts: sphinx-pre-install: only ask to activate valid venvs
  scripts: sphinx-pre-install: change the warning for version < 2.4.4
  scripts: sphinx-pre-install: change recommendation text if venv exists
  scripts: sphinx-pre-install: fix a bug when using with venv
  scripts: sphinx-pre-install: change the output order

 scripts/sphinx-pre-install | 118 ++++++++++++++++++++++++++++---------
 1 file changed, 91 insertions(+), 27 deletions(-)

-- 
2.25.2

[PATCH 1/5] scripts: sphinx-pre-install: only ask to activate valid venvs

[Mauro Carvalho Chehab]

If a venv doesn't contain Sphinx, or has an older Sphinx
version, ignore it.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/sphinx-pre-install | 53 ++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index d4dfe1e59989..249edb3932af 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -231,6 +231,27 @@ sub get_sphinx_fname()
 	return "";
 }
 
+sub get_sphinx_version($)
+{
+	my $cmd = shift;
+	my $ver;
+
+	open IN, "$cmd --version 2>&1 |";
+	while (<IN>) {
+		if (m/^\s*sphinx-build\s+([\d\.]+)(\+\/[\da-f]+)?$/) {
+			$ver=$1;
+			last;
+		}
+		# Sphinx 1.2.x uses a different format
+		if (m/^\s*Sphinx.*\s+([\d\.]+)$/) {
+			$ver=$1;
+			last;
+		}
+	}
+	close IN;
+	return $ver;
+}
+
 sub check_sphinx()
 {
 	my $rec_version;
@@ -266,19 +287,8 @@ sub check_sphinx()
 		return;
 	}
 
-	open IN, "$sphinx --version 2>&1 |" or die "$sphinx returned an error";
-	while (<IN>) {
-		if (m/^\s*sphinx-build\s+([\d\.]+)(\+\/[\da-f]+)?$/) {
-			$cur_version=$1;
-			last;
-		}
-		# Sphinx 1.2.x uses a different format
-		if (m/^\s*Sphinx.*\s+([\d\.]+)$/) {
-			$cur_version=$1;
-			last;
-		}
-	}
-	close IN;
+	$cur_version = get_sphinx_version($sphinx);
+	die ("$sphinx returned an error") if (!$cur_version);
 
 	die "$sphinx didn't return its version" if (!$cur_version);
 
@@ -765,10 +775,21 @@ sub check_needs()
 		my @activates = glob "$ENV{'PWD'}/${virtenv_prefix}*/bin/activate";
 
 		@activates = sort {$b cmp $a} @activates;
+		my ($activate, $ver);
+		foreach my $f (@activates) {
+			$activate = $f;
+			next if ($activate lt $min_activate);
 
-		if ($need_sphinx && scalar @activates > 0 && $activates[0] ge $min_activate) {
-			printf "\nNeed to activate a compatible Sphinx version on virtualenv with:\n";
-			printf "\t. $activates[0]\n";
+			my $sphinx_cmd = $activate;
+			$sphinx_cmd =~ s/activate/sphinx-build/;
+			next if (! -f $sphinx_cmd);
+
+			$ver = get_sphinx_version($sphinx_cmd);
+			last if ($ver ge $min_version);
+		}
+		if ($need_sphinx && ($activate ne "")) {
+			printf "\nNeed to activate Sphinx (version $ver) on virtualenv with:\n";
+			printf "\t. $activate\n";
 			deactivate_help();
 			exit (1);
 		} else {
-- 
2.25.2

[PATCH 2/5] scripts: sphinx-pre-install: change the warning for version < 2.4.4

[Mauro Carvalho Chehab]

As requested by Jon, change the version check, in order to not
emit a warning if version is >= 1.7.9, but below 2.4.4.

After this patch, if someone used an older version, it will
say:

	./scripts/sphinx-pre-install
	Sphinx version 1.7.9
	Note: It is recommended at least Sphinx version 2.4.4 if you need PDF support.
	Detected OS: Fedora release 31 (Thirty One).

	To upgrade Sphinx, use:

		/devel/v4l/docs/sphinx_1.7.9/bin/python3 -m venv sphinx_2.4.4
		. sphinx_2.4.4/bin/activate
		pip install -r ./Documentation/sphinx/requirements.txt

	If you want to exit the virtualenv, you can use:
		deactivate

	All optional dependencies are met.
	Needed package dependencies are met.

If Sphinx is not detected at all, it

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/sphinx-pre-install | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index 249edb3932af..0d5684c08bbc 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -29,6 +29,8 @@ my $install = "";
 my $virtenv_dir = "";
 my $python_cmd = "";
 my $min_version;
+my $rec_version = "1.7.9";	# PDF won't build here
+my $min_pdf_version = "2.4.4";	# Min version where pdf builds
 
 #
 # Command line arguments
@@ -254,7 +256,7 @@ sub get_sphinx_version($)
 
 sub check_sphinx()
 {
-	my $rec_version;
+	my $default_version;
 	my $cur_version;
 
 	open IN, $conf or die "Can't open $conf";
@@ -271,15 +273,15 @@ sub check_sphinx()
 	open IN, $requirement_file or die "Can't open $requirement_file";
 	while (<IN>) {
 		if (m/^\s*Sphinx\s*==\s*([\d\.]+)$/) {
-			$rec_version=$1;
+			$default_version=$1;
 			last;
 		}
 	}
 	close IN;
 
-	die "Can't get recommended sphinx version from $requirement_file" if (!$min_version);
+	die "Can't get default sphinx version from $requirement_file" if (!$default_version);
 
-	$virtenv_dir = $virtenv_prefix . $rec_version;
+	$virtenv_dir = $virtenv_prefix . $default_version;
 
 	my $sphinx = get_sphinx_fname();
 	if ($sphinx eq "") {
@@ -294,7 +296,7 @@ sub check_sphinx()
 
 	if ($cur_version lt $min_version) {
 		printf "ERROR: Sphinx version is %s. It should be >= %s (recommended >= %s)\n",
-		       $cur_version, $min_version, $rec_version;;
+		       $cur_version, $min_version, $default_version;
 		$need_sphinx = 1;
 		return;
 	}
@@ -302,6 +304,13 @@ sub check_sphinx()
 	if ($cur_version lt $rec_version) {
 		printf "Sphinx version %s\n", $cur_version;
 		print "Warning: It is recommended at least Sphinx version $rec_version.\n";
+		print "         If you want pdf, you need at least $min_pdf_version.\n";
+		$rec_sphinx_upgrade = 1;
+		return;
+	}
+	if ($cur_version lt $min_pdf_version) {
+		printf "Sphinx version %s\n", $cur_version;
+		print "Note: It is recommended at least Sphinx version $min_pdf_version if you need PDF support.\n";
 		$rec_sphinx_upgrade = 1;
 		return;
 	}
-- 
2.25.2

[PATCH 3/5] scripts: sphinx-pre-install: change recommendation text if venv exists

[Mauro Carvalho Chehab]

If one is running a Sphinx version older than what's recommended,
but there's already a newer working virtual env, change the
text, as it is just a matter of switching to the new venv, instead
of creating a new one from scratch.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/sphinx-pre-install | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index 0d5684c08bbc..938b65d40fc8 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -29,6 +29,7 @@ my $install = "";
 my $virtenv_dir = "";
 my $python_cmd = "";
 my $min_version;
+my $cur_version;
 my $rec_version = "1.7.9";	# PDF won't build here
 my $min_pdf_version = "2.4.4";	# Min version where pdf builds
 
@@ -257,7 +258,6 @@ sub get_sphinx_version($)
 sub check_sphinx()
 {
 	my $default_version;
-	my $cur_version;
 
 	open IN, $conf or die "Can't open $conf";
 	while (<IN>) {
@@ -703,8 +703,6 @@ sub check_needs()
 		print "Unknown OS\n\n";
 	}
 
-	print "To upgrade Sphinx, use:\n\n" if ($rec_sphinx_upgrade);
-
 	# Check python command line, trying first python3
 	$python_cmd = findprog("python3");
 	$python_cmd = check_program("python", 0) if (!$python_cmd);
@@ -786,24 +784,36 @@ sub check_needs()
 		@activates = sort {$b cmp $a} @activates;
 		my ($activate, $ver);
 		foreach my $f (@activates) {
-			$activate = $f;
-			next if ($activate lt $min_activate);
+			next if ($f lt $min_activate);
 
-			my $sphinx_cmd = $activate;
+			my $sphinx_cmd = $f;
 			$sphinx_cmd =~ s/activate/sphinx-build/;
 			next if (! -f $sphinx_cmd);
 
 			$ver = get_sphinx_version($sphinx_cmd);
-			last if ($ver ge $min_version);
+			if ($need_sphinx && ($ver ge $min_version)) {
+				$activate = $f;
+				last;
+			} elsif ($ver gt $cur_version) {
+				$activate = $f;
+				last;
+			}
 		}
-		if ($need_sphinx && ($activate ne "")) {
-			printf "\nNeed to activate Sphinx (version $ver) on virtualenv with:\n";
-			printf "\t. $activate\n";
-			deactivate_help();
-			exit (1);
+		if ($activate ne "") {
+			if ($need_sphinx) {
+				printf "\nNeed to activate Sphinx (version $ver) on virtualenv with:\n";
+				printf "\t. $activate\n";
+				deactivate_help();
+				exit (1);
+			} else {
+				printf "\nYou may also use a newer Sphinx (version $ver) with:\n";
+				printf "\tdeactivate && . $activate\n";
+			}
 		} else {
 			my $rec_activate = "$virtenv_dir/bin/activate";
 
+			print "To upgrade Sphinx, use:\n\n" if ($rec_sphinx_upgrade);
+
 			if ($need_venv) {
 				printf "\t$python_cmd -m venv $virtenv_dir\n";
 			} else {
-- 
2.25.2

[PATCH 4/5] scripts: sphinx-pre-install: fix a bug when using with venv

[Mauro Carvalho Chehab]

When python3 creates a venv, it adds python into it!

This causes any upgrade recommendation to look like this:

	/devel/v4l/docs/sphinx_1.7.9/bin/python3 -m venv sphinx_2.4.4
	. sphinx_2.4.4/bin/activate
	pip install -r ./Documentation/sphinx/requirements.txt

With is wrong (and it may not work). So, when recomending
an upgrade, exclude the venv dir from the search path, and
get the system's python.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/sphinx-pre-install | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index 938b65d40fc8..987aebf7e3a0 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -148,6 +148,24 @@ sub findprog($)
 	}
 }
 
+sub find_python_no_venv()
+{
+	my $prog = shift;
+
+	my $cur_dir = qx(pwd);
+	$cur_dir =~ s/\s+$//;
+
+	foreach my $dir (split(/:/, $ENV{PATH})) {
+		next if ($dir =~ m,($cur_dir)/sphinx,);
+		return "$dir/python3" if(-x "$dir/python3");
+	}
+	foreach my $dir (split(/:/, $ENV{PATH})) {
+		next if ($dir =~ m,($cur_dir)/sphinx,);
+		return "$dir/python" if(-x "$dir/python");
+	}
+	return "python";
+}
+
 sub check_program($$)
 {
 	my $prog = shift;
@@ -814,6 +832,8 @@ sub check_needs()
 
 			print "To upgrade Sphinx, use:\n\n" if ($rec_sphinx_upgrade);
 
+			$python_cmd = find_python_no_venv();
+
 			if ($need_venv) {
 				printf "\t$python_cmd -m venv $virtenv_dir\n";
 			} else {
-- 
2.25.2

[PATCH 5/5] scripts: sphinx-pre-install: change the output order

[Mauro Carvalho Chehab]

When the script detects the need for an upgrade, it will
print either a warning or a note.

Let's change a little bit the order where messages will be
displayed, in order to make easier for the user to identify
the more important messages.

It should now be like this:

	Detected OS: Fedora release 31 (Thirty One).
	Sphinx version: 1.7.9

	Note: It is recommended at least Sphinx version 2.4.4 if you need PDF support.
	To upgrade Sphinx, use:

		/usr/bin/python3 -m venv sphinx_2.4.4
		. sphinx_2.4.4/bin/activate
		pip install -r ./Documentation/sphinx/requirements.txt

	If you want to exit the virtualenv, you can use:
		deactivate

All optional dependencies are met.
Needed package dependencies are met.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/sphinx-pre-install | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index 987aebf7e3a0..2c7a4f9594cd 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -321,14 +321,11 @@ sub check_sphinx()
 
 	if ($cur_version lt $rec_version) {
 		printf "Sphinx version %s\n", $cur_version;
-		print "Warning: It is recommended at least Sphinx version $rec_version.\n";
-		print "         If you want pdf, you need at least $min_pdf_version.\n";
 		$rec_sphinx_upgrade = 1;
 		return;
 	}
 	if ($cur_version lt $min_pdf_version) {
 		printf "Sphinx version %s\n", $cur_version;
-		print "Note: It is recommended at least Sphinx version $min_pdf_version if you need PDF support.\n";
 		$rec_sphinx_upgrade = 1;
 		return;
 	}
@@ -799,6 +796,13 @@ sub check_needs()
 		my $min_activate = "$ENV{'PWD'}/${virtenv_prefix}${min_version}/bin/activate";
 		my @activates = glob "$ENV{'PWD'}/${virtenv_prefix}*/bin/activate";
 
+		if ($cur_version lt $rec_version) {
+			print "Warning: It is recommended at least Sphinx version $rec_version.\n";
+			print "         If you want pdf, you need at least $min_pdf_version.\n";
+		}
+		if ($cur_version lt $min_pdf_version) {
+			print "Note: It is recommended at least Sphinx version $min_pdf_version if you need PDF support.\n";
+		}
 		@activates = sort {$b cmp $a} @activates;
 		my ($activate, $ver);
 		foreach my $f (@activates) {
-- 
2.25.2

[PATCH v1.1 5/5] scripts: sphinx-pre-install: change the output order

[Mauro Carvalho Chehab]

When the script detects the need for an upgrade, it will
print either a warning or a note.

Let's change a little bit the order where messages will be
displayed, in order to make easier for the user to identify
the more important messages.

It should now be like this:

	Detected OS: Fedora release 31 (Thirty One).
	Sphinx version: 1.7.9

	Note: It is recommended at least Sphinx version 2.4.4 if you need PDF support.
	To upgrade Sphinx, use:

		/usr/bin/python3 -m venv sphinx_2.4.4
		. sphinx_2.4.4/bin/activate
		pip install -r ./Documentation/sphinx/requirements.txt

	If you want to exit the virtualenv, you can use:
		deactivate

All optional dependencies are met.
Needed package dependencies are met.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

v 1.1: I forgot to commit some minor changes to the patch

 scripts/sphinx-pre-install | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index 987aebf7e3a0..c680c3efb176 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -320,15 +320,10 @@ sub check_sphinx()
 	}
 
 	if ($cur_version lt $rec_version) {
-		printf "Sphinx version %s\n", $cur_version;
-		print "Warning: It is recommended at least Sphinx version $rec_version.\n";
-		print "         If you want pdf, you need at least $min_pdf_version.\n";
 		$rec_sphinx_upgrade = 1;
 		return;
 	}
 	if ($cur_version lt $min_pdf_version) {
-		printf "Sphinx version %s\n", $cur_version;
-		print "Note: It is recommended at least Sphinx version $min_pdf_version if you need PDF support.\n";
 		$rec_sphinx_upgrade = 1;
 		return;
 	}
@@ -716,10 +711,11 @@ sub check_needs()
 	check_sphinx();
 
 	if ($system_release) {
-		print "Detected OS: $system_release.\n\n";
+		print "Detected OS: $system_release.\n";
 	} else {
-		print "Unknown OS\n\n";
+		print "Unknown OS\n";
 	}
+	printf "Sphinx version: %s\n\n", $cur_version if ($cur_version);
 
 	# Check python command line, trying first python3
 	$python_cmd = findprog("python3");
@@ -799,6 +795,13 @@ sub check_needs()
 		my $min_activate = "$ENV{'PWD'}/${virtenv_prefix}${min_version}/bin/activate";
 		my @activates = glob "$ENV{'PWD'}/${virtenv_prefix}*/bin/activate";
 
+		if ($cur_version lt $rec_version) {
+			print "Warning: It is recommended at least Sphinx version $rec_version.\n";
+			print "         If you want pdf, you need at least $min_pdf_version.\n";
+		}
+		if ($cur_version lt $min_pdf_version) {
+			print "Note: It is recommended at least Sphinx version $min_pdf_version if you need PDF support.\n";
+		}
 		@activates = sort {$b cmp $a} @activates;
 		my ($activate, $ver);
 		foreach my $f (@activates) {
-- 
2.25.2

Re: [PATCH 0/5] More changes for sphinx-pre-install script

[Jonathan Corbet]

On Tue, 21 Apr 2020 16:31:04 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> As asked, I changed the sphinx-pre-install script to print a different message
> for the PDF minimal recommended version. This change itself was easy,
> but, while testing the patch, I noticed some new weird behaviors when python
> venv is used.
> 
> Basically, when using python venv, the venv environment will contain python
> itself. So, an attempt to create a new virtual environment to upgrade a version
> fails (at least here with Fedora 31). As I didn't notice this behavior before,
> maybe the problem was due to some Fedora upgrade.
> 
> In any case, the approach I took should be generic enough to work past eventual
> distro packaging differences.
> 
> -
> 
> At the end,  instead of a single patch, I ended needing to fix some other stuff, 
> for this to work better. Oh well...
> 
> The good news is that, at the cost of a slicely more complex logic, the script
> should now detect if a virtual environment works and to recommend activating
> a newer environment if it exists (instead of recommending to reinstall a
> venv using the name of an already-existing directory).
> 
> Mauro Carvalho Chehab (5):
>   scripts: sphinx-pre-install: only ask to activate valid venvs
>   scripts: sphinx-pre-install: change the warning for version < 2.4.4
>   scripts: sphinx-pre-install: change recommendation text if venv exists
>   scripts: sphinx-pre-install: fix a bug when using with venv
>   scripts: sphinx-pre-install: change the output order

Series applied, thanks.

jon