instaweb: make the perl path configurable
PATCH 1/1 instaweb: make the perl path configurable
It is convenient for the user to be able to customize the path to perl if they do not want to use the system perl. This may be the case, for example, if the user wants to use the plackup httpd but its extra dependencies are not installed in the system perl; they can set the perl path to a perl that they install and have control over in their own home directory.
--- Documentation/config.txt | 4 ++++ git-instaweb.sh | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-)
Documentation/config.txt +4 -0
@@ -1549,6 +1549,10 @@ instaweb.modulepath:: instead of /usr/lib/apache2/modules. Only used if httpd is Apache. +instaweb.perlpath:: + The path to the perl executable used by linkgit:git-instaweb[1] to + run gitweb and/or verify that the HTTP daemon is running. + instaweb.port:: The port number to bind the gitweb httpd to. See linkgit:git-instaweb[1].
git-instaweb.sh +3 -1
@@ -3,7 +3,6 @@ # Copyright (c) 2006 Eric Wong # -PERL='@@PERL@@' OPTIONS_KEEPDASHDASH= OPTIONS_SPEC="\ git instaweb [options] (--start | --stop | --restart)
@@ -26,9 +25,12 @@ local="$(git config --bool --get instaweb.local)" httpd="$(git config --get instaweb.httpd)" root="$(git config --get instaweb.gitwebdir)" port=$(git config --get instaweb.port) +perl_path="$(git config --get instaweb.perlpath)" module_path="$(git config --get instaweb.modulepath)" action="browse" +PERL=${perl_path:-@@PERL@@} + conf="$GIT_DIR/gitweb/httpd.conf" # Defaults:
Signature · 1 line
1.8.1.5
Quoted 7 lines · Charles McGarvey <chazmcgarvey <at> brokenzipper.com> writes:
It is convenient for the user to be able to customize the path to perl if they do not want to use the system perl. This may be the case, for example, if the user wants to use the plackup httpd but its extra dependencies are not installed in the system perl; they can set the perl path to a perl that they install and have control over in their own home directory.
Signed-off-by Charles McGarvey <chazmcgarvey <at> brokenzipper.com>
Is it really necessary? There is always PERL5LIB if one wants to use Perl modules installed in one's own home directory. If one is using local::lib one has it "for free".
If they do not want to use system perl there is always perlbrew.
Quoted 17 lines
--- Documentation/config.txt | 4 ++++ git-instaweb.sh | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-)Documentation/config.txt +4
-0<at> <at> -1549,6 +1549,10 <at> <at> instaweb.modulepath:: instead of /usr/lib/apache2/modules. Only used if httpd is Apache. +instaweb.perlpath:: + The path to the perl executable used by linkgit:git-instaweb[1] to + run gitweb and/or verify that the HTTP daemon is running. +
[...]
Signature · 2 lines
Jakub Narębski (via GMane)
Quoted 11 lines · Jakub Narebski
Quoted 5 lines · Charles McGarvey <chazmcgarvey <at> brokenzipper.com> writes:
It is convenient for the user to be able to customize the path to perl if they do not want to use the system perl. This may be the case, for example, if the user wants to use the plackup httpd but its extra dependencies are not installed in the system perl; they can set the perl path to a perl that they install and have control over in their own home directory.
Is it really necessary? There is always PERL5LIB if one wants to use Perl modules installed in one's own home directory. If one is using
local::libone has it "for free".
Yes, that's right. Using PERL5LIB would solve the example problem in the commit message, and it would even be pretty simple to set up using local::lib.
So, no, this isn't strictly necessary.
Quoted 1 line
If they do not want to use system perl there is always perlbrew.
Well, perlbrew is actually what I had in mind for this patch. Without it, it seems like the perl path -- which is configured while building git.git so is not easily changed by the user -- is "hard-coded" in the shebang line of the plackup script file which is then made executable and exec'd, to start the httpd. Given that process, I don't see how that code allows the user to use any other perl, or am I missing something?
If adding a new config variable seems too heavy-handed for such a trivial problem, another approach would be to use the first perl in PATH and fall back on the git.git build system-configured perl if there is no perl in PATH. However, considering the nature of perlbrew, the user may have many different perls installed and multiple terminals open with shells configured for different perls. To make the instaweb command search the PATH for perl would introduce some unpredictability; the command would succeed or fail depending on whether or not the perl being "used" in the current shell has the plackup httpd dependencies installed. So I think I prefer the ability to configure which perl to have git always use for instaweb over this approach.
In short summary, this patch isn't necessary because everyone could use local::lib to manage dependencies not installed at the system level, but I think this patch is desirable for those of us who use perlbrew and not local::lib. Of course, I'm open to alternatives and other suggestions.
Thanks Jakub,
Signature · 1 line
Charles McGarvey
Quoted 17 lines · Charles McGarvey
Quoted 11 lines · Jakub Narebski
Quoted 5 lines · Charles McGarvey <chazmcgarvey <at> brokenzipper.com> writes:
It is convenient for the user to be able to customize the path to perl if they do not want to use the system perl. This may be the case, for example, if the user wants to use the plackup httpd but its extra dependencies are not installed in the system perl; they can set the perl path to a perl that they install and have control over in their own home directory.
Is it really necessary? There is always PERL5LIB if one wants to use Perl modules installed in one's own home directory. If one is using
local::libone has it "for free".Yes, that's right. Using PERL5LIB would solve the example problem in the commit message, and it would even be pretty simple to set up using
local::lib.
So, no, this isn't strictly necessary.
Quoted 8 lines
Quoted 1 line
If they do not want to use system perl there is always perlbrew.
Well, perlbrew is actually what I had in mind for this patch. Without it, it seems like the perl path -- which is configured while building git.git so is not easily changed by the user -- is "hard-coded" in the shebang line of the plackup script file which is then made executable and exec'd, to start the httpd. Given that process, I don't see how that code allows the user to use any other perl, or am I missing something?
I am sorry, this is my mistake. Of course if one is using perlbrew then one wants to use perlbrew perl (nb. how does "perlbrew switch" work?), not necessarily system perl hardcoded during installation time.
Quoted 2 lines
If adding a new config variable seems too heavy-handed for such a trivial problem, another approach would be to use the first perl in PATH
Hmmm... git moved out of using first "perl" in PATH...
Quoted 2 lines
and fall back
on the git.git build system-configured perl if there is no perl in PATH.
How to do this?
[...]
Quoted 4 lines
In short summary, this patch isn't necessary because everyone could use
local::libto manage dependencies not installed at the system level, but I think this patch is desirable for those of us who use perlbrew and notlocal::lib. Of course, I'm open to alternatives and other suggestions.
One other thing to think about is: why here, and not other places?
Nb. for short Perl scripts not using extra modules we pick first "perl" in PATH, IIRC...
Signature · 1 line
Jakub Narębski
Quoted 20 lines · Jakub Narębski
Quoted 10 lines · Charles McGarvey
Quoted 4 lines · Jakub Narebski
Is it really necessary? There is always PERL5LIB if one wants to use Perl modules installed in one's own home directory. If one is using
local::libone has it "for free".Yes, that's right. Using PERL5LIB would solve the example problem in the commit message, and it would even be pretty simple to set up using
local::lib. So, no, this isn't strictly necessary.Quoted 8 lines
Quoted 1 line
If they do not want to use system perl there is always perlbrew.
Well, perlbrew is actually what I had in mind for this patch. Without it, it seems like the perl path -- which is configured while building git.git so is not easily changed by the user -- is "hard-coded" in the shebang line of the plackup script file which is then made executable and exec'd, to start the httpd. Given that process, I don't see how that code allows the user to use any other perl, or am I missing something?
Quoted 3 lines
I am sorry, this is my mistake. Of course if one is using perlbrew then one wants to use perlbrew perl (nb. how does "perlbrew switch" work?), not necessarily system perl hardcoded during installation time.
Perlbrew switch works by prepending the dirname of the specified perl to PATH (and removing paths to "inactive" perls); it is implemented as a shell function wrapper which is why it can change the current shell's environment.
So yes, searching PATH instead of using the hard-coded perl would allow the instaweb command to use a perlbrew perl (or any other perl). I think this would be better than not being able to use a non-system perl at all, but a drawback I see -- and I mentioned this before, but I'll just restate it -- is that a perl module developer usually has a lot of perls installed and it would be up to them to know whether or not the perl that is "active" for the shell they're currently in has the instaweb dependencies. It's probably not an unreasonable burden on the user, but it's something to consider. If the perl path were configurable, the user could set it to a perl they know has the dependencies and not have to deal with any uncertainty in the future whether or not the git instaweb command would succeed in their current environment.
I think that's the benefit of the current code with hard-coded perl path: it's more predictable. In other words, changes in the user's environment won't change how git (instaweb) works in potentially surprising ways. I think this is a benefit worth preserving, which is why the patch introduces a new config variable rather than using PATH.
Quoted 9 lines
Quoted 2 lines
If adding a new config variable seems too heavy-handed for such a trivial problem, another approach would be to use the first perl in PATH
Hmmm... git moved out of using first "perl" in PATH...
Quoted 2 lines
and fall back
on the git.git build system-configured perl if there is no perl in PATH.How to do this?
If you wanted to use PATH, I guess you could first try to invoke perl explicitly and let the shell search PATH implicitly, such as the following (except it doesn't make sense to do this for mongoose, so the case would need to be split).
@@ -110,11 +110,19 @@ start_httpd () {
case "$httpd" in
*mongoose*|*plackup*)
#These servers don't have a daemon mode so we'll have to fork it
- $full_httpd "$conf" &
+ perl "$full_httpd" "$conf" &
#Save the pid before doing anything else (we'll print it later) pid=$!
+ ret=$?
- if test $? != 0; then
+ #If the shell couldn't find perl, try to exec the script file directly
+ if test $ret = 127; then
+ $full_httpd "$conf" &
+ pid=$!
+ ret=$?
+ fi
+
+ if test $ret != 0; then
echo "Could not execute http daemon $httpd." exit 1
fi
I'm also not completely certain how this would affect win32 users, though I suspect it would work fine with the same caveats.
Quoted 7 lines
[...]
Quoted 4 lines
In short summary, this patch isn't necessary because everyone could use
local::libto manage dependencies not installed at the system level, but I think this patch is desirable for those of us who use perlbrew and notlocal::lib. Of course, I'm open to alternatives and other suggestions.One other thing to think about is: why here, and not other places?
I'm not familiar enough with git.git to know whether this problem exists for other components. If so, and if a solution can be agreed upon, definitely let's fix it at every place or perhaps come up with a broader solution. I'll spend some time poking around in the code.
Quoted 2 lines
Nb. for short Perl scripts not using extra modules we pick first "perl" in PATH, IIRC...
Thanks,
Signature · 1 line
Charles McGarvey