[PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Sebastian Schuberth]

For consistency, we should use the same source for symbolic links as for
hard links and copies.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index c44eb3a..21f23cb 100644
--- a/Makefile
+++ b/Makefile
@@ -2265,14 +2265,14 @@ endif
         $(RM) "$$bindir/$$p" && \
         test -z "$(NO_INSTALL_HARDLINKS)" && \
         ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
-        ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
+        ln -s "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
         cp "$$bindir/git$X" "$$bindir/$$p" || exit; \
     done && \
     for p in $(BUILT_INS); do \
         $(RM) "$$execdir/$$p" && \
         test -z "$(NO_INSTALL_HARDLINKS)" && \
         ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-        ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
+        ln -s "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
         cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
     done && \
     remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
@@ -2280,7 +2280,7 @@ endif
         $(RM) "$$execdir/$$p" && \
         test -z "$(NO_INSTALL_HARDLINKS)" && \
         ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
-        ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
+        ln -s "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
         cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
     done && \
     ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
-- 
2.1.2-mingw-1

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Kyle J. McKay]

On Feb 5, 2015, at 07:51, Sebastian Schuberth wrote:
> For consistency, we should use the same source for symbolic links as  
> for
> hard links and copies.
>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
> Makefile | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c44eb3a..21f23cb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2265,14 +2265,14 @@ endif
>         $(RM) "$$bindir/$$p" && \
>         test -z "$(NO_INSTALL_HARDLINKS)" && \
>         ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> -        ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> +        ln -s "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \

This is wrong.

Currently with symlinks you will get installed into bindir something  
like this:

   git
   git-tag -> git
   git-show -> git

etc.

With your change you would have

   git
   git-tag -> /usr/local/libexec/git-core/git
   git-show -> /usr/local/libexec/git-core/git

And I don't think we want that.  While those absolute path symlinks  
are technically correct, what we have now is much simpler.  While  
there are a number of build-time paths hard-coded into the  
executables, the binaries work for the most part if you relocate them  
as a whole.  Your change totally breaks that.

-Kyle

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

>> -        ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
>> +        ln -s "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
>
> This is wrong.
>
> Currently with symlinks you will get installed into bindir something
> like this:
>
>   git
>   git-tag -> git
>   git-show -> git
>
> etc.
>
> With your change you would have
>
>   git
>   git-tag -> /usr/local/libexec/git-core/git
>   git-show -> /usr/local/libexec/git-core/git
>
> And I don't think we want that.  While those absolute path symlinks
> are technically correct,...

It is not even correct, is it?

When DESTDIR is set to allow you to install into a temporary place
only so that you can "tar" up the resulting filesystem tree, bindir
points at the location we need to "cp" the built programs into, i.e.
inside DESTDIR.

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Sebastian Schuberth]

On Thu, Feb 5, 2015 at 8:23 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> This is wrong.
>>
>> Currently with symlinks you will get installed into bindir something
>> like this:
>>
>>   git
>>   git-tag -> git
>>   git-show -> git
>>
>> etc.
>>
>> With your change you would have
>>
>>   git
>>   git-tag -> /usr/local/libexec/git-core/git
>>   git-show -> /usr/local/libexec/git-core/git
>>
>> And I don't think we want that.  While those absolute path symlinks
>> are technically correct,...
>
> It is not even correct, is it?
>
> When DESTDIR is set to allow you to install into a temporary place
> only so that you can "tar" up the resulting filesystem tree, bindir
> points at the location we need to "cp" the built programs into, i.e.
> inside DESTDIR.

Agreed folks, please disregard this as well as 2/2 of this series.

-- 
Sebastian Schuberth

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Jeff King]

On Thu, Feb 05, 2015 at 08:26:08PM +0100, Sebastian Schuberth wrote:

> > It is not even correct, is it?
> >
> > When DESTDIR is set to allow you to install into a temporary place
> > only so that you can "tar" up the resulting filesystem tree, bindir
> > points at the location we need to "cp" the built programs into, i.e.
> > inside DESTDIR.
> 
> Agreed folks, please disregard this as well as 2/2 of this series.

We would still want an equivalent to 2/2 to set up a relative symlink
for $(ALL_PROGRAMS), though, right?

-Peff

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Sebastian Schuberth]

On Thu, Feb 5, 2015 at 8:51 PM, Jeff King <peff@peff.net> wrote:

