[PATCH 0/2] Set Makefile variables from configure

[PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

These patches add support for setting the newly created DEFAULT_EDITOR
and DEFAULT_PAGER from the configure script.  I also tacked in
ETC_GITCONFIG, since I can't currently toggle this without setting a
command line value when building, but have need to alter it.

The function added is generic, and will allow for setting new
variables as needed in the future.  No validation is done on the
values.  It is less specific than the *_PATH setting functions that
already exist.

Ben Walton (2):
  configure: add function to directly set Makefile variables
  configure: allow user to set gitconfig, pager and editor

 configure.ac |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

[PATCH 1/2] configure: add function to directly set Makefile variables

[Ben Walton]

Add function GIT_WITH_MAKE_VAR to provide an easy way to allow user
input to directly specify values for variables in the Makefile.

An example use is:
GIT_WITH_MAKE_VAR(gitconfig, ETC_GITCONFIG)

This would allow the user to add --with-gitconfig=/etc/mysiteconf
to their ./configure command line to add
ETC_GITCONFIG=/etc/mysiteconf to the config.mak.autogen file.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 configure.ac |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 84b6cf4..2829dbb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -68,6 +68,20 @@ else \
 	GIT_CONF_APPEND_LINE(${PACKAGE}DIR=$withval); \
 fi \
 ])# GIT_PARSE_WITH
+dnl
+dnl GIT_WITH_MAKE_VAR(withname, VAR)
+dnl ---------------------
+dnl Set VAR to the value specied by --with-$withname if --with-$withname
+dnl is specified.  This is a direct way to allow setting variables in the
+dnl Makefile.
+AC_DEFUN([GIT_WITH_MAKE_VAR],
+[AC_ARG_WITH([$1],
+ [AS_HELP_STRING([--with-$1=VALUE],
+		 [provide value for $2])],
+ if test -n "$withval"; then \
+    AC_MSG_NOTICE([Setting $2 to $withval]); \
+    GIT_CONF_APPEND_LINE($2=$withval); \
+ fi)])# GIT_WITH_MAKE_VAR
 
 dnl
 dnl GIT_CHECK_FUNC(FUNCTION, IFTRUE, IFFALSE)
-- 
1.6.5

[PATCH 2/2] configure: allow user to set gitconfig, pager and editor

[Ben Walton]

Use the new GIT_WITH_MAKE_VAR function to allow user specified
values for ETC_GITCONFIG, DEFAULT_PAGER and DEFAULT_EDITOR.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 configure.ac |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 2829dbb..d50d492 100644
--- a/configure.ac
+++ b/configure.ac
@@ -241,6 +241,16 @@ GIT_PARSE_WITH(iconv))
 # change being considered an inode change from the update-index perspective.
 
 #
+# Allow user to set ETC_GITCONFIG variable
+GIT_WITH_MAKE_VAR(gitconfig, ETC_GITCONFIG)
+#
+# Allow user to set the default pager
+GIT_WITH_MAKE_VAR(pager, DEFAULT_PAGER)
+#
+# Allow user to set the default editor
+GIT_WITH_MAKE_VAR(editor, DEFAULT_EDITOR)
+
+#
 # Define SHELL_PATH to provide path to shell.
 GIT_ARG_SET_PATH(shell)
 #
-- 
1.6.5

Re: [PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

Excerpts from Ben Walton's message of Sat Oct 31 16:41:33 -0400 2009:

Ping?  Is this useful or should I drop it?

Thanks
-Ben

> These patches add support for setting the newly created DEFAULT_EDITOR
> and DEFAULT_PAGER from the configure script.  I also tacked in
> ETC_GITCONFIG, since I can't currently toggle this without setting a
> command line value when building, but have need to alter it.
> 
> The function added is generic, and will allow for setting new
> variables as needed in the future.  No validation is done on the
> values.  It is less specific than the *_PATH setting functions that
> already exist.
> 
> Ben Walton (2):
>   configure: add function to directly set Makefile variables
>   configure: allow user to set gitconfig, pager and editor
> 
>  configure.ac |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/2] Set Makefile variables from configure

[Jonathan Nieder]

Ben Walton wrote:
> Excerpts from Ben Walton's message of Sat Oct 31 16:41:33 -0400 2009:
 
