Re: [PATCH] Add --interactive option to prompt for dependency installation if missing

[Jonathan Corbet]

Thanks for working to make our tooling better.  Various comments...

saivishnu725@xxxxxxxxx writes:

> From: saivishnu725 <saivishnu725@xxxxxxxxx>
>
> Introduce the --interactive flag to enable user prompts before running commands to install missing dependencies.
> Asks if the user would like to run the distro appropriate commands if any dependency is not available.

Please keep changelog text within the 80-column limit.

> Signed-off-by: saivishnu725 <saivishnu725@xxxxxxxxx>

The signoff should have your full name, please.

> ---
>  scripts/sphinx-pre-install | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
> index ad9945ccb0cf..581d694eb0fd 100755
> --- a/scripts/sphinx-pre-install
> +++ b/scripts/sphinx-pre-install
> @@ -42,6 +42,7 @@ my $latest_avail_ver;
>  my $pdf = 1;
>  my $virtualenv = 1;
>  my $version_check = 0;
> +my $interactive = 0;
>  
>  #
>  # List of required texlive packages on Fedora and OpenSuse
> @@ -338,6 +339,21 @@ sub which($)
>  	return undef;
>  }
>  
> +sub run_if_interactive($)
> +{
> +	my $command = shift;
> +
> +	if ($interactive) {
> +		printf("Do you want to run the command now [Y/n]: ");
> +		my $user_input = <STDIN>;
> +		chomp $user_input;
> +		if ($user_input =~ /Y|y/) {
> +			printf("\$ $command\n");
> +			system($command);
> +		}
> +	}
> +}

It seems to me that you might want to check the exit status of the
command and stop if something does not work properly.

>  #
>  # Subroutines that check distro-specific hints
>  #
> @@ -374,7 +390,9 @@ sub give_debian_hints()
>  
>  	return if (!$need && !$optional);
>  	printf("You should run:\n") if ($verbose_warn_install);
> -	printf("\n\tsudo apt-get install $install\n");
> +	my $command = "sudo apt-get install $install";
> +	printf("\n\t$command\n");
> +	run_if_interactive($command);
>  }

It seems you'll print the command twice?

>  sub give_redhat_hints()
> @@ -1002,12 +1020,15 @@ while (@ARGV) {
>  		$pdf = 0;
>  	} elsif ($arg eq "--version-check"){
>  		$version_check = 1;
> +	} elsif ($arg eq "--interactive") {
> +		$interactive = 1;
>  	} else {
> -		print "Usage:\n\t$0 <--no-virtualenv> <--no-pdf> <--version-check>\n\n";
> +		print "Usage:\n\t$0 <--no-virtualenv> <--no-pdf> <--version-check> <--interactive>\n\n";
>  		print "Where:\n";
>  		print "\t--no-virtualenv\t- Recommend installing Sphinx instead of using a virtualenv\n";
>  		print "\t--version-check\t- if version is compatible, don't check for missing dependencies\n";
> -		print "\t--no-pdf\t- don't check for dependencies required to build PDF docs\n\n";
> +		print "\t--no-pdf\t- don't check for dependencies required to build PDF docs\n";
> +		print "\t--interactive\t- Ask to install missing dependencies\n\n";
>  		exit -1;
>  	}
>  }
> -- 
>
> This is not the complete patch - I'm sending this to get early feedback before I go further. If this looks good, I plan to follow up with additional patches that will:
> 1. use the run_if_interactive on the hints for every distro
> 2. add more quality-to-life features to make the script more useful
>
> Any form of feedback would be helpful! If there is a reason why none of the scripts are interactable, please let me know why.

Here too, please stick with the 80-column limit.

When you post an RFC patch like this, it's good to put "RFC" into the
subject line, just to help ensure that it doesn't end up being applied
prematurely.

Overall, I have no objection to the intent of the patch.  Nobody has
asked for this, but that doesn't mean that they wouldn't have found it
useful.  If you want to proceed, I would expect to apply the final
result.

Thanks,

jon