> On Thu, Feb 05, 2015 at 08:26:08PM +0100, Sebastian Schuberth wrote:
>
>> > It is not even correct, is it?
>> >
>> > When DESTDIR is set to allow you to install into a temporary place
>> > only so that you can "tar" up the resulting filesystem tree, bindir
>> > points at the location we need to "cp" the built programs into, i.e.
>> > inside DESTDIR.
>>
>> Agreed folks, please disregard this as well as 2/2 of this series.
>
> We would still want an equivalent to 2/2 to set up a relative symlink
> for $(ALL_PROGRAMS), though, right?

Probably. But I'm not sure how to calculate the relative path
correctly so that it'll work with all possible bin / execdir
combinations. A simple

diff --git a/Makefile b/Makefile
index 21f23cb..d2849c3 100644

--- a/Makefile
+++ b/Makefile

@@ -2258,8 +2258,8 @@

 endif
 ········$(RM)·"$$execdir/$$p"·&&·\
 ········test·-z·"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)"·&&·\
 ········ln·"$$bindir/$$p"·"$$execdir/$$p"·2>/dev/null·||·\
+········ln·-s·"../$$p"·"$$execdir/$$p"·2>/dev/null·||·\
 ········cp·"$$bindir/$$p"·"$$execdir/$$p"·||·exit;·\
 ······done;·\
 ····}·&&·\
--·

does not seem to be correct in all cases.

-- 
Sebastian Schuberth

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Kyle J. McKay]

On Feb 5, 2015, at 11:51, Jeff King wrote:
> On Thu, Feb 05, 2015 at 08:26:08PM +0100, Sebastian Schuberth wrote:
>
>>> It is not even correct, is it?
>>>
>>> When DESTDIR is set to allow you to install into a temporary place
>>> only so that you can "tar" up the resulting filesystem tree, bindir
>>> points at the location we need to "cp" the built programs into, i.e.
>>> inside DESTDIR.
>>
>> Agreed folks, please disregard this as well as 2/2 of this series.
>
> We would still want an equivalent to 2/2 to set up a relative symlink
> for $(ALL_PROGRAMS), though, right?

It's this line here:

>                 ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \

But since bindir and execdir can be configured to be arbitrary paths  
you must compute the relative path between them in order to create a  
relative symlink.  In principle you just remove the common directory  
prefix, remove the non-directory part from the destination, change any  
remaining directories in the destination to '..' and then append  
what's left of the source to get the new source.

So if you have

   bindir=/usr/local/bin
   execdir=/usr/local/libexec/git-core

And the ln line would be:

   ln /usr/local/bin/git /usr/local/libexec/git-core/git

1) Strip the common prefix which is /usr/local/

   source = bin/git
   dest = libexec/git-core/git

2) remove non-directory part of dest (the basename part) and replace  
remaining dirs with '..'

   source = bin/git
   dest = ../../

3) append source to dest and you get

   ../../bin/git

So the symlink line becomes:

   ln -s ../../bin/git /usr/local/libexec/git-core/git

Now, can you do that easily in a Makefile? ;)

And lastly there's the issue of which should be the symlink and which  
should be the binary?

Should /usr/local/bin/git be the symlink or the binary?

If it's the binary, then /usr/local/libexec/git-core/git will be a  
symlink to it.  But we're already installing several other symlinks to  
'git' in /usr/local/libexec/git-core so they will need to traverse two  
symlinks to get to the binary rather than just the one.

That seems suboptimal.

On the other hand if /usr/local/bin/git becomes the symlink then we  
have a user-facing binary that's a symlink.  As generally it's the  
bindir that ends up in the PATH.

I'm not sure exactly why, but I think:

On Jan 30, 2015, at 13:10, Junio C Hamano wrote:
> That would make me feel dirty.


Having a user-facing binary that is actually a symlink can potentially  
cause problems on OS X if the binary it refers to locates its  
libraries using a relative path.  Not sure if that's the case on other  
systems or not.

Neither of these is probably a show-stopper (two-symlinks-to-binary or  
user-facing-symlink-binary) assuming the magic relative symlink  
computation can be worked out.

But it does not surprise me that those items were allowed to skip  
making a symlink and just go directly to cp.

-Kyle

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Jeff King]

On Thu, Feb 05, 2015 at 09:29:04PM +0100, Sebastian Schuberth wrote:

