[PATCH] Respect definition of prefix from autotools in ETC_GITCONFIG and ETC_GITATTRIBUTES

[PATCH] Respect definition of prefix from autotools in ETC_GITCONFIG and ETC_GITATTRIBUTES

[Kacper Kornet]

Definitions of ETC_GITCONFIG and ETC_GITATTRIBUTES depend on value of
prefix. As prefix can be changed in config.mak.autogen, all if blocks
with conditions based on prefix should be placed after the file is
included in Makefile.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Makefile |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index cbc3fce..5b4ae40 100644
--- a/Makefile
+++ b/Makefile
@@ -291,15 +291,8 @@ sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
-ifeq ($(prefix),/usr)
-sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
 ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
-else
-sysconfdir = $(prefix)/etc
-ETC_GITCONFIG = etc/gitconfig
-ETC_GITATTRIBUTES = etc/gitattributes
-endif
 lib = lib
 # DESTDIR=
 pathsep = :
@@ -1192,6 +1185,12 @@ endif
 -include config.mak.autogen
 -include config.mak
 
+ifeq ($(prefix),/usr)
+sysconfdir = /etc
+else
+sysconfdir = etc
+endif
+
 ifdef CHECK_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES =
 USE_COMPUTED_HEADER_DEPENDENCIES =
-- 
1.7.5

-- 
  Kacper Kornet

Re: [PATCH] Respect definition of prefix from autotools in ETC_GITCONFIG and ETC_GITATTRIBUTES

[Junio C Hamano]

Kacper Kornet <kornet@camk.edu.pl> writes:

> Definitions of ETC_GITCONFIG and ETC_GITATTRIBUTES depend on value of
> prefix. As prefix can be changed in config.mak.autogen, all if blocks
> with conditions based on prefix should be placed after the file is
> included in Makefile.

This is _not_ just about autogen, is it?  The same issue exists if the
user wants to manually tweak prefix in config.mak, no?

If so, perhaps the patch needs to be retitled to avoid confusion,
something like:

    Subject: Honor $(prefix) set in config.mak* when defining ETC_GIT* variables

> diff --git a/Makefile b/Makefile
> index cbc3fce..5b4ae40 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -291,15 +291,8 @@ sharedir = $(prefix)/share
>  gitwebdir = $(sharedir)/gitweb
>  template_dir = share/git-core/templates
>  htmldir = share/doc/git-doc
> -ifeq ($(prefix),/usr)
> -sysconfdir = /etc
>  ETC_GITCONFIG = $(sysconfdir)/gitconfig
>  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> -else
> -sysconfdir = $(prefix)/etc
> -ETC_GITCONFIG = etc/gitconfig
> -ETC_GITATTRIBUTES = etc/gitattributes
> -endif
>  lib = lib
>  # DESTDIR=
>  pathsep = :
> @@ -1192,6 +1185,12 @@ endif
>  -include config.mak.autogen
>  -include config.mak
>  
> +ifeq ($(prefix),/usr)
> +sysconfdir = /etc
> +else
> +sysconfdir = etc
> +endif

It makes sense to change the definition of ETC_GIT* variables to a form
that depends on a variable like your patch did, i.e.

    ETC_GITCONFIG = $(some_etc_prefix)/gitconfig
    ETC_GITATTRIBUTES = $(some_etc_prefix)/gitattributes

and define that variable, whose definition depends on $(prefix), after we
have read config.mak* files.  So I like the general direction of this
patch.

But this part in the Makefile outside the context of the patch bothers
me.  It seems to imply that sysconfdir is _not_ that variable you want to
define later.

   # Among the variables below, these:
   #   gitexecdir
   #   template_dir
   #   mandir
   #   infodir
   #   htmldir
   #   ETC_GITCONFIG (but not sysconfdir)
   #   ETC_GITATTRIBUTES
   # can be specified as a relative path some/where/else;

So I have a suspicion that your patch as is will break when prefix is set
to something other than /usr directory.  I don't think anybody in-tree
currently uses sysconfdir, but that does not mean nobody will ever do.

Re: [PATCH] Respect definition of prefix from autotools in ETC_GITCONFIG and ETC_GITATTRIBUTES

[Kacper Kornet]

On Thu, Apr 28, 2011 at 09:54:02AM -0700, Junio C Hamano wrote:
> Kacper Kornet <kornet@camk.edu.pl> writes:

> > Definitions of ETC_GITCONFIG and ETC_GITATTRIBUTES depend on value of
> > prefix. As prefix can be changed in config.mak.autogen, all if blocks
> > with conditions based on prefix should be placed after the file is
> > included in Makefile.

> This is _not_ just about autogen, is it?  The same issue exists if the
> user wants to manually tweak prefix in config.mak, no?

> If so, perhaps the patch needs to be retitled to avoid confusion,
> something like:

>     Subject: Honor $(prefix) set in config.mak* when defining ETC_GIT* variables

You are right.

