Re: [RFC PATCH v7 4/9] gitweb: Prepare for splitting gitweb

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 24 Dec 2010 10:29, Jonathan Nieder wrote:
> Jakub Narebski wrote:
> 
> > Prepare gitweb for having been split into modules that are to be
> > installed alongside gitweb in 'lib/' subdirectory, by adding
> > 
> >   use lib __DIR__.'/lib';
> > 
> > to gitweb.perl (to main gitweb script), and preparing for putting
> > modules (relative path) in $(GITWEB_MODULES) in gitweb/Makefile.
> 
> Spelled out, this means modules would typically go in
> 
> 	/usr/share/gitweb/lib

Yes, it's true.  It is mainly to support situation where one can install
files in (subdirectory of) cgi-bin, but nowehere else.  That is why the
default is to install modules alongside with gitweb.

The additional advantage is that t/gitweb-lib.sh used by gitweb tests
can very simply test source version of gitweb, with gitweb finding
source version of modules.  But it is not a very large obstacle to
change this.
 
> Is that the right place?  I suspect something like
> 
> 	/usr/lib/gitweb/
> 
> could make sense in some installations for two reasons:
> 
>  - even braindamaged webserver configurations would not serve lib/
>    as static files in that case;

Actually it doesn't matter what web server does with those files when
accessed directly, except for the client (user) confusion if he/she
goes where not invited.  Modules are used by Perl (by gitweb), not by
web server.

> 
>  - if some modules are implemented in C for speed, they would need
>    to go in /usr/lib anyway to follow usual filesystem conventions.

Ugh, XS!  I sincerely hope that when there would be decision to implement
some features in C for speed, we would be able to use Perl version of
ctypes for C-to-Perl interface, not XS.

Anyway most probable to be implemented in C would be Git.pm, or rather
Perl interface to libgit2.  It is probable that at some point gitweb
would be converted to use Git.pm or its successor.  But I guess that
Git Perl module would be installed somewhere in PERL5LIB, so it would
be found even without  "use lib __DIR__ . '/lib';"  or its replacement.

> Does the Makefile let us override the directory with such a setting?
> 

I have thought that I did provide 'gitweblibdir' as configurable knob,
but I see that in the version I have send I don't do this:

  # Shell quote;
  bindir_SQ = $(subst ','\'',$(bindir))#'
  gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
  gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
  gitweblibdir_SQ = $(subst ','\'',$(gitwebdir)/lib)#'

But if we are to allow custom gitweblibdir, we would have to change the
way gitweb is to find its modules.  One solution would be inetad of
current

  # __DIR__ is taken from Dir::Self __DIR__ fragment
  sub __DIR__ () {
  	File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
  }
  use lib __DIR__ . '/lib';

use simply

  use lib $ENV{GITWEBLIBDIR} || "++GITWEBLIBDIR++";

Of course both gitweb/Makefile and t/gitweb-lib.sh would have to be 
updated: gitweb/Makefile to include replacement rule for '++GITWEBLIBDIR++'
in GITWEB_REPLACE, and t/gitweb-lib.sh to declare and export GITWEBLIBDIR
environmental variable so that gitweb/gitweb.perl would be able to find
its modules when used for gitweb tests (see comment earlier).

> > While at it pass GITWEBLIBDIR in addition to GITWEB_TEST_INSTALLED to
> > allow testing installed version of gitweb and installed version of
> > modules (for future tests which would check individual (sub)modules).
> > 
> > Using __DIR__ from Dir::Self module (not in core, that's why currently
> > gitweb includes excerpt of code from Dir::Self defining __DIR__) was
> > chosen over using FindBin-based solution (in core since perl 5.00307,
> > while gitweb itself requires at least perl 5.8.0) because FindBin uses
> > BEGIN block
> 
> This explanation and the code below leave me nervous that the answer
> might be "no". ;-)

No it doesn't, but yes it could (see above).

> [...]
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -10,6 +10,14 @@
> >  use 5.008;
> >  use strict;
> >  use warnings;
> > +
> > +use File::Spec;
> > +# __DIR__ is taken from Dir::Self __DIR__ fragment
> > +sub __DIR__ () {
> > +	File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
> > +}
> > +use lib __DIR__ . '/lib';
> > +
> >  use CGI qw(:standard :escapeHTML -nosticky);
> >  use CGI::Util qw(unescape);
> >  use CGI::Carp qw(fatalsToBrowser);


-- 
Jakub Narebski
Poland