[PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
For consistency, we should use the same source for symbolic links as for hard links and copies. Signed-off-by: Sebastian Schuberth sschube...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
Kyle J. McKay mack...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
On Thu, Feb 5, 2015 at 9:45 PM, Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
Kyle J. McKay mack...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
On Thu, Feb 5, 2015 at 8:23 PM, Junio C Hamano gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
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. ;) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
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 sschube...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
On Thu, Feb 5, 2015 at 8:51 PM, Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Makefile: Use the same source directory for ln -s as for ln / cp
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