> > diff --git a/Makefile b/Makefile
> > index cbc3fce..5b4ae40 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -291,15 +291,8 @@ sharedir = $(prefix)/share
> >  gitwebdir = $(sharedir)/gitweb
> >  template_dir = share/git-core/templates
> >  htmldir = share/doc/git-doc
> > -ifeq ($(prefix),/usr)
> > -sysconfdir = /etc
> >  ETC_GITCONFIG = $(sysconfdir)/gitconfig
> >  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> > -else
> > -sysconfdir = $(prefix)/etc
> > -ETC_GITCONFIG = etc/gitconfig
> > -ETC_GITATTRIBUTES = etc/gitattributes
> > -endif
> >  lib = lib
> >  # DESTDIR=
> >  pathsep = :
> > @@ -1192,6 +1185,12 @@ endif
> >  -include config.mak.autogen
> >  -include config.mak

> > +ifeq ($(prefix),/usr)
> > +sysconfdir = /etc
> > +else
> > +sysconfdir = etc
> > +endif

Actually I have made a mistake here. I meant the last hunk to be:

@@ -1192,6 +1185,12 @@ endif
 -include config.mak.autogen
 -include config.mak

+ifeq ($(prefix),/usr)
+sysconfdir = /etc
+else
+sysconfdir = $(prefix)/etc
+endif

> But this part in the Makefile outside the context of the patch bothers
> me.  It seems to imply that sysconfdir is _not_ that variable you want to
> define later.

>    # Among the variables below, these:
>    #   gitexecdir
>    #   template_dir
>    #   mandir
>    #   infodir
>    #   htmldir
>    #   ETC_GITCONFIG (but not sysconfdir)
>    #   ETC_GITATTRIBUTES
>    # can be specified as a relative path some/where/else;

> So I have a suspicion that your patch as is will break when prefix is set
> to something other than /usr directory.  I don't think anybody in-tree
> currently uses sysconfdir, but that does not mean nobody will ever do.

See the corrected hunk above. I will be prepare the corrected patch.

-- 
  Kacper Kornet

[PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

Definitions of ETC_GITCONFIG, ETC_GITATTRIBUTES and sysconfdir depend on
value of prefix. As prefix can be changed in config.mak.autogen, all if
blocks with conditions based on prefix should be placed after the file
is included in Makefile.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Makefile |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index cbc3fce..bf912b9 100644
--- a/Makefile
+++ b/Makefile
@@ -291,15 +291,8 @@ sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
-ifeq ($(prefix),/usr)
-sysconfdir = /etc
-ETC_GITCONFIG = $(sysconfdir)/gitconfig
-ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
-else
-sysconfdir = $(prefix)/etc
-ETC_GITCONFIG = etc/gitconfig
-ETC_GITATTRIBUTES = etc/gitattributes
-endif
+ETC_GITCONFIG = $(git_etcdir)/gitconfig
+ETC_GITATTRIBUTES = $(git_etcdir)/gitattributes
 lib = lib
 # DESTDIR=
 pathsep = :
@@ -1192,6 +1185,14 @@ endif
 -include config.mak.autogen
 -include config.mak
 
+ifeq ($(prefix),/usr)
+sysconfdir = /etc
+git_etcdir = /etc
+else
+sysconfdir = $(prefix)/etc
+git_etcdir = etc
+endif
+
 ifdef CHECK_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES =
 USE_COMPUTED_HEADER_DEPENDENCIES =
-- 
1.7.5


-- 
  Kacper Kornet

[PATCH] Honor sysconfdir when set as an configure option

[Kacper Kornet]

./configure can be run with option --sysconfdir=...
and Makefile should respect that choice.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Makefile      |    8 ++++----
 config.mak.in |    2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index bf912b9..9d6cffa 100644
--- a/Makefile
+++ b/Makefile
@@ -1186,11 +1186,11 @@ endif
 -include config.mak
 
 ifeq ($(prefix),/usr)
-sysconfdir = /etc
-git_etcdir = /etc
+sysconfdir ?= /etc
+git_etcdir ?= /etc
 else
-sysconfdir = $(prefix)/etc
-git_etcdir = etc
+sysconfdir ?= $(prefix)/etc
+git_etcdir ?= etc
 endif
 
 ifdef CHECK_HEADER_DEPENDENCIES
diff --git a/config.mak.in b/config.mak.in
index e378534..ac5912d 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -15,6 +15,8 @@ TCLTK_PATH = @TCLTK_PATH@
 prefix = @prefix@
 exec_prefix = @exec_prefix@
 bindir = @bindir@
+sysconfdir = @sysconfdir@
+git_etcdir = @sysconfdir@
 gitexecdir = @libexecdir@/git-core
 datarootdir = @datarootdir@
 template_dir = @datadir@/git-core/templates
-- 
1.7.5

-- 
  Kacper Kornet

Re: [PATCH] Honor sysconfdir when set as an configure option

[Junio C Hamano]

Kacper Kornet <kornet@camk.edu.pl> writes:

> ./configure can be run with option --sysconfdir=...
> and Makefile should respect that choice.

Thanks.  That statement is not wrong per-se, but your patch will result in
git_etcdir always set to the same as $(sysconfdir) that is an absolute
path, which in turn would mean that the build product with prefix set to
something other than /usr won't relocate well, no?

Not that I personally deeply care about it, but I understand msysgit folks
spent considerable amount of brainpower to come up with the arrangement,
so they may want to have a say on this patch.

"git shortlog -n --no-merges --grep=msys --grep=Win -- Makefile" tells
me that Steffen Prohaska and J6t are the people to whom I need to Cc:
this.

>  ifeq ($(prefix),/usr)
> -sysconfdir = /etc
> -git_etcdir = /etc
> +sysconfdir ?= /etc
> +git_etcdir ?= /etc
>  else
> -sysconfdir = $(prefix)/etc
> -git_etcdir = etc
> +sysconfdir ?= $(prefix)/etc
> +git_etcdir ?= etc
>  endif
>  
>  ifdef CHECK_HEADER_DEPENDENCIES
> diff --git a/config.mak.in b/config.mak.in
> index e378534..ac5912d 100644
> --- a/config.mak.in
> +++ b/config.mak.in
> @@ -15,6 +15,8 @@ TCLTK_PATH = @TCLTK_PATH@
>  prefix = @prefix@
>  exec_prefix = @exec_prefix@
>  bindir = @bindir@
> +sysconfdir = @sysconfdir@
> +git_etcdir = @sysconfdir@
>  gitexecdir = @libexecdir@/git-core
>  datarootdir = @datarootdir@
>  template_dir = @datadir@/git-core/templates
> -- 
> 1.7.5

Re: [PATCH] Honor sysconfdir when set as an configure option

[Kacper Kornet]

On Thu, Apr 28, 2011 at 02:05:09PM -0700, Junio C Hamano wrote:
> Kacper Kornet <kornet@camk.edu.pl> writes:

> > ./configure can be run with option --sysconfdir=...
> > and Makefile should respect that choice.

> Thanks.  That statement is not wrong per-se, but your patch will result in
> git_etcdir always set to the same as $(sysconfdir) that is an absolute
> path, which in turn would mean that the build product with prefix set to
> something other than /usr won't relocate well, no?

Only when ./configure is used. But autotools do not support relative
paths, as far as I know. When autotools are not used, it is still
possible to have relative path in git_etcdir.

-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Please do not set Mail-Followup-To!! It makes communication on this list
extremely inconvenient.

Am 4/28/2011 21:27, schrieb Kacper Kornet:
> Definitions of ETC_GITCONFIG, ETC_GITATTRIBUTES and sysconfdir depend on
> value of prefix. As prefix can be changed in config.mak.autogen, all if
> blocks with conditions based on prefix should be placed after the file
> is included in Makefile.
> 
> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
> ---
>  Makefile |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index cbc3fce..bf912b9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -291,15 +291,8 @@ sharedir = $(prefix)/share
>  gitwebdir = $(sharedir)/gitweb
>  template_dir = share/git-core/templates
>  htmldir = share/doc/git-doc
> -ifeq ($(prefix),/usr)
> -sysconfdir = /etc
> -ETC_GITCONFIG = $(sysconfdir)/gitconfig
> -ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> -else
> -sysconfdir = $(prefix)/etc
> -ETC_GITCONFIG = etc/gitconfig
> -ETC_GITATTRIBUTES = etc/gitattributes
> -endif
> +ETC_GITCONFIG = $(git_etcdir)/gitconfig
> +ETC_GITATTRIBUTES = $(git_etcdir)/gitattributes
>  lib = lib
>  # DESTDIR=
>  pathsep = :
> @@ -1192,6 +1185,14 @@ endif
>  -include config.mak.autogen
>  -include config.mak
>  
> +ifeq ($(prefix),/usr)
> +sysconfdir = /etc
> +git_etcdir = /etc
> +else
> +sysconfdir = $(prefix)/etc
> +git_etcdir = etc
> +endif
> +
>  ifdef CHECK_HEADER_DEPENDENCIES
>  COMPUTE_HEADER_DEPENDENCIES =
>  USE_COMPUTED_HEADER_DEPENDENCIES =

Does this patch do anything useful? After the patch is applied, sysconfdir
is set-but-not-used. Therefore, you can remove the assignments. But then
you lose the reference to $(prefix) that the commit message claims is so
important. Puzzled...

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Please do not set Mail-Followup-To!! It makes communication on this list
> extremely inconvenient.
>
> Am 4/28/2011 21:27, schrieb Kacper Kornet:
>> Definitions of ETC_GITCONFIG, ETC_GITATTRIBUTES and sysconfdir depend on
>> value of prefix. As prefix can be changed in config.mak.autogen, all if
>> blocks with conditions based on prefix should be placed after the file
>> is included in Makefile.
> ...
> Does this patch do anything useful? After the patch is applied, sysconfdir
> is set-but-not-used. Therefore, you can remove the assignments. But then
> you lose the reference to $(prefix) that the commit message claims is so
> important. Puzzled...

The only thing it does is to to allow you to set prefix in config.mak and
then have it propaget to the selection of ETC_GITCONFIG (if prefix is /usr,
then it is always /etc/gitconfig, otherwise it is always etc/gitconfig).
The importance of prefix is not that the value is prefixed to ETC_GIT*,
but it is used in the conditional to choose between the two.

We can get rid of assignments to sysconfdir in that sense. But you spotted
a regression. If sysconfdir is set to somewhere else, even if you set prefix
to /usr, we should set ETC_GIT* using the value given to sysconfdir.  The
original code did so, but the patch lost it.

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Am 5/3/2011 19:32, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Please do not set Mail-Followup-To!! It makes communication on this list
>> extremely inconvenient.
>>
>> Am 4/28/2011 21:27, schrieb Kacper Kornet:
>>> Definitions of ETC_GITCONFIG, ETC_GITATTRIBUTES and sysconfdir depend on
>>> value of prefix. As prefix can be changed in config.mak.autogen, all if
>>> blocks with conditions based on prefix should be placed after the file
>>> is included in Makefile.
>> ...
>> Does this patch do anything useful? After the patch is applied, sysconfdir
>> is set-but-not-used. Therefore, you can remove the assignments. But then
>> you lose the reference to $(prefix) that the commit message claims is so
>> important. Puzzled...
> 
> The only thing it does is to to allow you to set prefix in config.mak and
> then have it propaget to the selection of ETC_GITCONFIG (if prefix is /usr,
> then it is always /etc/gitconfig, otherwise it is always etc/gitconfig).
> The importance of prefix is not that the value is prefixed to ETC_GIT*,
> but it is used in the conditional to choose between the two.

Fair enough.

> We can get rid of assignments to sysconfdir in that sense. But you spotted
> a regression. If sysconfdir is set to somewhere else, even if you set prefix
> to /usr, we should set ETC_GIT* using the value given to sysconfdir.  The
> original code did so, but the patch lost it.

Looking closer, the patch introduces git_etcdir for no good reason, IIUC.
It should just re-use sysconfdir (the meaning of this variable is to point
to the etc directory).

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Wed, May 04, 2011 at 07:52:30AM +0200, Johannes Sixt wrote:
> Looking closer, the patch introduces git_etcdir for no good reason,
> IIUC.
> It should just re-use sysconfdir (the meaning of this variable is to
> point
> to the etc directory).

And the first version of my patch did it. However Junio has written:

> But this part in the Makefile outside the context of the patch bothers
> me.  It seems to imply that sysconfdir is _not_ that variable you want
> to
> define later.
>
>    # Among the variables below, these:
>    #   gitexecdir
>    #   template_dir
>    #   mandir
>    #   infodir
>    #   htmldir
>    #   ETC_GITCONFIG (but not sysconfdir)
>    #   ETC_GITATTRIBUTES
>    # can be specified as a relative path some/where/else;
>
> So I have a suspicion that your patch as is will break when prefix is
> set
> to something other than /usr directory.  I don't think anybody in-tree
> currently uses sysconfdir, but that does not mean nobody will ever do.

>From that I understood that he prefers sysconfdir to be always an
absolute path.
-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Am 5/4/2011 15:58, schrieb Kacper Kornet:
> On Wed, May 04, 2011 at 07:52:30AM +0200, Johannes Sixt wrote:
>> Looking closer, the patch introduces git_etcdir for no good reason,
>> IIUC.
>> It should just re-use sysconfdir (the meaning of this variable is to
>> point
>> to the etc directory).
> 
> And the first version of my patch did it. However Junio has written:
> 
>> But this part in the Makefile outside the context of the patch bothers
>> me.  It seems to imply that sysconfdir is _not_ that variable you want
>> to
>> define later.
>>
>>    # Among the variables below, these:
>>    #   gitexecdir
>>    #   template_dir
>>    #   mandir
>>    #   infodir
>>    #   htmldir
>>    #   ETC_GITCONFIG (but not sysconfdir)
>>    #   ETC_GITATTRIBUTES
>>    # can be specified as a relative path some/where/else;
>>
>> So I have a suspicion that your patch as is will break when prefix is
>> set
>> to something other than /usr directory.  I don't think anybody in-tree
>> currently uses sysconfdir, but that does not mean nobody will ever do.
> 
>>From that I understood that he prefers sysconfdir to be always an
> absolute path.

Junio's worries should not be discarded lightly. But in this case they are
unfounded. Digging the history shows:

b51b8bbf (Create a sysconfdir variable, and use it for ETC_GITCONFIG,
2007-04-24) introduced the variable to be able to treat the special case
where prefix == /usr. It was never intended as a user-settable value.

In 49fa65a7 (Allow the built-in exec path to be relative to the command
invocation path, 2008-07-23), I added the comment above because at that
time, a relocatable build should be requested by setting ETC_GITCONFIG to
a relative path, but not by changing sysconfdir. (The comment sounds as if
the user can set sysconfdir, but I did not intend to say that.)

026fa0d5 (Move computation of absolute paths from Makefile to runtime (in
preparation for RUNTIME_PREFIX), 2009-01-18) practically obsoleted
sysconfdir. In particular, it removed one of the cases where the value of
sysconfdir mattered, leaving only the reference where it is guaranteed to
be set to /etc. This commit could have removed sysconfdir entirely.

6df42ab9 (Add global and system-wide gitattributes, 2010-09-01) added
another consumer of sysconfdir, but in the same spirit as ETC_GITCONFIG.

So, I don't think that sysconfdir must survive. It was always only a
helper variable to shorten the code.

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio's worries should not be discarded lightly. But in this case they are
> unfounded.

I appreciate the analysis and explanation.  As you can see from the
exchange, I was only basing my suggestion on the comments left in the
Makefile by you when you did the "relocatable installation" topic.

> So, I don't think that sysconfdir must survive. It was always only a
> helper variable to shorten the code.

My remaining worry was if ./configure users accidentally relied on this
internal use of a variable whose name happens to be sysconfdir.  If so,

    $ ./configure --sysconfdir=/some/where

may give them a different result once we get rid of sysconfdir from our
Makefile.

But I notice that we do not say that sysconfdir is to be replaced in
config.mak.in we ship, so such a command line option would have been
silently discarded anyway.  IOW, no ./configure user would have relied on
what our build procedure does.

BUT.

The vanilla Makefile users might have.  Among those who install with
prefix=$HOME/git, there may be people who do not want to use $HOME/git/etc
and an obvious way to do so is by setting sysconfdir to $HOME/etc (an
alternative would be to set both ETC_GITCONFIG and ETC_GITATTRIBUTES).

So I think it would probably be a less-impact and useful solution to keep
sysconfdir and add "sysconfdir = @sysconfdir@" to config.mak.in as well.

Is Kacper's latest patch with an obvious one-liner to config.mak.in
sufficient to achieve that?

  From: Kacper Kornet <kornet@camk.edu.pl>
  Subject: Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defin...
  Date: Wed, 4 May 2011 16:29:21 +0200
  Message-ID: <20110504142921.GE18585@camk.edu.pl>

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So I think it would probably be a less-impact and useful solution to keep
> sysconfdir and add "sysconfdir = @sysconfdir@" to config.mak.in as well.
>
> Is Kacper's latest patch with an obvious one-liner to config.mak.in
> sufficient to achieve that?
>
>   From: Kacper Kornet <kornet@camk.edu.pl>
>   Subject: Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defin...
>   Date: Wed, 4 May 2011 16:29:21 +0200
>   Message-ID: <20110504142921.GE18585@camk.edu.pl>

In other words, this one on top of the above (which defaults sysconfdir
to /etc when $(prefix) is /usr and then sets git_etcdir to $(sysconfdir)).

-- >8 --
Subject: [PATCH] config.mak.in: allow "configure --sysconfdir=/else/where"

We do allow vanilla Makefile users to say make sysconfdir=/else/where
and config.mak can also be tweaked manually for the same effect. Give
the same configurablity to ./configure users as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.in |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index 9614973..dd8f274 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -18,6 +18,7 @@ bindir = @bindir@
 gitexecdir = @libexecdir@/git-core
 datarootdir = @datarootdir@
 template_dir = @datadir@/git-core/templates
+sysconfdir = @sysconfdir@
 
 mandir=@mandir@
 
-- 
1.7.5.1.242.g2ec223

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Am 5/5/2011 4:26, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> So I think it would probably be a less-impact and useful solution to keep
>> sysconfdir and add "sysconfdir = @sysconfdir@" to config.mak.in as well.
>>
>> Is Kacper's latest patch with an obvious one-liner to config.mak.in
>> sufficient to achieve that?
>>
>>   From: Kacper Kornet <kornet@camk.edu.pl>
>>   Subject: Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defin...
>>   Date: Wed, 4 May 2011 16:29:21 +0200
>>   Message-ID: <20110504142921.GE18585@camk.edu.pl>
> 
> In other words, this one on top of the above (which defaults sysconfdir
> to /etc when $(prefix) is /usr and then sets git_etcdir to $(sysconfdir)).
> 
> -- >8 --
> Subject: [PATCH] config.mak.in: allow "configure --sysconfdir=/else/where"
> 
> We do allow vanilla Makefile users to say make sysconfdir=/else/where
> and config.mak can also be tweaked manually for the same effect. Give
> the same configurablity to ./configure users as well.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.mak.in |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/config.mak.in b/config.mak.in
> index 9614973..dd8f274 100644
> --- a/config.mak.in
> +++ b/config.mak.in
> @@ -18,6 +18,7 @@ bindir = @bindir@
>  gitexecdir = @libexecdir@/git-core
>  datarootdir = @datarootdir@
>  template_dir = @datadir@/git-core/templates
> +sysconfdir = @sysconfdir@
>  
>  mandir=@mandir@
>  

No, that's not sufficient. Notice that $(sysconfdir) is used for ETC_GIT*
variables *only* if $(prefix) == /usr (both before and after Kacper's
patch). Therefore, you won't gain a lot of configurability via sysconfdir;
you have to change ETC_GIT* variables directly.

I'm not opposed to keep sysconfdir at all if it gains a useful purpose
like with the oneliner above. But extra work is needed in Makefile; if
this doesn't materialize, I suggest you back out Kacper's patch from 'next'.

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> No, that's not sufficient. Notice that $(sysconfdir) is used for ETC_GIT*
> variables *only* if $(prefix) == /usr (both before and after Kacper's
> patch). Therefore, you won't gain a lot of configurability via sysconfdir;
> you have to change ETC_GIT* variables directly.

Huh?  You lost me.

The point of this approach is to make a progress without regressing, and
the (minimum) progress needed to be made that comes from the beginning of
this discussion is that setting $(prefix) from the command line of make
works but it does not when it is set in config.mak (or config.mak.autogen
which in turn is added by running "./configure --prefix=...").

As the "relocatable installation" topic wants to make everything relative
to prefix, I do not know if it is even worth trying to support a build
that has prefix set to anything other than /usr to set ETC_GIT* variables
to an arbitrary absolute path via --sysconfdir. I suspect that would
defeat the whole "relocatable" concept. More importantly, since I do not
care too much about "relocatable", I do not want to even have to worry
about it.

So the only thing my suggested approach tries to fix on top of Kacper's
patch is not to regress use of "sysconfdir = /some/where" when prefix is
set to /usr.

If you really care about being able to use sysconfdir together with the
relocate logic, I think you first have to rethink "if $(prefix) == /usr"
and change it into a new "RELOCATABLE_GIT = YesPlease/NoThanks".  After
all, even when you set prefix to /usr/local, you may want the installed
binaries know where to look without the "let's find everything relative
to /usr/local/bin/git" logic.

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Am 5/5/2011 18:17, schrieb Junio C Hamano:
> The point of this approach is to make a progress without regressing, and
> the (minimum) progress needed to be made that comes from the beginning of
> this discussion is that setting $(prefix) from the command line of make
> works but it does not when it is set in config.mak (or config.mak.autogen
> which in turn is added by running "./configure --prefix=...").

Ah! That's a piece of informatin that I was missing.

To fix that, wouldn't it have been sufficient to just move the
ifeq($(prefix),/usr) conditional past -include config.mak, without any
other changes?

(But I'm mostly clueless about when which make variables from what source
override what.)

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Wed, May 04, 2011 at 07:26:29PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:

> > So I think it would probably be a less-impact and useful solution to keep
> > sysconfdir and add "sysconfdir = @sysconfdir@" to config.mak.in as well.

> > Is Kacper's latest patch with an obvious one-liner to config.mak.in
> > sufficient to achieve that?

> >   From: Kacper Kornet <kornet@camk.edu.pl>
> >   Subject: Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defin...
> >   Date: Wed, 4 May 2011 16:29:21 +0200
> >   Message-ID: <20110504142921.GE18585@camk.edu.pl>

> In other words, this one on top of the above (which defaults sysconfdir
> to /etc when $(prefix) is /usr and then sets git_etcdir to $(sysconfdir)).

There is one more problem with this. If you call:

 ./configure --prefix=/usr --sysconfdir=/<dir>

sysconfdir and git_etcdir are set to /etc not /<dir>. I admit that it is
a rather unusual set of options ./configure, but maybe it should be
supported. So maybe I will first describe how it should all work in my
opinion and I will wait for comments before I will try to implement it.

1. ./configure --prefix=dir1  and dir1 != /usr

    git_etcdir = etc

2. ./configure --prefix=/usr

    git_etcdir = /etc

3. ./configure --prefix=dir1 --syscondir=dir2 (here dir2 has to be an
absolute path)

    git_etcdir =  dir2

4. make prefix=dir1 and dir1 != /usr

    git_etcdir = etc

5. make prefix=/usr

    git_etcdir = /etc

6. make prefix=dir1 sysconfdir=dir2 (here dir2 can be an absolute or
a relative path)

    git_etcdir = dir2


-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

A:

> 1. ./configure --prefix=dir1  and dir1 != /usr
> 
>     git_etcdir = etc
> 
> 2. ./configure --prefix=/usr
> 
>     git_etcdir = /etc
> 
> 3. ./configure --prefix=dir1 --syscondir=dir2 (here dir2 has to be an
> absolute path)
> 
>     git_etcdir =  dir2

Why do you have the restriction that --sysconfdir must be absolute? Is it
imposed by autotools?

  4. ./configure --sysconfdir=dir2

     git_etcdir =  dir2

B:

> 1. make prefix=dir1 and dir1 != /usr
> 
>     git_etcdir = etc
> 
> 2. make prefix=/usr
> 
>     git_etcdir = /etc
> 
> 3. make prefix=dir1 sysconfdir=dir2 (here dir2 can be an absolute or
> a relative path)
> 
>     git_etcdir = dir2

  4. make sysconfdir=dir2

      git_etcdir = dir2

C:
in config.mak write none, one, or both of

  prefix=dir1
  sysconfdir=dir2

with the same 4 case distinctions and corresponding desired results as in B.

Looks fine and simple to implement, except that I don't see that you have
to introduce git_etcdir; can't you just stick with the name "sysconfdir"?

-- Hannes

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Thu, May 05, 2011 at 04:45:40PM +0200, Johannes Sixt wrote:
> A:

> > 1. ./configure --prefix=dir1  and dir1 != /usr

> >     git_etcdir = etc

> > 2. ./configure --prefix=/usr

> >     git_etcdir = /etc

> > 3. ./configure --prefix=dir1 --syscondir=dir2 (here dir2 has to be
> > an
> > absolute path)

> >     git_etcdir =  dir2

> Why do you have the restriction that --sysconfdir must be absolute? Is
> it
> imposed by autotools?

Yes, configure does not accept relative pathnames.

>   4. ./configure --sysconfdir=dir2

>      git_etcdir =  dir2

> B:

> > 1. make prefix=dir1 and dir1 != /usr

> >     git_etcdir = etc

> > 2. make prefix=/usr

> >     git_etcdir = /etc

> > 3. make prefix=dir1 sysconfdir=dir2 (here dir2 can be an absolute or
> > a relative path)

> >     git_etcdir = dir2

>   4. make sysconfdir=dir2

>       git_etcdir = dir2

> C:
> in config.mak write none, one, or both of

>   prefix=dir1
>   sysconfdir=dir2

> with the same 4 case distinctions and corresponding desired results as
> in B.

> Looks fine and simple to implement, except that I don't see that you
> have
> to introduce git_etcdir; can't you just stick with the name
> "sysconfdir"?

I can do it, if Junio is ok with it.

Should the patch be based on main or on the previous patch that was
merged to next?

-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Junio C Hamano]

