[PATCH] Introduce file with the common default build-time items.

[PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Words 'wish' and 'tclsh' are scattered across at least three files,
but they are tied to the same entities.  To ease the maintenance
and remove errors, these configuration items were gathered into the
separate file named 'common-make-vars.def'.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 Makefile             |   17 +++++++++++------
 common-make-vars.def |   11 +++++++++++
 configure.ac         |    4 +++-
 git-gui/Makefile     |    6 ++++--
 4 files changed, 29 insertions(+), 9 deletions(-)
 create mode 100644 common-make-vars.def

diff --git a/Makefile b/Makefile
index 6cd9ea2..e98f16b 100644
--- a/Makefile
+++ b/Makefile
@@ -116,16 +116,21 @@ all::
 #
 # The TCL_PATH variable governs the location of the Tcl interpreter
 # used to optimize git-gui for your system.  Only used if NO_TCLTK
-# is not set.  Defaults to the bare 'tclsh'.
+# is not set.  Defaults to the value of the __GIT_DEFAULT_TCLSH_NAME
+# from common-make-vars.def.
 #
-# The TCLTK_PATH variable governs the location of the Tcl/Tk interpreter.
-# If not set it defaults to the bare 'wish'. If it is set to the empty
-# string then NO_TCLTK will be forced (this is used by configure script).
+# The TCLTK_PATH variable governs the location of the Tcl/Tk
+# interpreter.  If not set it defaults to the value of the
+# __GIT_DEFAULT_TCLTK_NAME from common-make-vars.def.  If it is set
+# to the empty string then NO_TCLTK will be forced (this is used
+# by configure script).
 #
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
+# Default values
+-include common-make-vars.def
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
 uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
@@ -177,8 +182,8 @@ AR ?= ar
 TAR ?= tar
 INSTALL ?= install
 RPMBUILD ?= rpmbuild
-TCL_PATH ?= tclsh
-TCLTK_PATH ?= wish
+TCL_PATH ?= $(__GIT_DEFAULT_TCLSH_NAME)
+TCLTK_PATH ?= $(__GIT_DEFAULT_TCLTK_NAME)
 
 export TCL_PATH TCLTK_PATH
 
diff --git a/common-make-vars.def b/common-make-vars.def
new file mode 100644
index 0000000..43a3a8b
--- /dev/null
+++ b/common-make-vars.def
@@ -0,0 +1,11 @@
+# This file is meant to be sourced from the autoconf script
+# and included from the Makefile.  It carries the default values
+# to eliminate their redundancy across the files.  The syntax
+# is 'name=value' without extra spaces across the '=' sign to
+# make shell happy.  Symbols '#' are starting comments ;))
+
+# Default Tcl/Tk interpreter name
+__GIT_DEFAULT_TCLTK_NAME=wish
+
+# Default Tcl interpreter name
+__GIT_DEFAULT_TCLSH_NAME=tclsh
diff --git a/configure.ac b/configure.ac
index 50d2b85..fd1d241 100644
--- a/configure.ac
+++ b/configure.ac
@@ -12,6 +12,8 @@ config_in=config.mak.in
 
 echo "# ${config_append}.  Generated by configure." > "${config_append}"
 
+## Defaults
+source common-make-vars.def
 
 ## Definitions of macros
 # GIT_CONF_APPEND_LINE(LINE)
@@ -97,7 +99,7 @@ AC_CHECK_PROGS(TAR, [gtar tar])
 if test -z "$NO_TCLTK"; then
   if test "$with_tcltk" = ""; then
   # No Tcl/Tk switches given. Do not check for Tcl/Tk, use bare 'wish'.
-    TCLTK_PATH=wish
+    TCLTK_PATH=${__GIT_DEFAULT_TCLTK_NAME}
     AC_SUBST(TCLTK_PATH)
   elif test "$with_tcltk" = "yes"; then
   # Tcl/Tk check requested.
diff --git a/git-gui/Makefile b/git-gui/Makefile
index 3de0de1..654641a 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -6,6 +6,8 @@ all::
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
+# Default values
+-include ../common-make-vars.def
 
 SCRIPT_SH = git-gui.sh
 GITGUI_BUILT_INS = git-citool
@@ -36,8 +38,8 @@ ifndef V
 	QUIET_2DEVNULL = 2>/dev/null
 endif
 
-TCL_PATH   ?= tclsh
-TCLTK_PATH ?= wish
+TCL_PATH   ?= $(__GIT_DEFAULT_TCLSH_NAME)
+TCLTK_PATH ?= $(__GIT_DEFAULT_TCLTK_NAME}
 
 ifeq ($(findstring $(MAKEFLAGS),s),s)
 QUIET_GEN =
-- 
1.5.2.1

Re: [PATCH] Introduce file with the common default build-time items.

[Shawn O. Pearce]

Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> Words 'wish' and 'tclsh' are scattered across at least three files,
> but they are tied to the same entities.  To ease the maintenance
> and remove errors, these configuration items were gathered into the
> separate file named 'common-make-vars.def'.
...
> diff --git a/git-gui/Makefile b/git-gui/Makefile
> index 3de0de1..654641a 100644
> --- a/git-gui/Makefile
> +++ b/git-gui/Makefile
> @@ -6,6 +6,8 @@ all::
>  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
>  	@$(SHELL_PATH) ./GIT-VERSION-GEN
>  -include GIT-VERSION-FILE
> +# Default values
> +-include ../common-make-vars.def

A good idea to try and make these all common, but you cannot do this,
or at least this part of the patch.

The git-gui subdirectory of git.git is actually its own project,
maintained in its own Git repository.  It appears inside of
git.git only as a convience for users.  So I cannot reference
../common-make-vars.def as not all git-gui repositories have a
git.git repository wrapped around them.

-- 
Shawn.

Re: [PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Shawn, good day.

Thu, Jun 14, 2007 at 12:36:33AM -0400, Shawn O. Pearce wrote:
> Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> > Words 'wish' and 'tclsh' are scattered across at least three files,
> > but they are tied to the same entities.  To ease the maintenance
> > and remove errors, these configuration items were gathered into the
> > separate file named 'common-make-vars.def'.
> ...
> > diff --git a/git-gui/Makefile b/git-gui/Makefile
> > index 3de0de1..654641a 100644
> > --- a/git-gui/Makefile
> > +++ b/git-gui/Makefile
> > @@ -6,6 +6,8 @@ all::
> >  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
> >  	@$(SHELL_PATH) ./GIT-VERSION-GEN
> >  -include GIT-VERSION-FILE
> > +# Default values
> > +-include ../common-make-vars.def
> 
> A good idea to try and make these all common, but you cannot do this,
> or at least this part of the patch.
> 
> The git-gui subdirectory of git.git is actually its own project,
> maintained in its own Git repository.

OK, but then may be it will be logical to put Tcl/Tk-related variables
to the git-gui/common-make-vars.def?  I am a little worried about
this, because gitk is the part of the git.git and it should get the
variables too.  The drawback here is if you will update your git-gui
repo, then git.git will need to wait for your import to get the
changes in the variables.  And this will seriously couple git.git
and git-gui, though I don't know if it is good or bad.

Any ideas?
-- 
Eygene

Re: [PATCH] Introduce file with the common default build-time items.

[Shawn O. Pearce]

Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> 
> Thu, Jun 14, 2007 at 12:36:33AM -0400, Shawn O. Pearce wrote:
> > Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> > > Words 'wish' and 'tclsh' are scattered across at least three files,
> > > but they are tied to the same entities.  To ease the maintenance
> > > and remove errors, these configuration items were gathered into the
> > > separate file named 'common-make-vars.def'.
...
> > A good idea to try and make these all common, but you cannot do this,
> > or at least this part of the patch.
> 
> OK, but then may be it will be logical to put Tcl/Tk-related variables
> to the git-gui/common-make-vars.def?  I am a little worried about
> this, because gitk is the part of the git.git and it should get the
> variables too.  The drawback here is if you will update your git-gui
> repo, then git.git will need to wait for your import to get the
> changes in the variables.  And this will seriously couple git.git
> and git-gui, though I don't know if it is good or bad.

No, because Junio has already stated a desire to remove git-gui.git
from git.git and convert it to a proper subproject by the time of
Git 1.6.  That means the git-gui/ subdirectory will become optional,
though I imagine most git-gui users will still have it.  But not
all Git users are git-gui users.  ;-)

The best we can do is let the user pick their TCL_PATH and
TCLTK_PATH up in git's own Makefile, and have it pass down into
git-gui's Makefile when git-gui is being built from within git.
That is the arrangement we currently have.

-- 
Shawn.

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Shawn,

Thu, Jun 14, 2007 at 11:09:29AM -0400, Shawn O. Pearce wrote:
> Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> No, because Junio has already stated a desire to remove git-gui.git
> from git.git and convert it to a proper subproject by the time of
> Git 1.6.  That means the git-gui/ subdirectory will become optional,
> though I imagine most git-gui users will still have it.  But not
> all Git users are git-gui users.  ;-)