>> These patches add support for setting the newly created DEFAULT_EDITOR
>> and DEFAULT_PAGER from the configure script.  I also tacked in
>> ETC_GITCONFIG, since I can't currently toggle this without setting a
>> command line value when building, but have need to alter it.
[...]
> Ping?  Is this useful or should I drop it?

I am not sure these patches alone would make the configuration much
easier.  The --help text means one has to check the Makefile to learn
what these options do, anyway:

--with-editor=VALUE	provide value for DEFAULT_EDITOR
--with-pager=VALUE	provide value for DEFAULT_PAGER

But then, why not just set the value directly in the Makefile, 'make'
command line, or config.mak?

Maybe something like this could help.  The help text is not carefully
written; it’s just a placeholder to show the idea.

 configure.ac |   56 ++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac
index f8db034..aeef08c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -68,20 +68,25 @@ else \
 	GIT_CONF_APPEND_LINE(${PACKAGE}DIR=$withval); \
 fi \
 ])# GIT_PARSE_WITH
+
 dnl
-dnl GIT_WITH_MAKE_VAR(withname, VAR)
-dnl ---------------------
-dnl Set VAR to the value specied by --with-$withname if --with-$withname
-dnl is specified.  This is a direct way to allow setting variables in the
-dnl Makefile.
-AC_DEFUN([GIT_WITH_MAKE_VAR],
-[AC_ARG_WITH([$1],
- [AS_HELP_STRING([--with-$1=VALUE],
-		 [provide value for $2])],
- if test -n "$withval"; then \
-    AC_MSG_NOTICE([Setting $2 to $withval]); \
-    GIT_CONF_APPEND_LINE($2=$withval); \
- fi)])# GIT_WITH_MAKE_VAR
+dnl GIT_PARSE_VALUE(WITHNAME, VAR)
+dnl ------------------------------
+dnl For use in AC_ARG_WITH action-if-found, for tweakable values that
+dnl are not actually enabling or disabling anything.
+dnl  * Set VAR=ARG for --with-WITHNAME=ARG
+dnl  * Complain for --without-WITHNAME or --with-WITHNAME without ARG
+AC_DEFUN([GIT_PARSE_VALUE], [
+if test "$withval" = "no"; then
+	AC_MSG_ERROR([$1 cannot be disabled])
+fi
+if test "$withval" = "yes"; then
+	AC_MSG_WARN([You should provide a value for --with-$1])
+else
+	AC_MSG_NOTICE([Setting $2 to $withval])
+	GIT_CONF_APPEND_LINE($2=$withval)
+fi
+])
 
 dnl
 dnl GIT_CHECK_FUNC(FUNCTION, IFTRUE, IFFALSE)
@@ -242,13 +247,32 @@ GIT_PARSE_WITH(iconv))
 
 #
 # Allow user to set ETC_GITCONFIG variable
-GIT_WITH_MAKE_VAR(gitconfig, ETC_GITCONFIG)
+AC_ARG_WITH(gitconfig,
+  [AS_HELP_STRING([--with-gitconfig=PATH],
+                  [Use PATH instead of /etc/gitconfig.]
+                  [Interpreted by git as a path relative to the computed]
+                  [prefix at run-time.])],
+  [GIT_PARSE_VALUE(gitconfig, ETC_GITCONFIG)])
+
 #
 # Allow user to set the default pager
-GIT_WITH_MAKE_VAR(pager, DEFAULT_PAGER)
+AC_ARG_WITH(pager,
+  [AS_HELP_STRING([--with-pager=CMD],
+                  [Set fall-back pager to CMD instead of less.]
+                  [The fall-back pager will be used by commands such]
+                  [as git log if the user does not specify some]
+                  [preferred pager.])],
+  [GIT_PARSE_VALUE(pager, DEFAULT_PAGER)])
+
 #
 # Allow user to set the default editor
-GIT_WITH_MAKE_VAR(editor, DEFAULT_EDITOR)
+AC_ARG_WITH(editor,
+  [AS_HELP_STRING([--with-editor=CMD],
+                  [Set fall-back text editor to CMD instead of vi.]
+                  [The fall-back editor will be used by commands such]
+                  [as git commit if the user does not specify some]
+                  [preferred editor.])],
+  [GIT_PARSE_VALUE(editor, DEFAULT_EDITOR)])
 
 #
 # Define SHELL_PATH to provide path to shell.