Kacper Kornet <kornet@camk.edu.pl> writes:

> Should the patch be based on main or on the previous patch that was
> merged to next?

After having this much discussion, it is preferrable to have a patch
relative to what you have in 'next' (2910bf5) as a fix-up, explaining "The
initial version does not cover these cases / has these problems" to
summarize the discussion so far, followed by explanation of the
incremental change "Fix this and that by doing ...".

We could revert what is in 'next' and start from scratch, but then it is
likely that the thought process will be lost when you write the log
message.

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Johannes Sixt]

Am 5/5/2011 17:46, schrieb Junio C Hamano:
> Kacper Kornet <kornet@camk.edu.pl> writes:
> 
>> Should the patch be based on main or on the previous patch that was
>> merged to next?
> 
> After having this much discussion, it is preferrable to have a patch
> relative to what you have in 'next' (2910bf5) as a fix-up, explaining "The
> initial version does not cover these cases / has these problems" to
> summarize the discussion so far, followed by explanation of the
> incremental change "Fix this and that by doing ...".
> 
> We could revert what is in 'next' and start from scratch, but then it is
> likely that the thought process will be lost when you write the log
> message.

I'd implement it like this, discarding Kacper's patch, but I'm not sure
whether the commit message summarizes the discussion sufficiently. Let me
know whether you want a version based on top of Kacper's patch, or how
else to proceed.