OK, it means that git-gui will be totally separated from the
git.git?  And one will download it as the separate tarball?

> The best we can do is let the user pick their TCL_PATH and
> TCLTK_PATH up in git's own Makefile, and have it pass down into
> git-gui's Makefile when git-gui is being built from within git.
> That is the arrangement we currently have.

OK, fine, thanks for the explanations.  The corrected patch follows.

-----

Words 'wish' and 'tclsh' are scattered across at least three files,
but they are tied to the same entities.  To ease the maintenance
and remove errors, these configuration items were gathered into the
separate file named 'common-make-vars.def'.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 Makefile             |   17 +++++++++++------
 common-make-vars.def |   11 +++++++++++
 configure.ac         |    4 +++-
 3 files changed, 25 insertions(+), 7 deletions(-)
 create mode 100644 common-make-vars.def

diff --git a/Makefile b/Makefile
index 6cd9ea2..e98f16b 100644
--- a/Makefile
+++ b/Makefile
@@ -116,16 +116,21 @@ all::
 #
 # The TCL_PATH variable governs the location of the Tcl interpreter
 # used to optimize git-gui for your system.  Only used if NO_TCLTK
-# is not set.  Defaults to the bare 'tclsh'.
+# is not set.  Defaults to the value of the __GIT_DEFAULT_TCLSH_NAME
+# from common-make-vars.def.
 #