-- 
1.6.5.2

Re: [PATCH 0/2] Set Makefile variables from configure

[Jonathan Nieder]

Ben Walton wrote:
> These patches add support for setting the newly created DEFAULT_EDITOR
> and DEFAULT_PAGER from the configure script.  I also tacked in
> ETC_GITCONFIG, since I can't currently toggle this without setting a
> command line value when building, but have need to alter it.

Would the --sysconfdir option work for you here?  Setting --sysconfdir
currently does nothing, so the question is kind of moot without some
change like this (untested):

Hook up more of ./configure’s installation directory options to
actually do something.  Unfortunately, this defeats the RUNTIME_PREFIX
facility unless all the relevant paths are explicitly set.  Probably
the defaults set in Makefile should not be overridden unless
explicitly requested.

This patch also changes the default location for HTML documentation
after running ./configure from /usr/local/share/git-doc to
/usr/local/share/git.  In my opinion, the fix to this would also be to
make the configure script not override defaults from Makefile.

 config.mak.in |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index 67b12f7..430134a 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -11,17 +11,21 @@ TAR = @TAR@
 TCLTK_PATH = @TCLTK_PATH@
 
 prefix = @prefix@
-exec_prefix = @exec_prefix@
 bindir = @bindir@
+mandir = @mandir@
+infodir = @infodir@
 gitexecdir = @libexecdir@/git-core
-datarootdir = @datarootdir@
+sharedir = @datadir@
 template_dir = @datadir@/git-core/templates
+htmldir = @htmldir@
+sysconfdir = @sysconfdir@
 
-mandir=@mandir@
-
+exec_prefix = @exec_prefix@
+datarootdir = @datarootdir@
+PACKAGE_TARNAME = @PACKAGE_TARNAME@
+docdir = @docdir@
 srcdir = @srcdir@
 VPATH = @srcdir@
-
 export exec_prefix mandir
 export srcdir VPATH
 
-- 
1.6.5.2

[PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

Ok, a second attempt, taking into account Jonathan's feedback and
proposed updates.  The revised macro allows for specified help text.

I opted to use a more 'dry' approach to the macro instead of repeating
the boilerplate AC_WITH_ARG bits when calling this feature.

Additionally, I only WARN when 'yes' or 'no' is passed.  Bailing on
errors in most cases is reasonable, but I don't think it's globally
appropriate.

As to why I want this...It's cleaner, in my opinion, to do all build
configuration through a single mechanism than to do most with
./configure and the rest with variables passed to make.  In other
words, this is purely a style issue.

I like the extension of config.mak.in to support more of the autoconf
variables as well and would like to see it applied.

Ben Walton (2):
  configure: add macro to set arbitrary make variables
  configure: add settings for gitconfig, editor and pager

 configure.ac |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

[PATCH 1/2] configure: add macro to set arbitrary make variables

[Ben Walton]

Add macro GIT_PARSE_WITH_SET_MAKE_VAR to configure.ac to allow --with
style options that set values for variables used during the make
process.

Arguments are the $name part of --with-$name, the name of
the variable to set in the Makefile (config.mak.autogen) and
the help text for the option.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 configure.ac |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 84b6cf4..a305d0b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -68,6 +68,26 @@ else \
 	GIT_CONF_APPEND_LINE(${PACKAGE}DIR=$withval); \
 fi \
 ])# GIT_PARSE_WITH
+#
+# GIT_PARSE_WITH_SET_MAKE_VAR(WITHNAME, VAR, HELP_TEXT)
+# ---------------------
+# Set VAR to the value specied by --with-WITHNAME.
+# No verification of arguments is performed, but warnings are issued
+# if either 'yes' or 'no' is specified.
+# HELP_TEXT is presented when --help is called.
+# This is a direct way to allow setting variables in the Makefile.
+AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
+[AC_ARG_WITH([$1],
+ [AS_HELP_STRING([--with-$1=VALUE], $3)],
+ if test -n "$withval"; then \
+  if test "$withval" = "yes" -o "$withval" = "no"; then \
+    AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
+		     [a value for $1 ($2).  Maybe you do...?]); \
+  fi; \
+  \
+  AC_MSG_NOTICE([Setting $2 to $withval]); \
+  GIT_CONF_APPEND_LINE($2=$withval); \
+ fi)])# GIT_PARSE_WITH_SET_MAKE_VAR
 
 dnl
 dnl GIT_CHECK_FUNC(FUNCTION, IFTRUE, IFFALSE)
