On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski <jnareb@xxxxxxxxx> 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:
>
>> 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.
>> 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.
>
>>
>> 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.
>
>> 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.
>
>>
>> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@xxxxxxxxx>
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.
>
>> ---
>>
>> 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.
>
>>
>> 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?
>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html