Of your two patches regarding configure --sysconfdir, only the second
(e8de44bc05) is needed on top of this patch.

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] Honor $(prefix) set in config.mak* when defining ETC_GIT*

Notice that the prefix specified for the build influenced the definitions
of ETC_GITCONFIG and ETC_GITATTRIBUTES only when it was exactly '/usr'.
Kacper Kornet noticed that this was furthermore only the case when the
build was triggered using 'make prefix=/usr', i.e., the prefix was given
on the command line; it did not work when the prefix was specified in
config.mak because this file is included much later in the Makefile.

To fix this, move the conditional after the inclusion of config.mak.

Additionally, it is desirable to specify the etc directory for a build
(for example, a build with prefix /usr/local may still want to have the
system configuration in /etc/gitconfig). For this purpose, promote the
variable 'sysconfdir' from a helper variable to a configuration
variable. The prefix check that was moved must now be wrapped so that it
does not override sysconfdir setting given in config.mak.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Makefile |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index ca4b38e..10d6bd5 100644
--- a/Makefile
+++ b/Makefile
@@ -274,8 +274,7 @@ STRIP ?= strip
 #   mandir
 #   infodir
 #   htmldir
-#   ETC_GITCONFIG (but not sysconfdir)
-#   ETC_GITATTRIBUTES
+#   sysconfdir
 # can be specified as a relative path some/where/else;
 # this is interpreted as relative to $(prefix) and "git" at
 # runtime figures out where they are based on the path to the executable.