-- 
1.6.5.1

[PATCH 2/2] configure: add settings for gitconfig, editor and pager

[Ben Walton]

Use the new GIT_PARSE_WITH_SET_MAKE_VAR macro to allow configuration
settings for ETC_GITCONFIG, DEFAULT_PAGER and DEFAULT_EDITOR.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 configure.ac |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index a305d0b..78345eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -247,6 +247,29 @@ GIT_PARSE_WITH(iconv))
 # change being considered an inode change from the update-index perspective.
 
 #
+# Allow user to set ETC_GITCONFIG variable
+GIT_PARSE_WITH_SET_MAKE_VAR(gitconfig, ETC_GITCONFIG,
+			Use VALUE instead of /etc/gitconfig as the
+			global git configuration file.
+			If VALUE is not fully qualified it will be interpretted
+			as a path relative to the computed prefix at runtime.)
+
+#
+# Allow user to set the default pager
+GIT_PARSE_WITH_SET_MAKE_VAR(pager, DEFAULT_PAGER,
+			Use VALUE as the fall-back pager instead of 'less'.
+			This is used by things like 'git log' when the user
+			does not specify a pager to use through alternate
+			methods. eg: /usr/bin/pager)
+#
+# Allow user to set the default editor
+GIT_PARSE_WITH_SET_MAKE_VAR(editor, DEFAULT_EDITOR,
+			Use VALUE as the fall-back editor instead of 'vi'.
+			This is used by things like 'git commit' when the user
+			does not specify a preferred editor through other
+			methods. eg: /usr/bin/editor)
+
+#
 # Define SHELL_PATH to provide path to shell.
 GIT_ARG_SET_PATH(shell)
 #
-- 
1.6.5.1

Re: [PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

Excerpts from Jonathan Nieder's message of Tue Nov 03 17:21:23 -0500 2009:

> Would the --sysconfdir option work for you here?  Setting --sysconfdir
> currently does nothing, so the question is kind of moot without some
> change like this (untested):

I think this is a generally useful patch.

Thanks
-Ben
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/2] Set Makefile variables from configure

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> Ok, a second attempt, taking into account Jonathan's feedback and
> proposed updates.  The revised macro allows for specified help text.
>
> I opted to use a more 'dry' approach to the macro instead of repeating
> the boilerplate AC_WITH_ARG bits when calling this feature.
>
> Additionally, I only WARN when 'yes' or 'no' is passed.  Bailing on
> errors in most cases is reasonable, but I don't think it's globally
> appropriate.
>
> As to why I want this...It's cleaner, in my opinion, to do all build
> configuration through a single mechanism than to do most with
> ./configure and the rest with variables passed to make.  In other
> words, this is purely a style issue.

Sorry, I wasn't following the discussion primarily because I am totally
uninterested in autoconf myself (it's purely a style issue and using a
single mechanism of echoing into config.mak is just as clean), so allow me
to ask a stupid question that might have been already answered while the
initial round was discussed.

I am a bit puzzled about the "warning" logic.  Is this because you expect
variables we typically give YesPlease/NoThanks as their values will not be
handled with this new PARSE_WITH_SET_MAKE_VAR() macro?