> > We would still want an equivalent to 2/2 to set up a relative symlink
> > for $(ALL_PROGRAMS), though, right?
> 
> Probably. But I'm not sure how to calculate the relative path
> correctly so that it'll work with all possible bin / execdir
> combinations. A simple
> 
> diff --git a/Makefile b/Makefile
> index 21f23cb..d2849c3 100644
> 
> --- a/Makefile
> +++ b/Makefile
> 
> @@ -2258,8 +2258,8 @@
> 
>  endif
>  ········$(RM)·"$$execdir/$$p"·&&·\
>  ········test·-z·"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)"·&&·\
>  ········ln·"$$bindir/$$p"·"$$execdir/$$p"·2>/dev/null·||·\
> +········ln·-s·"../$$p"·"$$execdir/$$p"·2>/dev/null·||·\
>  ········cp·"$$bindir/$$p"·"$$execdir/$$p"·||·exit;·\
>  ······done;·\
>  ····}·&&·\
> --·
> 
> does not seem to be correct in all cases.

Ah, I see. Yeah, you'd have to calculate that relative path between
$bindir and $execdir. We have C code already to do that (see
relative_path() in path.c), so in theory you could build a wrapper
during the build phase and then run:

  ln -s $(./my-wrapper "$bindir" "$execdir")/$p $execdir/$p

or something during the install phase.

-Peff

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Sebastian Schuberth]

On Thu, Feb 5, 2015 at 9:45 PM, Jeff King <peff@peff.net> wrote:

>>  endif
>>  ········$(RM)·"$$execdir/$$p"·&&·\
>>  ········test·-z·"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)"·&&·\
>>  ········ln·"$$bindir/$$p"·"$$execdir/$$p"·2>/dev/null·||·\
>> +········ln·-s·"../$$p"·"$$execdir/$$p"·2>/dev/null·||·\
>>  ········cp·"$$bindir/$$p"·"$$execdir/$$p"·||·exit;·\
>>  ······done;·\
>>  ····}·&&·\
>> --·
>>
>> does not seem to be correct in all cases.
>
> Ah, I see. Yeah, you'd have to calculate that relative path between
> $bindir and $execdir. We have C code already to do that (see
> relative_path() in path.c), so in theory you could build a wrapper
> during the build phase and then run:
>
>   ln -s $(./my-wrapper "$bindir" "$execdir")/$p $execdir/$p
>
> or something during the install phase.

Thanks for pointing out the C code to do that. But IMO creating and
using such a wrapper is not worth the effort.

-- 
Sebastian Schuberth

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> Now, can you do that easily in a Makefile? ;)

Or is it worth doing it?

I do not mind a full symbolic link as long as it points at the
correct place (Sebastian's version did not under DESTDIR which was
the only issue I had against it), but is there a good reason why we
would prefer a relative one (or we would want to avoid a full one)?

> I'm not sure exactly why, but I think:
>
> On Jan 30, 2015, at 13:10, Junio C Hamano wrote:
>> That would make me feel dirty.

That is a confusing style of quoting.  I suspect that I said the
above in a totally different context.

> Having a user-facing binary that is actually a symlink can potentially
> cause problems on OS X if the binary it refers to locates its
> libraries using a relative path.

I am not sure what problem you are trying to single out by repeating
"user-facing" here.

As doing this is still fully supported:

    $ PATH=$(git --exec-path):$PATH
    $ export PATH
    $ git-cat-file -t HEAD

You can arrange things in different ways:

 - /usr/libexec/git-core/git-cat-file can a symbolic link to
   /usr/bin/git (cross directory)

 - /usr/libexec/git-core/git-cat-file can a symbolic link to "git"
   (in the same directory) and then /usr/bin/git may be a symbolic
   link to /usr/libexec/git-core/git (cross directory)

No matter what, as long as you have these two directories, you would
have the issue that a symbolic link that is given to execv(2) might
misbehave somewhere anyway, no?

I do not know (and I am not quite sure if I want to know) how
serious your "potential" problems would be, and I do not doubt you
know OS X quirks much better than I do and do not intend to argue
there aren't such problems.  I am just curious how "user-facing"
comes into the picture.

Re: [PATCH 1/2] Makefile: Use the same source directory for "ln -s" as for "ln" / "cp"

[Kyle J. McKay]

