patchRespect definition of prefix from autotools in ETC_GITCONFIG and ETC_GITATTRIBUTES

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(-)

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

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

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.

On Thu, Apr 28, 2011 at 09:54:02AM -0700, Junio C Hamano wrote:

You are right.

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

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(-)

./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(-)

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

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.

On Thu, Apr 28, 2011 at 02:05:09PM -0700, Junio C Hamano wrote:

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.

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

Am 4/28/2011 21:27, schrieb Kacper Kornet:

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

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

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.

Am 5/3/2011 19:32, schrieb Junio C Hamano:

Fair enough.

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

On Wed, May 04, 2011 at 07:52:30AM +0200, Johannes Sixt wrote:

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

absolute path.

Am 5/4/2011 15:58, schrieb Kacper Kornet:

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

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

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.

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>

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

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(-)

Am 5/5/2011 4:26, schrieb Junio C Hamano:

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

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

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.

Am 5/5/2011 18:17, schrieb Junio C Hamano:

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

On Wed, May 04, 2011 at 07:26:29PM -0700, Junio C Hamano wrote:

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

A:

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

  4. ./configure --sysconfdir=dir2

     git_etcdir =  dir2

B:

  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

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

Yes, configure does not accept relative pathnames.

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 <kornet@camk.edu.pl> writes:

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.

Am 5/5/2011 17:46, schrieb Junio C Hamano:

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(-)

On Mon, May 09, 2011 at 10:24:55AM +0200, Johannes Sixt wrote:

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

On Mon, May 09, 2011 at 10:24:55AM +0200, Johannes Sixt wrote:

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

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:

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]

On Tue, May 03, 2011 at 10:32:14AM -0700, Junio C Hamano wrote:

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: 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(-)