[PATCH 0/2] Fix some issues at scripts/sphinx-pre-install

[PATCH 0/2] Fix some issues at scripts/sphinx-pre-install

[Mauro Carvalho Chehab]

Currently, when using with --no-virtualenv, the script doesn't behave well,
as it prints both instructions to install Sphinx via distribution's package
manager and via pip.

Address it.

While here, rework the logic which recommends Sphinx install, by
splitting it into three parts and making easier to maintain it, as
there are too much complexity there. Splitting the Sphinx part
of the logic on 3 separate functions allow to detect if the venv/virtualenv
python packages are needed or not, and helps to have a cleaner
logic.

Mauro Carvalho Chehab (2):
  scripts: sphinx-pre-install: rework the sphinx install logic
  scripts: sphinx-pre-install: fix the need of virtenv packages

 scripts/sphinx-pre-install | 262 +++++++++++++++++++++++++------------
 1 file changed, 180 insertions(+), 82 deletions(-)

-- 
2.31.1

[PATCH 1/2] scripts: sphinx-pre-install: rework the sphinx install logic

[Mauro Carvalho Chehab]

The sphinx-pre-install supports installing sphinx via a virtual
environment using pip/pypi or directly from the distribution's
package, when --no-virtualenv is used.

However, even when --no-virtualenv, the current logic is
still recomending to install a virtual env, due to a regression.

It turns that the logic there is complex, as it depends on
several different conditions.

Split the code which recommends Sphinx on two separate
functions, in order to clean up the code.

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

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index fe92020d67e3..b5fec149f473 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -24,7 +24,6 @@ my $need_symlink = 0;
 my $need_sphinx = 0;
 my $need_venv = 0;
 my $need_virtualenv = 0;
-my $rec_sphinx_upgrade = 0;
 my $install = "";
 my $virtenv_dir = "";
 my $python_cmd = "";
@@ -33,6 +32,7 @@ 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
 
+
 #
 # Command line arguments
 #
@@ -319,10 +319,7 @@ sub check_sphinx()
 		return;
 	}
 
-	if ($cur_version lt $rec_version) {
-		$rec_sphinx_upgrade = 1;
-		return;
-	}
+	return if ($cur_version lt $rec_version);
 
 	# On version check mode, just assume Sphinx has all mandatory deps
 	exit (0) if ($version_check);
@@ -701,6 +698,141 @@ sub deactivate_help()
 	printf "\tdeactivate\n";
 }
 
