026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes

In 026fa0d5ad9538ca76838070861531c037d7b9ba, "Move computation of absolute paths from Makefile to runtime (in preparation for RUNTIME_PREFIX)", the following change was made to the Makefile. The problem is that $(abspath names...) is a relatively recent addition to GNU make and when used in an older GNU make, the test always fails resulting incorrect installation dirs for the templates and commands.

The new test is also wrong; (for *nix systems) in that it really wants to test if the first character is a '/' but GNU make doesn't have a way to do that directly. Instead, it tests if the first character is a / _AND_ the path string does not include . or .. components,

The older test has problems itself but at least it allowed you to specify absolute paths.

@@ -1407,17 +1417,17 @@ remove-dashes:

### Installation rules

-ifeq ($(firstword $(subst /, ,$(template_dir))),..) -template_instdir = $(bindir)/$(template_dir) -else
+ifeq ($(abspath $(template_dir)),$(template_dir))
template_instdir = $(template_dir)
+else
+template_instdir = $(prefix)/$(template_dir)
endif
export template_instdir

-ifeq ($(firstword $(subst /, ,$(gitexecdir))),..) -gitexec_instdir = $(bindir)/$(gitexecdir) -else
+ifeq ($(abspath $(gitexecdir)),$(gitexecdir))
gitexec_instdir = $(gitexecdir)
+else
+gitexec_instdir = $(prefix)/$(gitexecdir)
endif
gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir)) export gitexec_instdir

I see 3 ways to fix the problem:
1) go back to using the old test.
2) keep the new test but add a test that will break the build if $(abspath names...) does not work.
3) something like the following (untested)

### Installation rules

temp = $(subst " ",x,$(template_dir))
temp = $(subst //,/,$(temp))
temp = $(addsuffix x,$(temp))
temp = $(subst /, ,$(temp))
ifeq ($(strip $(temp)),$(temp))
template_instdir = $(template_dir)
else
template_instdir = $(prefix)/$(template_dir)
endif
export template_instdir

temp = $(subst " ",x,$(gitexecdir))
temp = $(subst //,/,$(temp))
temp = $(addsuffix x,$(temp))
temp = $(subst /, ,$(temp))
ifeq ($(strip $(temp)),$(temp)) gitexec_instdir = $(gitexecdir)
else
gitexec_instdir = $(prefix)/$(gitexecdir) endif
gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir)) export gitexec_instdir

Hmph. Isn't it just the matter of doing something silly like this?

ifeq ($(filter /%,$(template_dir)),)
... it does not start with a slash
else

	... it does

endif

That is, something like this.

The patch also protects the $(filter) check from sick users that use

template_dir = My Files /the-leading-dir-ends-with-a-space

by using $(firstword)

No, I didn't test it.

 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index a82f173..605b147 100644
@@ -1434,14 +1434,14 @@ remove-dashes:
 
 ### Installation rules
 
-ifeq ($(abspath $(template_dir)),$(template_dir))
+ifneq ($(filter /%,$(firstword $(template_dir))),)
 template_instdir = $(template_dir)
 else
 template_instdir = $(prefix)/$(template_dir)
 endif
 export template_instdir
 
-ifeq ($(abspath $(gitexecdir)),$(gitexecdir))
+ifneq ($(filter /%,$(firstword $(gitexecdir))),)
 gitexec_instdir = $(gitexecdir)
 else
 gitexec_instdir = $(prefix)/$(gitexecdir)

I have just proposed a patch to fix this in another thread.

Pascal.

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Does your patch address the lack of $(abspath) in older GNU make?

I do not know if mine that used firstword did for that matter, either, though.

Regardless, I think your patch to config.mak is independently good, because the way Makefile defines these directories is without the trailing slash, and it would be better to be consistent.

In other words, I am thinking about applying your patch after amending the log message like this.

[PATCH] config.mak.in: define paths without trailing slash

The main Makefile defines gitexecdir and template_dir without trailing slash. config.mak.in should do the same to be consistent.

I think the misuse of abspath is a separate issue.

The main Makefile should be made robust not to misdetect the relative vs absolute, especially because not everybody uses config.mak.autogen, and because people should be able to override these variables from the command line of make invocation and the name of the directory with or without trailing slash ought to mean the same thing.

Junio,

Don't think so.

Right, better to have no slash ending directories to avoid double slashes in $(dir)/file.c.

Pascal.

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Is there anything being done on this issue?

I could revert ed096c4 (Merge branch 'sp/runtime-prefix', 2009-01-31), but I'd rather not if we can avoid it.

Not yet. I haven't had time.

I'll propose a fix on Saturday.

Steffen

Ok, here is the final one from me that I am considering to commit, but it would be nice to hear success/failure feedback from people who had trouble with Steffen's changes. I'd also like to hear from people who have been happy with Steffen's changes that this does not break things for them.

-- >8 --

The installation rules wanted to differentiate between a template_dir that is given as an absolute path (e.g. /usr/share/git-core/templates) and a relative one (e.g. share/git-core/templates) but it was done by checking if $(abspath $(template_dir)) and $(template_dir) yield the same string.

This was wrong in number of ways.

* The user can give template_dir with a trailing slash from the command line to invoke make, or in the included config.mak. A directory path ought to mean the same thing with or without such a trailing slash but use of $(abspath) means with an absolute path with a trailing slash fails the test.

* The compilation could be done inside /usr directory with a relative pathname share/git-core/templates and it will be mistaken as an absolute path.

* Versions of GNU make older than 3.81 do not have $(abspath) to begin with.

This changes the detection logic to see if the given path begins with a slash.

---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index a82f173..605b147 100644
@@ -1434,14 +1434,14 @@ remove-dashes:
 
 ### Installation rules
 
-ifeq ($(abspath $(template_dir)),$(template_dir))
+ifneq ($(filter /%,$(firstword $(template_dir))),)
 template_instdir = $(template_dir)
 else
 template_instdir = $(prefix)/$(template_dir)
 endif
 export template_instdir
 
-ifeq ($(abspath $(gitexecdir)),$(gitexecdir))
+ifneq ($(filter /%,$(firstword $(gitexecdir))),)
 gitexec_instdir = $(gitexecdir)
 else
 gitexec_instdir = $(prefix)/$(gitexecdir)

1.6.1.2.382.gda69a

Hmm, not c:\Program Files\Git?

In any case, thanks for an Ack. It clears one remaining issue for -rc0 (and there are certainly others I have forgot).