From: Pavan Kumar Sunkara Date: Sat, 22 May 2010 16:57:51 GMT Subject: Re: [PATCHv2 GSoC] git-instaweb: Configure it to work with new gitweb structure Message-ID: In-Reply-To: <201005221757.38376.jnareb@gmail.com> 2010/5/22 Jakub Narebski : > On Sat, 22 May 2010, Pavan Kumar Sunkara wrote: > >> git-instaweb in its current form (re)creates gitweb.cgi and >> (some of) required static files in $GIT_DIR/gitweb/ directory. >> Splitting gitweb would make it difficult for git-instaweb to >> continue with this method. >> >> Use the instaweb.gitwebdir config variable to point git-instaweb script >> to a global directory which contains gitweb files as server root >> and the httpd.conf along with server logs and pid go into >> '$(GIT_DIR)/gitweb' directory. > > That's not all this patch changes, isn't it? > >  While at it, change apache2 configuration to use the same access log >  and error log files as the rest of web servers supported by >  git-instaweb. > > While it would be better to have it as a separate commit, I think it > doesn't matter much, and having it in this patch is acceptable as > "while at it" change. > >> >> Signed-off-by: Pavan Kumar Sunkara > > I like this change, because it simplifies greatly git-instaweb > generation; besides the fact that it is necessary for future splitting > gitweb for better maintability, and for write functionality for gitweb. Thanks. > Acked-by: Jakub Narebski > > _If_ there is no problem with $(gitwebdir) and not $(gitwebdir_SQ), > see below for details. > >> --- >> >> This patch is based on commit 'jn/gitweb-install' in the next branch. > > I think it is based on your earlier patches: > > * gitweb: Move static files into seperate subdirectory >  http://article.gmane.org/gmane.comp.version-control.git/147321 > * gitweb: Set default destination directory for installing gitweb in Makefile >  http://article.gmane.org/gmane.comp.version-control.git/147160 > > Those are necessary for this patch to be applied.  Well, the second is > necessary for it to make sense... Yes. They are necessary patches before this patch. I assume, they are going to be merged before this patch is. > BTW. which web servers supported by git-instaweb: lighttpd, apache2, > webrick, mongoose you have tested your change with? I tested this patch with lighttpd and apache2. And am sure about mongoose. But don't know the status with webrick. >>  Makefile        |   10 +------ >>  git-instaweb.sh |   71 ++++++++++++++++++++---------------------------------- >>  2 files changed, 28 insertions(+), 53 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index caf2f64..91cd726 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -1592,15 +1592,8 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/static/gitweb.css gitweb/ >>       sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ > [...] >> +         -e 's|@@GITWEBDIR@@|$(gitwebdir)|g' \ >>           -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \ >> -            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \ >> -            -e 's|@@GITWEB_JS_NAME@@|$(GITWEB_JS)|' \ >>           $@.sh > $@+ && \ >>       chmod +x $@+ && \ >>       mv $@+ $@ >> @@ -1972,6 +1965,7 @@ install: all >>       $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install >>  ifndef NO_PERL >>       $(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install >> +     $(MAKE) -C gitweb gitwebdir=$(gitwebdir) install >>  endif > > Have you checked that you can use $(gitwebdir) and don't need > $(gitwebdir_SQ) here?  Does git-instaweb installs and works correctly > if 'gitwebdir' contains spaces and single or double quote characters? > But perhaps that doesn't matter in practice, and this is good enough. Nope. I didn't check it. But you are right. > >> diff --git a/git-instaweb.sh b/git-instaweb.sh >> index f608014..b3e9192 100755 >> --- a/git-instaweb.sh >> +++ b/git-instaweb.sh >> @@ -24,6 +24,7 @@ restart        restart the web server >>  fqgitdir="$GIT_DIR" >>  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) >>  module_path="$(git config --get instaweb.modulepath)" >> >> @@ -34,6 +35,9 @@ conf="$GIT_DIR/gitweb/httpd.conf" >>  # if installed, it doesn't need further configuration (module_path) >>  test -z "$httpd" && httpd='lighttpd -f' >> >> +# Default is @@GITWEBDIR@@ >> +test -z "$root" && root='@@GITWEBDIR@@' >> + > > Nice. > >> @@ -57,7 +61,7 @@ resolve_full_httpd () { >>               # these days and those are not in most users $PATHs >>               # in addition, we may have generated a server script >>               # in $fqgitdir/gitweb. >> -             for i in /usr/local/sbin /usr/sbin "$fqgitdir/gitweb" >> +             for i in /usr/local/sbin /usr/sbin "$root" "$fqgitdir/gitweb" >>               do >>                       if test -x "$i/$httpd_only" >>                       then > > You probably want to update comment above this loop.  But this is not > something very important. > > Alternatively, if e.g. webrick.rb and webrick (shell script) are > installed in "$fqgitdir/gitweb" directory, there is no need to check > "$root". > >>  webrick_conf () { > [...] >> -:DocumentRoot: "$fqgitdir/gitweb" >> +:DocumentRoot: "$root" > >>  lighttpd_conf () { >>       cat > "$conf" <> -server.document-root = "$fqgitdir/gitweb" >> +server.document-root = "$root" > [...] >> -setenv.add-environment = ( "PATH" => env.PATH ) >> +setenv.add-environment = ( "PATH" => env.PATH, "GITWEB_CONFIG" => env.GITWEB_CONFIG ) > >>  apache2_conf () { > [...] >> -ServerRoot "$fqgitdir/gitweb" >> -DocumentRoot "$fqgitdir/gitweb" >> +ServerRoot "$root" >> +DocumentRoot "$root" > [...] >>  PerlPassEnv GIT_DIR >>  PerlPassEnv GIT_EXEC_DIR >> +PerlPassEnv GITWEB_CONFIG > >> @@ -353,7 +359,7 @@ mongoose_conf() { >>  # For detailed description of every option, visit >>  # http://code.google.com/p/mongoose/wiki/MongooseManual >> >> -root         $fqgitdir/gitweb >> +root         $root > [...] >> -cgi_env              PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH >> +cgi_env              PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH,GITWEB_CONFIG=$GITWEB_CONFIG > > All right, those changes are pretty clear. > >> @@ -277,14 +281,15 @@ EOF >> >>  apache2_conf () { >>       test -z "$module_path" && module_path=/usr/lib/apache2/modules >> -     mkdir -p "$GIT_DIR/gitweb/logs" >>       bind= >>       test x"$local" = xtrue && bind='127.0.0.1:' >>       echo 'text/css css' > "$fqgitdir/mime.types" >>       cat > "$conf" <>  ServerName "git-instaweb" >> -ServerRoot "$fqgitdir/gitweb" >> -DocumentRoot "$fqgitdir/gitweb" >> +ServerRoot "$root" >> +DocumentRoot "$root" >> +ErrorLog "$fqgitdir/gitweb/error.log" >> +CustomLog "$fqgitdir/gitweb/access.log" combined >>  PidFile "$fqgitdir/pid" >>  Listen $bind$port >>  EOF > > This is independent change, modifying configuration of apache2 web > server to use the same files for access log and for error log > ("$fqgitdir/gitweb/access.log" and "$fqgitdir/gitweb/error.log", > respectively) as the rest of web servers. > > Isn't it? Yes. But I included it in this commit because, it is not a big change to be included in another commit. >> @@ -370,41 +376,16 @@ mime_types      .gz=application/x-gzip,.tar.gz=application/x-tgz,.tgz=application/x-t >>  EOF >>  } >> >> - >> -script=' >> -s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#; >> -s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#; >> -s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#; >> -s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;' >> - >> -gitweb_cgi () { > [...] > >> +gitweb_conf() { >> +     cat > "$fqgitdir/gitweb/gitweb_config.perl" <> +#!/usr/bin/perl >> +our \$projectroot = "$(dirname "$fqgitdir")"; >> +our \$git_temp = "$fqgitdir/gitweb/tmp"; >> +our \$projects_list = \$projectroot; >> +EOF >>  } > > Right, $GIT (formerly $gitbin) is set when generating gitweb.cgi from > gitweb.perl. > > Actually $git_temp is not needed by modern gitweb (from quite some time, > since using external /usr/bin/diff was replaced by git-diff-tree), so > setting it could be removed from gitweb_config.perl.  Nevertheless it is > not a problem having it. > >> -gitweb_cgi "$GIT_DIR/gitweb/gitweb.cgi" >> -gitweb_css "$GIT_DIR/@@GITWEB_CSS_NAME@@" >> -gitweb_js  "$GIT_DIR/@@GITWEB_JS_NAME@@" >> +gitweb_conf >> >>  case "$httpd" in >>  *lighttpd*) >> -- >> 1.7.1.18.g74211d.dirty > > > -- > Jakub Narebski > Poland > Thanks -Pavan