+sub get_virtenv()
+{
+	my $min_activate = "$ENV{'PWD'}/${virtenv_prefix}${min_version}/bin/activate";
+	my @activates = glob "$ENV{'PWD'}/${virtenv_prefix}*/bin/activate";
+
+	@activates = sort {$b cmp $a} @activates;
+	my ($activate, $ver);
+	foreach my $f (@activates) {
+		next if ($f lt $min_activate);
+
+		my $sphinx_cmd = $f;
+		$sphinx_cmd =~ s/activate/sphinx-build/;
+		next if (! -f $sphinx_cmd);
+
+		my $ver = get_sphinx_version($sphinx_cmd);
+		if ($need_sphinx && ($ver ge $min_version)) {
+			return ($f, $ver);
+		} elsif ($ver gt $cur_version) {
+			return ($f, $ver);
+		}
+	}
+	return ("", "");
+}
+
+#
+# The logic here is complex, as it have to deal with different versions:
+#	- minimal supported version;
+#	- minimal PDF version;
+#	- recommended version.
+# It also needs to work fine with both distro's package and venv/virtualenv
+sub recommend_sphinx_version($)
+{
+	my $virtualenv_cmd = shift;
+
+	# Avoid running sphinx-builds from venv if $cur_version is good
+	return if ($cur_version && ($cur_version ge $rec_version));
+
+	my $latest_avail_ver;
+	my $rec_sphinx_upgrade = 0;
+
+	# Get the highest version from sphinx_*/bin/sphinx-build and the
+	# corresponding command to activate the venv/virtenv
+	my ($activate, $venv_ver) = get_virtenv();
+
+	if (($activate ne "") && ($venv_ver gt $cur_version)) {
+		$latest_avail_ver = $venv_ver;
+	} else {
+		$latest_avail_ver = $cur_version if ($cur_version);
+	}
+
+	if (!$need_sphinx) {
+		# sphinx-build is present and its version is >= $min_version
+
+		#only recommend enabling a newer virtenv version if makes sense.
+		if ($latest_avail_ver gt $cur_version) {
+			printf "\nYou may also use the newer Sphinx version $venv_ver with:\n";
+			printf "\tdeactivate\n"  if ($ENV{'PWD'} =~ /${virtenv_prefix}/);
+			printf "\t. $activate\n";
+			deactivate_help();
+
+			return;
+		}
+		return if ($latest_avail_ver ge $rec_version);
+	}
+
+	if (!$virtualenv) {
+		# No sphinx either via package or via virtenv. As we can't
+		# Compare the versions here, just return, recommending the
+		# user to install it from the package distro.
+		return if (!$latest_avail_ver);
+
+		# User doesn't want a virtenv recommendation, but he already
+		# installed one via virtenv with a newer version.
+		# So, print commands to enable it
+		if ($latest_avail_ver gt $cur_version) {
+			printf "\nYou may also use the Sphinx virtualenv version $venv_ver with:\n";
+			printf "\tdeactivate\n"  if ($ENV{'PWD'} =~ /${virtenv_prefix}/);
+			printf "\t. $activate\n";
+			deactivate_help();
+
+			return;
+		}
+		print "\n";
+	} else {
+		$need++ if ($need_sphinx);
+	}
+
+	# Suggest newer versions if current ones are too old
+	if ($latest_avail_ver && $cur_version ge $min_version) {
+		# If there's a good enough version, ask the user to enable it
+		if ($latest_avail_ver ge $rec_version) {
+			printf "\nNeed to activate Sphinx (version $venv_ver) on virtualenv with:\n";
+			printf "\t. $activate\n";
+			deactivate_help();
+
+			return;
+		}
+
+		# Version is above the minimal required one, but may be
+		# below the recommended one. So, print warnings/notes
+
+		if ($latest_avail_ver lt $rec_version) {
+			print "Warning: It is recommended at least Sphinx version $rec_version.\n";
+			$rec_sphinx_upgrade = 1;
+		}
+		if ($latest_avail_ver lt $min_pdf_version) {
+			print "note: If you want pdf, you need at least $min_pdf_version.\n";
+		}
+	}
+
+	# At this point, either it needs Sphinx or upgrade is recommended,
+	# both via pip
+
+	if ($rec_sphinx_upgrade) {
+		if (!$virtualenv) {
+			print "Instead of install/upgrade Python Sphinx pkg, you could use pip/pypi with:\n\n";
+		} else {
+			print "To upgrade Sphinx, use:\n\n";
+		}
+	} else {
+		print "Sphinx needs to be installed either as a package or via pip/pypi with:\n";
+	}
+
+	$python_cmd = find_python_no_venv();
+
+	if ($need_venv) {
+		printf "\t$python_cmd -m venv $virtenv_dir\n";
+	} else {
+		printf "\t$virtualenv_cmd $virtenv_dir\n";
+	}
+	printf "\t. $virtenv_dir/bin/activate\n";
+	printf "\tpip install -r $requirement_file\n";
+	deactivate_help();
+}
+
 sub check_needs()
 {
 	# Check if Sphinx is already accessible from current environment
@@ -763,8 +895,8 @@ sub check_needs()
 	check_program("rsvg-convert", 2) if ($pdf);
 	check_program("latexmk", 2) if ($pdf);
 
-	if ($need_sphinx || $rec_sphinx_upgrade) {
-		check_python_module("ensurepip", 0) if ($need_venv);
+	if ($need_venv) {
+		check_python_module("ensurepip", 0);
 	}
 
 	# Do distro-specific checks and output distro-install commands
@@ -784,67 +916,7 @@ sub check_needs()
 		       which("sphinx-build-3");
 	}
 
-	# NOTE: if the system has a too old Sphinx version installed,
-	# it will recommend installing a newer version using virtualenv
-
-	if ($need_sphinx || $rec_sphinx_upgrade) {
-		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) {
-			next if ($f lt $min_activate);
-
-			my $sphinx_cmd = $f;
-			$sphinx_cmd =~ s/activate/sphinx-build/;
-			next if (! -f $sphinx_cmd);
-
-			$ver = get_sphinx_version($sphinx_cmd);
-			if ($need_sphinx && ($ver ge $min_version)) {
-				$activate = $f;
-				last;
-			} elsif ($ver gt $cur_version) {
-				$activate = $f;
-				last;
-			}
-		}
-		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);
-
-			$python_cmd = find_python_no_venv();
-
-			if ($need_venv) {
-				printf "\t$python_cmd -m venv $virtenv_dir\n";
-			} else {
-				printf "\t$virtualenv_cmd $virtenv_dir\n";
-			}
-			printf "\t. $rec_activate\n";
-			printf "\tpip install -r $requirement_file\n";
-			deactivate_help();
-
-			$need++ if (!$rec_sphinx_upgrade);
-		}
-	}
+	recommend_sphinx_version($virtualenv_cmd);
 	printf "\n";
 
 	print "All optional dependencies are met.\n" if (!$optional);
