# 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes 13 messages from 2009-02-01 to 2009-02-05. Participants: A Large Angry SCM, Junio C Hamano, Steffen Prohaska, Pascal Obry, Johannes Sixt. Thread: https://gitlist.dev/t/17485 ## A Large Angry SCM, 2009-02-01 18:24 Subject: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <4985E8E1.90303@gmail.com> URL: https://gitlist.dev/e/4985E8E1.90303%40gmail.com ``` 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, 2009-02-05 07:04 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <7vab91icu0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vab91icu0.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4985E8E1.90303@gmail.com> ``` A Large Angry SCM 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. ``` ## Steffen Prohaska, 2009-02-05 07:13 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <54B6E81F-FCC5-453D-9F38-9F29C41F5613@zib.de> URL: https://gitlist.dev/e/54B6E81F-FCC5-453D-9F38-9F29C41F5613%40zib.de In-Reply-To: <7vab91icu0.fsf@gitster.siamese.dyndns.org> ``` On Feb 5, 2009, at 8:04 AM, Junio C Hamano wrote: > A Large Angry SCM 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, 2009-02-05 07:35 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <7v63jpibe8.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63jpibe8.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4985E8E1.90303@gmail.com> ``` A Large Angry SCM 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, 2009-02-05 07:38 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: URL: https://gitlist.dev/e/a2633edd0902042338n25dc7be4vbb202d011eb4ad78%40mail.gmail.com In-Reply-To: <7v63jpibe8.fsf@gitster.siamese.dyndns.org> ``` 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, 2009-02-05 07:44 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <7vzlh1gwf4.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzlh1gwf4.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7v63jpibe8.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > A Large Angry SCM 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, 2009-02-05 07:53 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <7vvdrpgvzl.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdrpgvzl.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Pascal Obry 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, 2009-02-05 07:57 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: URL: https://gitlist.dev/e/a2633edd0902042357s35ab8fe7g59cb3ef109e9b0bb%40mail.gmail.com In-Reply-To: <7vvdrpgvzl.fsf@gitster.siamese.dyndns.org> ``` 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, 2009-02-05 08:01 Subject: Re: 026fa0d5ad Breaks installs with absolue $(gitexecdir) and $(template_dir) variables using older GNU makes Message-ID: <7vprhxgvnc.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vprhxgvnc.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vvdrpgvzl.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano 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 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, 2009-02-05 08:18 Subject: [PATCH] Makefile: fix misdetection of relative pathnames Message-ID: <7vhc39guv2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhc39guv2.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4985E8E1.90303@gmail.com> ``` A Large Angry SCM 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 --- 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, 2009-02-05 12:27 Subject: Re: [PATCH] Makefile: fix misdetection of relative pathnames Message-ID: <498ADB1B.7030300@viscovery.net> URL: https://gitlist.dev/e/498ADB1B.7030300%40viscovery.net In-Reply-To: <7vhc39guv2.fsf@gitster.siamese.dyndns.org> ``` 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, 2009-02-05 17:19 Subject: Re: [PATCH] Makefile: fix misdetection of relative pathnames Message-ID: <7v1vucg5ss.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vucg5ss.fsf%40gitster.siamese.dyndns.org In-Reply-To: <498ADB1B.7030300@viscovery.net> ``` Johannes Sixt 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). ``` ## A Large Angry SCM, 2009-02-05 22:16 Subject: Re: [PATCH] Makefile: fix misdetection of relative pathnames Message-ID: <498B652D.8060708@gmail.com> URL: https://gitlist.dev/e/498B652D.8060708%40gmail.com In-Reply-To: <7vhc39guv2.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > A Large Angry SCM 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 Tested-by: A Large Angry SCM ```