Обсуждение: Use LN_S instead of "ln -s" in Makefile

Ashwin Agrawal <aagrawal@pivotal.io> writes:
> In general, the variable LN_S is 'ln -s', however, LN_S changes to 'cp
> -pR' if configure finds the file system does not support symbolic
> links.
> I was playing with 'ln' for some other reason where I symbolic linked
> it to '/bin/false'. That revealed the failure for
> src/backend/Makefile. Greping for 'ln -s' revealed three places it's
> used. Attaching the patch to fix the same.

Hm.  So, these oversights are certainly bugs in narrow terms.  However,
they've all been like that for a *long* time --- the three instances
you found date to 2005, 2006, and 2008 according to "git blame".
The complete lack of complaints shows that nobody cares.  So I think
a fairly strong case could be made for going the other way, ie
s/$(LN_S)/ln -s/g and get rid of the configure-time cycles wasted in
setting up that variable.  Otherwise I fear somebody will "break"
it again soon, and it will stay "broken" for another 15 years till
someone happens to notice.  We have better things to do than spend
our time maintaining such nonfunctional differences.

            regards, tom lane

Ashwin Agrawal <aagrawal@pivotal.io> writes:
> On Fri, Feb 14, 2020 at 4:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  So, these oversights are certainly bugs in narrow terms.  However,
>> they've all been like that for a *long* time --- the three instances
>> you found date to 2005, 2006, and 2008 according to "git blame".
>> The complete lack of complaints shows that nobody cares.  So I think
>> a fairly strong case could be made for going the other way, ie
>> s/$(LN_S)/ln -s/g and get rid of the configure-time cycles wasted in
>> setting up that variable.

> I accede to it.

Oh ... 2005 was just the last time anybody touched that particular
line in backend/Makefile.  Further digging shows that we've been
installing the postmaster -> postgres symlink with raw "ln -s"
clear back to the Postgres95 virgin sources.  I didn't bother to
chase down the oldest instances of the other two cases.

(Man, "git blame" is such a great tool for software archaeology.)

            regards, tom lane

On 2/15/20 1:57 AM, Tom Lane wrote:
> Hm.  So, these oversights are certainly bugs in narrow terms.  However,
> they've all been like that for a *long* time --- the three instances
> you found date to 2005, 2006, and 2008 according to "git blame".
> The complete lack of complaints shows that nobody cares.  So I think
> a fairly strong case could be made for going the other way, ie
> s/$(LN_S)/ln -s/g and get rid of the configure-time cycles wasted in
> setting up that variable.

Here is a patch which does that.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 2/15/20 1:57 AM, Tom Lane wrote:
>> Hm.  So, these oversights are certainly bugs in narrow terms.  However,
>> they've all been like that for a *long* time --- the three instances
>> you found date to 2005, 2006, and 2008 according to "git blame".
>> The complete lack of complaints shows that nobody cares.  So I think
>> a fairly strong case could be made for going the other way, ie
>> s/$(LN_S)/ln -s/g and get rid of the configure-time cycles wasted in
>> setting up that variable.

> Here is a patch which does that.

I was just about to push that when I noticed something that gave me
pause: the "ln -s" in backend/Makefile is wrapped in 
    ifneq ($(PORTNAME), win32)
This means there's one popular platform where we *don't* know for
sure that people aren't building in environments that don't support
"ln -s".  (The other two direct uses that Ashwin found are in test
code that a non-developer person very likely would never exercise,
so I don't think they prove much.)

I'm still on balance inclined to push this.  We have no buildfarm
animals exercising the case (they all report "ln -s" as supported,
even the Windows animals), and these days I think most people who
are building for Windows use the MSVC scripts not the makefiles.

Moreover, $(LN_S) is a horribly error-prone macro, because of the
fact that "ln -s" and "cp" don't have the same semantics for the
source argument.  The Autoconf manual says

     If you make a link in a directory other than the current
     directory, its meaning depends on whether `ln' or `ln -s' is used.
     To safely create links using `$(LN_S)', either find out which
     form is used and adjust the arguments, or always invoke `ln' in
     the directory where the link is to be created.

     In other words, it does not work to do:
          $(LN_S) foo /x/bar

     Instead, do:

          (cd /x && $(LN_S) foo bar)

So Ashwin's original patch would, far from fixing the code for
symlink-less systems, just have caused them to fail in a different way.
I could do without having that sort of gotcha in our build system,
especially if the set of people it would help is so close to empty,
and most especially when we have no testing that would catch mistakes.

Nonetheless, it looks like the current makefiles do work, for moderate
values of "work", on non-symlink Windows.  If we apply this patch
then they won't.

An alternative we could consider is to go back to Ashwin's patch,
after fixing it to use the "cd && ln" approach.  I noticed though
while chasing through the git history that that approach was in place
there originally and was specifically rejected in commit ccca61b5f.
That commit is quite old enough to drink, so maybe the underlying
concern no longer applies --- certainly we're using "cd && ln"
elsewhere.  But this seems like another point in favor of the whole
business being too complex/error-prone to want to support.

            regards, tom lane