-- 
2.31.1

[PATCH 2/2] scripts: sphinx-pre-install: fix the need of virtenv packages

[Mauro Carvalho Chehab]

The pip packages are only needed when the distro-provided
Sphinx version is not good enough.

Don't recommend installing it if not needed.

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

diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
index b5fec149f473..288e86a9d1e5 100755
--- a/scripts/sphinx-pre-install
+++ b/scripts/sphinx-pre-install
@@ -22,16 +22,18 @@ my $need = 0;
 my $optional = 0;
 my $need_symlink = 0;
 my $need_sphinx = 0;
-my $need_venv = 0;
+my $need_pip = 0;
 my $need_virtualenv = 0;
+my $rec_sphinx_upgrade = 0;
 my $install = "";
 my $virtenv_dir = "";
 my $python_cmd = "";
+my $activate_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
-
+my $latest_avail_ver;
 
 #
 # Command line arguments
@@ -700,11 +702,12 @@ sub deactivate_help()
 
 sub get_virtenv()
 {
+	my $ver;
 	my $min_activate = "$ENV{'PWD'}/${virtenv_prefix}${min_version}/bin/activate";
 	my @activates = glob "$ENV{'PWD'}/${virtenv_prefix}*/bin/activate";
 
 	@activates = sort {$b cmp $a} @activates;
-	my ($activate, $ver);
+
 	foreach my $f (@activates) {
 		next if ($f lt $min_activate);
 
@@ -722,6 +725,42 @@ sub get_virtenv()
 	return ("", "");
 }
 
+sub recommend_sphinx_upgrade()
+{
+	my $venv_ver;
+
+	# Avoid running sphinx-builds from venv if $cur_version is good
+	if ($cur_version && ($cur_version ge $rec_version)) {
+		$latest_avail_ver = $cur_version;
+		return;
+	}
+
+	# Get the highest version from sphinx_*/bin/sphinx-build and the
+	# corresponding command to activate the venv/virtenv
+	$activate_cmd = get_virtenv();
+
+	# Store the highest version from Sphinx existing virtualenvs
+	if (($activate_cmd ne "") && ($venv_ver gt $cur_version)) {
+		$latest_avail_ver = $venv_ver;
+	} else {
+		$latest_avail_ver = $cur_version if ($cur_version);
+	}
+
+	# As we don't know package version of Sphinx, and there's no
+	# virtual environments, don't check if upgrades are needed
+	if (!$virtualenv) {
+		return if (!$latest_avail_ver);
+	}
+
+	# Either there are already a virtual env or a new one should be created
+	$need_pip = 1;
+
+	# Return if the reason is due to an upgrade or not
+	if ($latest_avail_ver lt $rec_version) {
+		$rec_sphinx_upgrade = 1;
+	}
+}
+
 #
 # The logic here is complex, as it have to deal with different versions:
 #	- minimal supported version;
@@ -732,30 +771,21 @@ sub recommend_sphinx_version($)
 {
 	my $virtualenv_cmd = shift;
 
-	# Avoid running sphinx-builds from venv if $cur_version is good
+	if ($latest_avail_ver lt $min_pdf_version) {
+		print "note: If you want pdf, you need at least Sphinx $min_pdf_version.\n";
+	}
+
+	# Version is OK. Nothing to do.
 	return if ($cur_version && ($cur_version ge $rec_version));
 
-	my $latest_avail_ver;
-	my $rec_sphinx_upgrade = 0;
-
-	# Get the highest version from sphinx_*/bin/sphinx-build and the
-	# corresponding command to activate the venv/virtenv
-	my ($activate, $venv_ver) = get_virtenv();
-
-	if (($activate ne "") && ($venv_ver gt $cur_version)) {
-		$latest_avail_ver = $venv_ver;
-	} else {
-		$latest_avail_ver = $cur_version if ($cur_version);
-	}
-
 	if (!$need_sphinx) {
 		# sphinx-build is present and its version is >= $min_version
 
 		#only recommend enabling a newer virtenv version if makes sense.
 		if ($latest_avail_ver gt $cur_version) {
-			printf "\nYou may also use the newer Sphinx version $venv_ver with:\n";
+			printf "\nYou may also use the newer Sphinx version $latest_avail_ver with:\n";
 			printf "\tdeactivate\n"  if ($ENV{'PWD'} =~ /${virtenv_prefix}/);
-			printf "\t. $activate\n";
+			printf "\t. $activate_cmd\n";
 			deactivate_help();
 
 			return;
@@ -773,9 +803,9 @@ sub recommend_sphinx_version($)
 		# installed one via virtenv with a newer version.
 		# So, print commands to enable it
 		if ($latest_avail_ver gt $cur_version) {
-			printf "\nYou may also use the Sphinx virtualenv version $venv_ver with:\n";
+			printf "\nYou may also use the Sphinx virtualenv version $latest_avail_ver with:\n";
 			printf "\tdeactivate\n"  if ($ENV{'PWD'} =~ /${virtenv_prefix}/);
-			printf "\t. $activate\n";
+			printf "\t. $activate_cmd\n";
 			deactivate_help();
 
 			return;
@@ -789,8 +819,8 @@ sub recommend_sphinx_version($)
 	if ($latest_avail_ver && $cur_version ge $min_version) {
 		# If there's a good enough version, ask the user to enable it
 		if ($latest_avail_ver ge $rec_version) {
-			printf "\nNeed to activate Sphinx (version $venv_ver) on virtualenv with:\n";
-			printf "\t. $activate\n";
+			printf "\nNeed to activate Sphinx (version $latest_avail_ver) on virtualenv with:\n";
+			printf "\t. $activate_cmd\n";
 			deactivate_help();
 
 			return;
@@ -801,10 +831,6 @@ sub recommend_sphinx_version($)
 
 		if ($latest_avail_ver lt $rec_version) {
 			print "Warning: It is recommended at least Sphinx version $rec_version.\n";
-			$rec_sphinx_upgrade = 1;
-		}
-		if ($latest_avail_ver lt $min_pdf_version) {
-			print "note: If you want pdf, you need at least $min_pdf_version.\n";
 		}
 	}
 
@@ -823,11 +849,8 @@ sub recommend_sphinx_version($)
 
 	$python_cmd = find_python_no_venv();
 
-	if ($need_venv) {
-		printf "\t$python_cmd -m venv $virtenv_dir\n";
-	} else {
-		printf "\t$virtualenv_cmd $virtenv_dir\n";
-	}
+	printf "\t$virtualenv_cmd $virtenv_dir\n";
+
 	printf "\t. $virtenv_dir/bin/activate\n";
 	printf "\tpip install -r $requirement_file\n";
 	deactivate_help();
@@ -854,15 +877,14 @@ sub check_needs()
 		if ($virtualenv) {
 			my $tmp = qx($python_cmd --version 2>&1);
 			if ($tmp =~ m/(\d+\.)(\d+\.)/) {
-				if ($1 >= 3 && $2 >= 3) {
-					$need_venv = 1;		# python 3.3 or upper
-				} else {
-					$need_virtualenv = 1;
-				}
 				if ($1 < 3) {
 					# Fail if it finds python2 (or worse)
 					die "Python 3 is required to build the kernel docs\n";
 				}
+				if ($1 == 3 && $2 < 3) {
+					# Need Python 3.3 or upper for venv
+					$need_virtualenv = 1;
+				}
 			} else {
 				die "Warning: couldn't identify $python_cmd version!";
 			}
@@ -871,14 +893,22 @@ sub check_needs()
 		}
 	}
 
-	# Set virtualenv command line, if python < 3.3
+	recommend_sphinx_upgrade();
+
 	my $virtualenv_cmd;
-	if ($need_virtualenv) {
-		$virtualenv_cmd = findprog("virtualenv-3");
-		$virtualenv_cmd = findprog("virtualenv-3.5") if (!$virtualenv_cmd);
-		if (!$virtualenv_cmd) {
-			check_program("virtualenv", 0);
-			$virtualenv_cmd = "virtualenv";
+
+	if ($need_pip) {
+		# Set virtualenv command line, if python < 3.3
+		if ($need_virtualenv) {
+			$virtualenv_cmd = findprog("virtualenv-3");
+			$virtualenv_cmd = findprog("virtualenv-3.5") if (!$virtualenv_cmd);
+			if (!$virtualenv_cmd) {
+				check_program("virtualenv", 0);
+				$virtualenv_cmd = "virtualenv";
+			}
+		} else {
+			$virtualenv_cmd = "$python_cmd -m venv";
+			check_python_module("ensurepip", 0);
 		}
 	}
 
@@ -895,10 +925,6 @@ sub check_needs()
 	check_program("rsvg-convert", 2) if ($pdf);
 	check_program("latexmk", 2) if ($pdf);
 
-	if ($need_venv) {
-		check_python_module("ensurepip", 0);
-	}
-
 	# Do distro-specific checks and output distro-install commands
 	check_distros();
 
-- 
2.31.1

Re: [PATCH 0/2] Fix some issues at scripts/sphinx-pre-install

[Jonathan Corbet]

Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:

> Currently, when using with --no-virtualenv, the script doesn't behave well,
> as it prints both instructions to install Sphinx via distribution's package
> manager and via pip.
>
> Address it.
>
> While here, rework the logic which recommends Sphinx install, by
> splitting it into three parts and making easier to maintain it, as
> there are too much complexity there. Splitting the Sphinx part
> of the logic on 3 separate functions allow to detect if the venv/virtualenv
> python packages are needed or not, and helps to have a cleaner
> logic.
>
> Mauro Carvalho Chehab (2):
>   scripts: sphinx-pre-install: rework the sphinx install logic
>   scripts: sphinx-pre-install: fix the need of virtenv packages
>
>  scripts/sphinx-pre-install | 262 +++++++++++++++++++++++++------------
>  1 file changed, 180 insertions(+), 82 deletions(-)

I've applied this set, thanks.

jon