@@ -291,15 +290,8 @@ sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
-ifeq ($(prefix),/usr)
-sysconfdir = /etc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig
 ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
-else
-sysconfdir = $(prefix)/etc
-ETC_GITCONFIG = etc/gitconfig
-ETC_GITATTRIBUTES = etc/gitattributes
-endif
 lib = lib
 # DESTDIR=
 pathsep = :
@@ -1192,6 +1184,14 @@ endif
 -include config.mak.autogen
 -include config.mak
 
+ifndef sysconfdir
+ifeq ($(prefix),/usr)
+sysconfdir = /etc
+else
+sysconfdir = etc
+endif
+endif
+
 ifdef CHECK_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES =
 USE_COMPUTED_HEADER_DEPENDENCIES =
-- 
1.7.4.1.317.g0b25

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Mon, May 09, 2011 at 10:24:55AM +0200, Johannes Sixt wrote:
> --- 8< ---
> From: Johannes Sixt <j6t@kdbg.org>
> Subject: [PATCH] Honor $(prefix) set in config.mak* when defining ETC_GIT*

> Notice that the prefix specified for the build influenced the definitions
> of ETC_GITCONFIG and ETC_GITATTRIBUTES only when it was exactly '/usr'.
> Kacper Kornet noticed that this was furthermore only the case when the
> build was triggered using 'make prefix=/usr', i.e., the prefix was given
> on the command line; it did not work when the prefix was specified in
> config.mak because this file is included much later in the Makefile.