On Feb 5, 2015, at 12:59, Junio C Hamano wrote:
> I do not know (and I am not quite sure if I want to know) how
> serious your "potential" problems would be, and I do not doubt you
> know OS X quirks much better than I do and do not intend to argue
> there aren't such problems.  I am just curious how "user-facing"
> comes into the picture.

Suppose you have built and installed some software to /usr/local and  
it's ended up in the usual subdirectories, /usr/local/bin, /usr/local/ 
lib, etc.

Now you get an installer for package X, and to avoid stepping on  
things it installs to /opt/XYZ with the usual suspects /opt/XYZ/bin, / 
opt/XYZ/lib.  There are several of these that I'm aware of where XYZ  
is whatever package you're installing.

Now since /opt/XYZ/bin is not usually in one's PATH it installs a few  
select symlinks from /usr/local/bin/whatever to /opt/XYZ/bin/ 
whatever.  Not all of the /opt/XYZ installers do this, for some of  
them it's a selectable option.

So now we have:

# This one can be a relative or absolute symlink, doesn't matter
/usr/local/bin/foo -> /opt/XYZ/bin/foo

And we also now have this:

# A real binary, not a symlink
# Uses and locates libfoo with ../lib/libfoo.dylib
/opt/XYZ/bin/foo

# A real library binary, not a symlink
/opt/XYZ/lib/libfoo.dylib

So /opt/XYZ/bin/foo locates libfoo.dylib by using a relative path  
embedded within it (in this case it would be @loader_path/../lib/ 
libfoo.dylib).

So far, so good, right?

Nothing complicated going on.

But the user has built and installed an older version of libfoo  
already to /usr/local/lib/libfoo.dylib.

Ah-oh.

User attempts to run foo.

Shell finds foo at /usr/local/bin/foo and runs it.

The dynamic linker correctly loads /opt/XYZ/bin/foo and starts to  
resolve the dynamic library links embedded in it.

For the "@loader_path/../lib/libfoo.dylib" one it first tries /usr/ 
local/bin/../lib/libfoo.dylib rather than /opt/XYZ/bin/foo/../lib/ 
libfoo.dylib because /usr/local/bin/foo was the executable that was  
run and since the user has installed an older version of libfoo.dylib  
in /usr/local/lib, it finds and loads that instead of the intended  
one.  If it didn't find that it would then try /opt/XYZ/bin/foo/../lib/ 
libfoo.dylib and get the right one.

Sometimes you get an immediate error if the unintended library version  
is just too old to be compatible.  On the other hand, if it's just old  
enough to be buggy but not version incompatible now you're running  
with a buggy library or some other incompatibility that doesn't show  
up right away.

I think this is a nasty bug specific to OS X, the fact that it tries  
both paths though suggests it was deliberate.

I have not tested all versions of OS X to see if it might have been  
fixed in the latest, but there it is.

If you put /opt/XYZ/bin directly in the PATH this can't happen  
(provided /opt/XYZ/bin/foo is not itself a symlink).

So that's how a user-facing binary that is a symlink can cause  
problems.  I suppose it's not just limited to user-facing binaries,  
but that's where it tends to show up as it seems the non-user-facing,  
non-executable stuff usually has a sufficiently controlled surrounds  
that they're not vulnerable.

I say "user-facing" because the binary is found in the user's $PATH.   
I do not consider the libexec/git-core binaries to be user-facing  
since they are not normally in the user's $PATH but rather  normally  
accessed via the "git" binary which is likely in the user's $PATH.

In the case of Git, a /usr/local/bin/git -> ../libexec/git-core/git  
symlink should not be vulnerable to this problem, because even if the  
executable ends up with some relative dylib paths in it (usually not  
the case) when applied to /usr/local/bin they're unlikely to lead  
anywhere with user-installed libraries since git-core is one directory  
deeper than bin.

Anyhow, that's why I tend to avoid putting symlink-to-binary stuff in  
PATH.  In any case a packager can make adjustments to avoid the  
problem before creating an installer.

-Kyle

P.S.

On Feb 5, 2015, at 12:59, Junio C Hamano wrote:
>> I'm not sure exactly why, but I think:
>>
>> On Jan 30, 2015, at 13:10, Junio C Hamano wrote:
>>> That would make me feel dirty.
>
> That is a confusing style of quoting.  I suspect that I said the
> above in a totally different context.

But I've been dying to work that in somehow ever since I saw that.  ;)