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

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

[A Large Angry SCM]

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

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

[Junio C Hamano]

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?

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

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

[Steffen Prohaska]

On Feb 5, 2009, at 8:04 AM, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

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

[Pascal Obry]

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

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

[Junio C Hamano]

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.

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

[Pascal Obry]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[PATCH] Makefile: fix misdetection of relative pathnames

[Junio C Hamano]

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

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

[Johannes Sixt]

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

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

[Junio C Hamano]

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

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

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

[A Large Angry SCM]

Junio C Hamano wrote:
> 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>