> To fix this, move the conditional after the inclusion of config.mak.

> Additionally, it is desirable to specify the etc directory for a build
> (for example, a build with prefix /usr/local may still want to have the
> system configuration in /etc/gitconfig). For this purpose, promote the
> variable 'sysconfdir' from a helper variable to a configuration
> variable. The prefix check that was moved must now be wrapped so that it
> does not override sysconfdir setting given in config.mak.

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  Makefile |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)

> diff --git a/Makefile b/Makefile
> index ca4b38e..10d6bd5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -274,8 +274,7 @@ STRIP ?= strip
>  #   mandir
>  #   infodir
>  #   htmldir
> -#   ETC_GITCONFIG (but not sysconfdir)
> -#   ETC_GITATTRIBUTES
> +#   sysconfdir
>  # can be specified as a relative path some/where/else;
>  # this is interpreted as relative to $(prefix) and "git" at
>  # runtime figures out where they are based on the path to the executable.
> @@ -291,15 +290,8 @@ sharedir = $(prefix)/share
>  gitwebdir = $(sharedir)/gitweb
>  template_dir = share/git-core/templates
>  htmldir = share/doc/git-doc
> -ifeq ($(prefix),/usr)
> -sysconfdir = /etc
>  ETC_GITCONFIG = $(sysconfdir)/gitconfig
>  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
> -else
> -sysconfdir = $(prefix)/etc
> -ETC_GITCONFIG = etc/gitconfig
> -ETC_GITATTRIBUTES = etc/gitattributes
> -endif
>  lib = lib
>  # DESTDIR=
>  pathsep = :
> @@ -1192,6 +1184,14 @@ endif
>  -include config.mak.autogen
>  -include config.mak

