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).
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.
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 <redacted>
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).
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.
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.
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.
quoted hunk
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 hunk
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.
Second, why don't you set default value of 'gitwebdir' in gitweb/Makefile
instead?
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...).
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.
NAK to this part of patch.
endif
ifndef NO_PYTHON
$(MAKE) -C git_remote_helpers prefix='$(prefix_SQ)'
DESTDIR='$(DESTDIR_SQ)' install
--
1.7.1.16.g5d405c.dirty
--
Jakub Narebski
Poland
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
On Fri, 14 May 2010, Pavan Kumar Sunkara wrote:
On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski [off-list ref] wrote:
quoted
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.
As I said, I agree with the reasoning and the fact that patch is split
into two. What I disagree with is the splitting itself.
Making 'install-gitweb' prerequisite to 'install' target, or in other
words adding installing gitweb *somewhere*, should in my opinion by in
this second patch.
I choose Makefile rather than choosing gitweb/Makefile becuase
git-instaweb is a package of git not a package of gitweb. So, I choose
Makefile rather than choosing gitweb/Makefile.
This is important fact; the description that 'gitwebdir' has default
value set (also) in Makefile because of the future change to the way
git-instaweb is build / installed should be in commit message of a new
first patch.
BTW. if 'gitwebdir' would be set to some default value both in Makefile
and in gitweb/Makefile, then in gitweb/Makefile the "?=" operator must
be used (set if undefined).
Note that default value in Makefile can be "$(sharedir)/gitweb", and in
gitweb/Makefile can be '/var/www/cgi-bin'.
[...]
quoted
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.
O.K., I can agree with this. You might want to put this substantiation
in the commit message, though.
quoted
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 just 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.
That is a good practice, and a good idea.
My complaints are about putting too much in this first patch (I think
that making 'install' target install gitweb should be put in the commit
that changes git-instaweb, as those two changes have to be
synchronized), and about that commit message seems to claim that this
commit does more than it really does.
--
Jakub Narebski
Poland
As I said, I agree with the reasoning and the fact that patch is split
into two. What I disagree with is the splitting itself.
Making 'install-gitweb' prerequisite to 'install' target, or in other
words adding installing gitweb *somewhere*, should in my opinion by in
this second patch.
quoted
I choose Makefile rather than choosing gitweb/Makefile becuase
git-instaweb is a package of git not a package of gitweb. So, I choose
Makefile rather than choosing gitweb/Makefile.
This is important fact; the description that 'gitwebdir' has default
value set (also) in Makefile because of the future change to the way
git-instaweb is build / installed should be in commit message of a new
first patch.
BTW. if 'gitwebdir' would be set to some default value both in Makefile
and in gitweb/Makefile, then in gitweb/Makefile the "?=" operator must
be used (set if undefined).
Note that default value in Makefile can be "$(sharedir)/gitweb", and in
gitweb/Makefile can be '/var/www/cgi-bin'.
[...]
quoted
quoted
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.
O.K., I can agree with this. You might want to put this substantiation
in the commit message, though.
quoted
quoted
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 just 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.
That is a good practice, and a good idea.
My complaints are about putting too much in this first patch (I think
that making 'install' target install gitweb should be put in the commit
that changes git-instaweb, as those two changes have to be
synchronized), and about that commit message seems to claim that this
commit does more than it really does.
--
Jakub Narebski
Poland
So, what I need to do in this patch is add default values. Not
installing in 'install' target. ??
Or any thing else.
If you could specify what needs to be done, I will be happy resend the patch.
- Pavan
On Sat, 15 May 2010, Pavan Kumar Sunkara wrote:
So, what I need to do in this patch is add default values. Not
installing in 'install' target. ??
Or any thing else.
If you could specify what needs to be done, I will be happy resend
the patch.
To get my ACK for this patch it is enough to *not* add installing gitweb
to 'install' target in main Makefile. IMVHO it should be left for next
patch in this series.
P.S. By the way, you should write in comments to patch, i.e. between
"---" separator and the diffstat, that this patch is based on 'next'
branch, or perhaps even that it is based on commit 152d943 in
'jn/gitweb-install'.
From Documentation/SubmittingChanges:
If you are preparing a work based on "next" branch, that is fine,
but please mark it as such.
^^^^^^^^^^^^^^^
Keep up good work!
--
Jakub Narebski
Poland