Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski [off-list ref] wrote:

Cc-ed Eric Wong, the main author and maintainer of git-instaweb

In short: I think that this patch should be split into two patches, one
which sets default value of 'gitwebdir' (in Makefile or gitweb/Makefile;
please explain why you chosen one or the other), and second that "fixes"
git-instaweb (and might include installing gitweb, in $(gitwebdir) or in
$(sharedir)/gitweb).

Yes, I agree. This is the first patch.
The second patch which fixes git-instaweb is in discussion with my
mentors. after that I will be sending it to the git mailing list.

I choose Makefile rather than chossing gitweb/Makefile becuase
git-instaweb is a package of git not a package of gitweb. So, I choose
Makefile rather than choosing gitweb/Makefile

On Thu, 13 May 2010, Pavan Kumar Sunkara wrote:

quoted

Subject: gitweb: Add global installation target for gitweb

The name "target" is here a bit misleading in the context of Makefile.
You meant that you set default installation destination (default
installation directory) for install-gitweb target in Makefile, by adding
default value of 'gitwebdir' variable.  To clarify this issue I think it
would be better to write:

 gitweb: Set default destination directory for installing gitweb

or something like that.

Ok.

quoted

The current installation of gitweb requires us to give it a
target directory. But splitting of gitweb makes it difficult
for git instaweb to continue with the current method.

This paragraph has not a best grammar, and is also slightly inaccurate.
Also having default value for build variables (for a destination
directories for install targets) is a good thing in itself, independent
of the issue of git-instaweb and splitting gitweb.

What about this?

 Currently installing gitweb requires to give a target directory
 (via 'gitwebdir' build variable).  Giving it a default value
 protects against user errors.

 Also git-instaweb in its current form (re)creates gitweb.cgi and
 (some of) required static files in $GIT_DIR/gitweb/ directory for
 each repository it is ran.  Splitting gitweb would make it difficult
 for git-instaweb to continue with this method.

See also comment about git-instaweb below.

quoted

This commit allow installation of gitweb files into the target
'$(sharedir)/gitweb' by default when user type 'make install'.

I would phrase it a bit differently myself, emphasizing that
"make install" would now also install gitweb (!):

 This commit sets default installation directory for gitweb files
 to "$(sharedir)/gitweb".  The 'install' target ("make install")
 now also installs gitweb.

quoted

This target act as root directory for instaweb servers.

It does not, as you have not provided required changes to
git-instaweb.sh and 'git-instaweb' target in main Makefile.

quoted

Signed-off-by: Pavan Kumar Sunkara <redacted>

Ok.

Two things.

First, I think providing default value for 'gitwebdir' could be I good
idea.  I think that all other build variables containing installation
directories have default values.  But I do wonder whether the
"$(sharedir)/gitweb" is a good default value for 'gitwebdir' (see also
comment about git-instaweb below).

I chose it because the lib files of gitk and git-gui are being
installed in sharedir.
I am happy to accept if you have any other better idea. :-)

Second, the issue with git-instaweb in its current form, and splitting
gitweb is very important.  I am not sure though if this is correct
solution for this problem.  It is NOT A FULL SOLUTION, that is sure.

Yes, It is not a full solution. There is another patch that is
currently in discussion with my mentors.
Petr and Christian told me to make sure that I send patches as small
as possibl so that they will be merged into the mainstream. That is my
GSoC aim too.

So, I sent this small patch which justs installs gitweb with git's
"make install" without breaking the git-instaweb.sh script.
The patch for modifying git-instaweb will be sent soon to his mailing list.

The gitweb.cgi file that git-instaweb puts into $GIT_DIR/gitweb/gitweb.cgi
is *modified* to browse given git repository; the gitweb_cgi() function
in git-instaweb that creates gitweb.cgi also changes $projectroot in it:

 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 () {
       cat > "$1.tmp" <<\EOFGITWEB
 @@GITWEB_CGI@@
 EOFGITWEB
       # Use the configured full path to perl to match the generated
       # scripts' 'hashpling' line
       "$PERL" -p -e "$script" "$1.tmp"  > "$1" # <------ '-e "$script"'
       chmod +x "$1"
       rm -f "$1.tmp"
 }