> +ifndef sysconfdir
> +ifeq ($(prefix),/usr)
> +sysconfdir = /etc
> +else
> +sysconfdir = etc
> +endif
> +endif
> +
>  ifdef CHECK_HEADER_DEPENDENCIES
>  COMPUTE_HEADER_DEPENDENCIES =
>  USE_COMPUTED_HEADER_DEPENDENCIES =

For me it looks all right. Thanks for writing the patch instead of me. 
-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Mon, May 09, 2011 at 10:24:55AM +0200, Johannes Sixt wrote:
> Am 5/5/2011 17:46, schrieb Junio C Hamano:
> > Kacper Kornet <kornet@camk.edu.pl> writes:

> >> Should the patch be based on main or on the previous patch that was
> >> merged to next?

> > After having this much discussion, it is preferrable to have a patch
> > relative to what you have in 'next' (2910bf5) as a fix-up, explaining "The
> > initial version does not cover these cases / has these problems" to
> > summarize the discussion so far, followed by explanation of the
> > incremental change "Fix this and that by doing ...".

> > We could revert what is in 'next' and start from scratch, but then it is
> > likely that the thought process will be lost when you write the log
> > message.

> I'd implement it like this, discarding Kacper's patch, but I'm not sure
> whether the commit message summarizes the discussion sufficiently. Let me
> know whether you want a version based on top of Kacper's patch, or how
> else to proceed.

> Of your two patches regarding configure --sysconfdir, only the second
> (e8de44bc05) is needed on top of this patch.

I'm afraid e8de44bc05 was lost somewhere, as 1.7.5.3 does not contain
it. 

-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On second thought, I think the point A.1 and A.2 has to be changed and
merged to one:

On Thu, May 05, 2011 at 04:45:40PM +0200, Johannes Sixt wrote:
> A:

> > 1. ./configure --prefix=dir1  and dir1 != /usr

> >     git_etcdir = etc

> > 2. ./configure --prefix=/usr

> >     git_etcdir = /etc

1. ./configure --prefix=dir1

      git_etcdir = /dir1/etc

Otherwise there would be a conflict with:

./configure --help
    --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]

-- 
  Kacper Kornet

Re: [PATCH 1/1] Honor $(prefix) set in config.mak* when defining ETC_GIT* and sysconfdir

[Kacper Kornet]

On Tue, May 03, 2011 at 10:32:14AM -0700, Junio C Hamano wrote:
> We can get rid of assignments to sysconfdir in that sense. But you spotted
> a regression. If sysconfdir is set to somewhere else, even if you set prefix
> to /usr, we should set ETC_GIT* using the value given to sysconfdir.  The
> original code did so, but the patch lost it.

Actually syconfdir was respected only when prefix was /usr. When it was
not /usr, ETC_GIT* were always set to etc/... The following version of
patch reproduces that behaviour:

>From 1a438bc0ac71a96398260b73b3c24c5e752a02f5 Mon Sep 17 00:00:00 2001
From: Kacper Kornet <draenog@pld-linux.org>
Date: Thu, 28 Apr 2011 02:42:48 +0100
Subject: [PATCH] Honor $(prefix) set in config.mak* when defining ETC_GIT*
 and sysconfdir

Definitions of ETC_GITCONFIG, ETC_GITATTRIBUTES and sysconfdir depend on
value of prefix. As prefix can be changed in config.mak.autogen, all if
blocks with conditions based on prefix should be placed after the file
is included in Makefile.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Makefile |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index cbc3fce..31b558e 100644
--- a/Makefile
+++ b/Makefile
@@ -291,15 +291,8 @@ sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
-ifeq ($(prefix),/usr)
-sysconfdir = /etc
-ETC_GITCONFIG = $(sysconfdir)/gitconfig
-ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
-else
-sysconfdir = $(prefix)/etc
-ETC_GITCONFIG = etc/gitconfig
-ETC_GITATTRIBUTES = etc/gitattributes
-endif
+ETC_GITCONFIG = $(git_etcdir)/gitconfig
+ETC_GITATTRIBUTES = $(git_etcdir)/gitattributes
 lib = lib
 # DESTDIR=
 pathsep = :
@@ -1192,6 +1185,14 @@ endif
 -include config.mak.autogen
 -include config.mak
 
+ifeq ($(prefix),/usr)
+sysconfdir = /etc
+git_etcdir = $(sysconfdir)
+else
+sysconfdir = $(prefix)/etc
+git_etcdir = etc
+endif
+
 ifdef CHECK_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES =
 USE_COMPUTED_HEADER_DEPENDENCIES =
-- 
1.7.5

-- 
  Kacper Kornet