Re: [PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

Excerpts from Junio C Hamano's message of Wed Nov 04 14:36:32 -0500 2009:

> I am a bit puzzled about the "warning" logic.  Is this because you expect
> variables we typically give YesPlease/NoThanks as their values will not be
> handled with this new PARSE_WITH_SET_MAKE_VAR() macro?

No, this is because it's perfectly acceptable, in my opinion for a
user to say:

--with-pager=no
or
--with-editor=yes

Neither of those are smart things to do, but they're not necessarily
wrong, either.  I'm open to bailing on error for these, but I thought
leaving these as unvalidated, but with a warning, was more
flexible...if say a user wanted to have a pager called 'no' or
something.

For the variables that are accepting YesPlease/NoThanks, I think it's
more suitable to use the standard autoconf header/function/library
detection as it stands now.  This macro is more for 'oddball'
variables like DEFAULT_PAGER, DEFAULT_EDITOR, etc that aren't
necessarily easily detectable.  In some cases, even if it were
detectable, the detection might not be right.

Thanks
-Ben
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/2] Set Makefile variables from configure

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> Excerpts from Junio C Hamano's message of Wed Nov 04 14:36:32 -0500 2009:
>
>> I am a bit puzzled about the "warning" logic.  Is this because you expect
>> variables we typically give YesPlease/NoThanks as their values will not be
>> handled with this new PARSE_WITH_SET_MAKE_VAR() macro?
>
> No, this is because it's perfectly acceptable, in my opinion for a
> user to say:
>
> --with-pager=no
> or
> --with-editor=yes
>
> Neither of those are smart things to do, but they're not necessarily
> wrong, either.  I'm open to bailing on error for these, but I thought
> leaving these as unvalidated, but with a warning, was more
> flexible...if say a user wanted to have a pager called 'no' or
> something.

What puzzled me was not "why is it not an error but just a warning?", which
is what you just explained, but "why should we even care what value is
being given to begin with?".

I am _not_ saying "I think it is more correct not to check the value at
all".  I just wanted to understand in what situation it may be benefitial
to give this warning in the first place.

> For the variables that are accepting YesPlease/NoThanks, I think it's
> more suitable to use the standard autoconf header/function/library
> detection as it stands now.  This macro is more for 'oddball'
> variables like DEFAULT_PAGER, DEFAULT_EDITOR, etc that aren't
> necessarily easily detectable.  In some cases, even if it were
> detectable, the detection might not be right.

I am guessing from this description of 'oddball variables' that your
answer to my question is yes.  That is, configure.ac writers are strongly
discouraged from using this new macro for variables that would usually get
YesPlease/NoThanks kind of values.

And then it makes sense to warn 'yes/no', as it is unlikely that the user
wants to set name (or path) of programs we allow to be tweaked to 'yes' or
'no'.

Re: [PATCH 0/2] Set Makefile variables from configure

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Excerpts from Junio C Hamano's message of Wed Nov 04 14:56:57 -0500 2009:

> What puzzled me was not "why is it not an error but just a warning?", which
> is what you just explained, but "why should we even care what value is
> being given to begin with?".

I'm sorry.  I misunderstood then.  I think that in most cases, setting
either 'yes' or 'no' is wrong, but I wanted to leave the door open for
it for the 1%.  I thought that a warning message might be noticed and
cause someone to rethink.

> I am guessing from this description of 'oddball variables' that your
> answer to my question is yes.  That is, configure.ac writers are
> strongly discouraged from using this new macro for variables that
> would usually get YesPlease/NoThanks kind of values.

Yes, that's correct.  This new macro is for variables that are not a
simple toggle.  It is also distinct from the mechanism used to specify
a path for perl, tcl/tk, etc, in that for those, we do want to error
out on no.

Thanks
-Ben

-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/2] Set Makefile variables from configure

[Jonathan Nieder]

Ben Walton wrote:
> Excerpts from Junio C Hamano's message of Wed Nov 04 14:36:32 -0500 2009:
> 
>> I am a bit puzzled about the "warning" logic.  Is this because you expect
>> variables we typically give YesPlease/NoThanks as their values will not be
>> handled with this new PARSE_WITH_SET_MAKE_VAR() macro?
> 
> No, this is because it's perfectly acceptable, in my opinion for a
> user to say:
> 
> --with-pager=no
> or
> --with-editor=yes

More to the point, that’s what autoconf gives for "--without-pager" or
plain "--with-editor".  It seems strange to silently use PAGER=no or
EDITOR=yes in that case.

Maybe the options should just be --pager=foo and --editor=bar, which
would be less misleading and avoid this problem.  But this is not my
itch (I find it cleaner to use the Makefile directly), so I have no
strong opinion.  Unfortunately, my autoconf-fu is too weak for a demo.

Jonathan