So in its current for git-instaweb wouldn't be able to use generic
gitweb.cgi that is installed in "$(sharedir)/gitweb".

Also git-instaweb when run (when starting server) puts appropriate
configuration into $GIT_DIR/gitweb/httpd.conf (the same file for any web
server chosen).  This configuration includes paths to static files that
gitweb requires, and that currently git-instaweb puts (installs) in
$GIT_DIR/gitweb (in $fqgitdir).  So you would need to change that for a
full solution of git-instaweb problem.

The solution to modifying gitweb.cgi in git-instaweb could be for
git-instaweb to put (install) appropriately configured
gitweb_config.perl file into $GIT_DIR/gitweb, and set GITWEB_CONFIG (and
export) environmental variable to it inside git-instaweb.

As we discussed earlier, I will be making use of git-instaweb script
and gitweb to create a local server for user which takes a file list
of repositories and provide client UI for them. According to this, the
user needs to execute 'git instaweb' only once, not in every
repository.

The next patch will take care of modifying git-instaweb.sh and
configuring gitweb.perl

By the way, perhaps in the future split gitweb should install its
subpackages (the *.pm files) in the same place that Git.pm is installed,
and for gitweb.perl to have

 use lib (split(/:/, $ENV{GITPERLLIB}));

Or not.  Food for though.

quoted

---

This is necessary step to achieve the goals of my GSoC project.
Currently instaweb script creates gitweb.* files in every repository
which is unnecessary. So if we have global folder for gitweb files we
can configure the instaweb server root to point to that direction.

*Can* configure, but you actually doesn't do this.

quoted

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index de7f680..0b262a9 100644
--- a/Makefile
+++ b/Makefile

@@ -269,6 +269,7 @@ mandir = share/man

 infodir = share/info
 gitexecdir = libexec/git-core
 sharedir = $(prefix)/share
+gitwebdir = $(sharedir)/gitweb

Why in Makefile, and not in gitweb/Makefile?

quoted

 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)

@@ -1971,6 +1972,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

First, I think that gitwebdir=$(gitwebdir) is not necessary here. Make
automatically passes variables to invoked submakes. And then it would be
as easy as adding 'install-gitweb' (or new 'install-git-instaweb') as
additional dependency for 'install' target.

So, you want to me to do something like this ?
+     $(MAKE) -c gitweb gitwebdir=$(sharedir)/gitweb install

Second, why don't you set default value of 'gitwebdir' in gitweb/Makefile
instead?

It's explained above.
I don't want the user to install gitweb specifically, from now onwards
gitweb will installed along with main program so that git-instaweb
script will be working properly and the users will be happy to get a
web client along with git.

The above are questions that needs to be answered, but not necessarily
require changes to patch.


Please also note that if user wants to install gitweb in for example
'/var/www/cgi-bin', and to do that sets "gitwebdir=/var/www/cgi-bin" in
config.mak file, so that it would be present for "make install" (and not
only "make install-gitweb"), gitweb files would be not present in
"$(sharedir)/gitweb" for git-instaweb to find (well, unless git-instaweb
search for them in "$(gitwebdir)" instead...).

User need to specify instaweb.root in config file to point to the
gitweb files which defaults to /usr/share/gitweb
This is covered in my next patch.

Most important of all, without changes to git-instaweb.sh and
git-instaweb target in main Makefile, installing gitweb by default, be
it

 install: all install-gitweb

or

       $(MAKE) -C gitweb gitwebdir="$(sharedir)/gitweb" install

doesn't make absoultely no sense.

Yes, it doesn't make sense. But petr and christian asked me to send my
patches as small as possible without breaking any of the current
functionalities. That is the reason I sent this.

I need to complete my GSoC, so you can be sure that every patch of
mine has a reason.

NAK to this part of patch.

Just rethink about this or you can wait until my next patch :-)

Thanks
- Pavan