-# The TCLTK_PATH variable governs the location of the Tcl/Tk interpreter.
-# If not set it defaults to the bare 'wish'. If it is set to the empty
-# string then NO_TCLTK will be forced (this is used by configure script).
+# The TCLTK_PATH variable governs the location of the Tcl/Tk
+# interpreter.  If not set it defaults to the value of the
+# __GIT_DEFAULT_TCLTK_NAME from common-make-vars.def.  If it is set
+# to the empty string then NO_TCLTK will be forced (this is used
+# by configure script).
 #
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
+# Default values
+-include common-make-vars.def
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
 uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
@@ -177,8 +182,8 @@ AR ?= ar
 TAR ?= tar
 INSTALL ?= install
 RPMBUILD ?= rpmbuild
-TCL_PATH ?= tclsh
-TCLTK_PATH ?= wish
+TCL_PATH ?= $(__GIT_DEFAULT_TCLSH_NAME)
+TCLTK_PATH ?= $(__GIT_DEFAULT_TCLTK_NAME)
 
 export TCL_PATH TCLTK_PATH
 
diff --git a/common-make-vars.def b/common-make-vars.def
new file mode 100644
index 0000000..43a3a8b
--- /dev/null
+++ b/common-make-vars.def
@@ -0,0 +1,11 @@
+# This file is meant to be sourced from the autoconf script
+# and included from the Makefile.  It carries the default values
+# to eliminate their redundancy across the files.  The syntax
+# is 'name=value' without extra spaces across the '=' sign to
+# make shell happy.  Symbols '#' are starting comments ;))
+
+# Default Tcl/Tk interpreter name
+__GIT_DEFAULT_TCLTK_NAME=wish
+
+# Default Tcl interpreter name
+__GIT_DEFAULT_TCLSH_NAME=tclsh
diff --git a/configure.ac b/configure.ac
index 50d2b85..fd1d241 100644
--- a/configure.ac
+++ b/configure.ac
@@ -12,6 +12,8 @@ config_in=config.mak.in
 
 echo "# ${config_append}.  Generated by configure." > "${config_append}"
 
