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

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 26, 2010, 22:54 UTC
Message-ID: <201012262354.16955.jnareb@gmail.com>
In-Reply-To: <20101224092932.GA31537@burratino>

On Fri, 24 Dec 2010 10:29, Jonathan Nieder wrote:

Show 13 quoted lines

> 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.

Show 8 quoted lines

> 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).

Show 12 quoted lines

> > 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).

Show 18 quoted lines

> [...]
> > --- 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