threads / discuss / 17485

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

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

## tl;dr

13 messages between Feb 1, 2009 and Feb 5, 2009.

replies: 12people: 5as markdown or json

A Large Angry SCM· Feb 1, 2009, 18:24 UTC · lore

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
Junio C Hamano· Feb 5, 2009, 07:04 UTC · re: A Large Angry SCM · lore

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

A Large Angry SCM <gitzilla@gmail.com> writes:
Show 6 quoted lines
> 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.
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.

Steffen Prohaska· Feb 5, 2009, 07:13 UTC · re: Junio C Hamano · lore

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

On Feb 5, 2009, at 8:04 AM, Junio C Hamano wrote:
Show 11 quoted lines
> A Large Angry SCM <gitzilla@gmail.com> writes:
>
>> 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.
>
> Is there anything being done on this issue?
Not yet.  I haven't had time.
> I could revert ed096c4 (Merge branch 'sp/runtime-prefix',  
> 2009-01-31), but
> I'd rather not if we can avoid it.
I'll propose a fix on Saturday.
	Steffen
Junio C Hamano· Feb 5, 2009, 07:35 UTC · re: A Large Angry SCM · lore

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

A Large Angry SCM <gitzilla@gmail.com> writes:
> 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.
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
Pascal Obry· Feb 5, 2009, 07:38 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Feb 5, 2009, 07:53 UTC · re: Pascal Obry · lore

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

Pascal Obry <pascal@obry.net> writes:
> I have just proposed a patch to fix this in another thread.
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.

Pascal Obry· Feb 5, 2009, 07:57 UTC · re: Junio C Hamano · lore

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

Junio,
> Does your patch address the lack of $(abspath) in older GNU make?
Don't think so.
> 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.

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
Junio C Hamano· Feb 5, 2009, 08:01 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
> 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.
    Signed-off-by: Pascal Obry <pascal@obry.net>
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 C Hamano· Feb 5, 2009, 07:44 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> A Large Angry SCM <gitzilla@gmail.com> writes:
>
>> 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.
>
> 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(-)
diff --git c/Makefile w/Makefile
index a82f173..605b147 100644
--- c/Makefile
+++ w/Makefile
@@ -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)
Junio C Hamano· Feb 5, 2009, 08:18 UTC · re: A Large Angry SCM · lore

[PATCH] Makefile: fix misdetection of relative pathnames

A Large Angry SCM <gitzilla@gmail.com> writes:
Show 9 quoted lines
> 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 '/'

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index a82f173..605b147 100644
--- a/Makefile
+++ b/Makefile
@@ -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
Johannes Sixt· Feb 5, 2009, 12:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: fix misdetection of relative pathnames

Junio C Hamano schrieb:
> -ifeq ($(abspath $(template_dir)),$(template_dir))
> +ifneq ($(filter /%,$(firstword $(template_dir))),)
> -ifeq ($(abspath $(gitexecdir)),$(gitexecdir))
> +ifneq ($(filter /%,$(firstword $(gitexecdir))),)

No, this does not work on Windows for the use-case that this check is intended for, namely when the user specifies an *absolute* path for gitexecdir and/or template_dir in config.mak [*].

Neither does the version that uses $(abspath ...)! Because $(abspath ...) does not work in our msysgit environment (that has GNU make 3.79.1).

That said, I don't think it's worth to cater for this use-case, precisely because we want to *avoid* absolute paths on Windows anyway, and apply the change that you proposed here.

[*] The reason it does not work is that we cannot use MSYS-style absolute paths /c/Foo/Bar because the paths will be interpreted by git, which does not understand them; the user must specify drive-letter absolute paths c:/Foo/Bar, but the check does not catch them.

-- Hannes
Junio C Hamano· Feb 5, 2009, 17:19 UTC · re: Johannes Sixt · lore

Re: [PATCH] Makefile: fix misdetection of relative pathnames

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 8 quoted lines
> That said, I don't think it's worth to cater for this use-case, precisely
> because we want to *avoid* absolute paths on Windows anyway, and apply the
> change that you proposed here.
>
> [*] The reason it does not work is that we cannot use MSYS-style absolute
> paths /c/Foo/Bar because the paths will be interpreted by git, which does
> not understand them; the user must specify drive-letter absolute paths
> c:/Foo/Bar, but the check does not catch them.
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).

A Large Angry SCM· Feb 5, 2009, 22:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: fix misdetection of relative pathnames

Junio C Hamano wrote:
Show 43 quoted lines
> A Large Angry SCM <gitzilla@gmail.com> writes:
> 
>> 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 '/'
> 
> 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.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Tested-by: A Large Angry SCM <gitzilla@gmail.com>

← back to recent threads