+## Defaults
+source common-make-vars.def
 
 ## Definitions of macros
 # GIT_CONF_APPEND_LINE(LINE)
@@ -97,7 +99,7 @@ AC_CHECK_PROGS(TAR, [gtar tar])
 if test -z "$NO_TCLTK"; then
   if test "$with_tcltk" = ""; then
   # No Tcl/Tk switches given. Do not check for Tcl/Tk, use bare 'wish'.
-    TCLTK_PATH=wish
+    TCLTK_PATH=${__GIT_DEFAULT_TCLTK_NAME}
     AC_SUBST(TCLTK_PATH)
   elif test "$with_tcltk" = "yes"; then
   # Tcl/Tk check requested.
-- 
1.5.2.1

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Shawn O. Pearce]

Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> Thu, Jun 14, 2007 at 11:09:29AM -0400, Shawn O. Pearce wrote:
> > Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> > No, because Junio has already stated a desire to remove git-gui.git
> > from git.git and convert it to a proper subproject by the time of
> > Git 1.6.  That means the git-gui/ subdirectory will become optional,
> > though I imagine most git-gui users will still have it.  But not
> > all Git users are git-gui users.  ;-)
> 
> OK, it means that git-gui will be totally separated from the
> git.git?  And one will download it as the separate tarball?

That's one option.  But Junio and I are also considering keeping
it inside the git tarball as well, as many users have gotten used
to it being in the core Git distribution.  I think it all depends
on if myself (or someone else) adds subproject recursion support
into git-archive.  ;-)

No subproject recusion in git-archive will probably mean git-gui
would get dropped from the core git tarball.  Given we're talking
about 1.6 timeframe I think we might be able to get that feature
working by then.

> > The best we can do is let the user pick their TCL_PATH and
> > TCLTK_PATH up in git's own Makefile, and have it pass down into
> > git-gui's Makefile when git-gui is being built from within git.
> > That is the arrangement we currently have.
> 
> OK, fine, thanks for the explanations.  The corrected patch follows.
 
...
>  Makefile             |   17 +++++++++++------
>  common-make-vars.def |   11 +++++++++++
>  configure.ac         |    4 +++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
>  create mode 100644 common-make-vars.def
...

I dunno.  25 insertions and 7 deletions to reduce two uses of 'wish'
into one use of 'wish'?  That hardly seems worth the additional
18 lines of code.  Feels like code churn to me.  And I rarely feel
code churn.  I'm usually a lot more caviler about changing things
than Junio, Dscho, Nico, Linus, ...

-- 
Shawn.

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Shawn, good da.y

Thu, Jun 14, 2007 at 11:22:04PM -0400, Shawn O. Pearce wrote:
> > OK, it means that git-gui will be totally separated from the
> > git.git?  And one will download it as the separate tarball?
> 
> That's one option.  But Junio and I are also considering keeping
> it inside the git tarball as well, as many users have gotten used
> to it being in the core Git distribution.  I think it all depends
> on if myself (or someone else) adds subproject recursion support
> into git-archive.  ;-)

OK, I just wondered what to expect from the 1.6, since I am
trying to keep FreeBSD port in sync with the latest Git release.
> ...
> >  Makefile             |   17 +++++++++++------
> >  common-make-vars.def |   11 +++++++++++
> >  configure.ac         |    4 +++-
> >  3 files changed, 25 insertions(+), 7 deletions(-)
> >  create mode 100644 common-make-vars.def
> ...
> 
> I dunno.  25 insertions and 7 deletions to reduce two uses of 'wish'
> into one use of 'wish'?  That hardly seems worth the additional
> 18 lines of code.

11 of new lines are common-make-vars.def: it is heavily commented.

> Feels like code churn to me.  And I rarely feel
> code churn.  I'm usually a lot more caviler about changing things
> than Junio, Dscho, Nico, Linus, ...

OK, let us wait for the other's reaction.  May be you're right and
it doesn't worth it.  But I always felt that if I can change something
by changing it only in one place, then it worth it.  Otherwise I
should remember all places where it is used and this leads to errors.
But maybe the 'wish' and 'tclsh' are not worth it.

Thank you!
-- 
Eygene

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Shawn O. Pearce]

Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
> Thu, Jun 14, 2007 at 11:22:04PM -0400, Shawn O. Pearce wrote:
> > > OK, it means that git-gui will be totally separated from the
> > > git.git?  And one will download it as the separate tarball?
> > 
> > That's one option.  But Junio and I are also considering keeping
> > it inside the git tarball as well, as many users have gotten used
> > to it being in the core Git distribution.  I think it all depends
> > on if myself (or someone else) adds subproject recursion support
> > into git-archive.  ;-)
> 
> OK, I just wondered what to expect from the 1.6, since I am
> trying to keep FreeBSD port in sync with the latest Git release.

Heh.  Got a crystal ball?

I'm not sure anyone knows what the 1.6 release will look like yet.
Right now I know we're trying to get the submodule user interface
built and stable, before we start eating that dogfood as part of
the core Git code itself.  But we do want to be using our own tools
to build our tools.  ;-)

1.6 is just a number thrown on the wall as its likely that by the
time we get around to having a release worthy of the name 1.6 enough
users will be running a 1.5.2 or newer that has at least some of
the basic plumbing and porcelain for subprojects that it wouldn't
be a problem if we started using it ourselves in the git.git project.

-- 
Shawn.

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Fri, Jun 15, 2007 at 01:58:13AM -0400, Shawn O. Pearce wrote:
> > OK, I just wondered what to expect from the 1.6, since I am
> > trying to keep FreeBSD port in sync with the latest Git release.
> 
> Heh.  Got a crystal ball?

Sort of: when time will come and new release will be out, various
rumors I heard might help me to get the release to be ported quickly.
At least I hope so ;)
-- 
Eygene

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Johannes Sixt]

Eygene Ryabinkin wrote:
> OK, let us wait for the other's reaction.  May be you're right and
> it doesn't worth it.  But I always felt that if I can change something
> by changing it only in one place, then it worth it.  Otherwise I
> should remember all places where it is used and this leads to errors.
> But maybe the 'wish' and 'tclsh' are not worth it.

Huh? This "one place" already exists. Its name is 'config.mak'. I have
this in it:

TCL_PATH=tclsh84
TCLTK_PATH=wish84

-- Hannes

Re: [CORRECTED PATCH] Introduce file with the common default build-time items.

[Eygene Ryabinkin]

Johannes, good day.

Fri, Jun 15, 2007 at 09:15:43AM +0200, Johannes Sixt wrote:
> Eygene Ryabinkin wrote:
> > OK, let us wait for the other's reaction.  May be you're right and
> > it doesn't worth it.  But I always felt that if I can change something
> > by changing it only in one place, then it worth it.  Otherwise I
> > should remember all places where it is used and this leads to errors.
> > But maybe the 'wish' and 'tclsh' are not worth it.
> 
> Huh? This "one place" already exists. Its name is 'config.mak'. I have
> this in it:
> 
> TCL_PATH=tclsh84
> TCLTK_PATH=wish84

config.mak is a bit different: it is not used by the configure when
it sets the default TCL_PATH.  But ok, I feel that my change is not
very good, since two of you already disliked it.  Please, don't
waste your time on it anymore.
-- 
Eygene