Re: Use LN_S instead of "ln -s" in Makefile

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andreas Karlsson <andreas(at)proxel(dot)se>
Cc: Ashwin Agrawal <aagrawal(at)pivotal(dot)io>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Use LN_S instead of "ln -s" in Makefile
Date: 2020-02-15 18:29:23
Message-ID: 710.1581791363@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Andreas Karlsson <andreas(at)proxel(dot)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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-02-15 20:25:55 Re: assert pg_class.relnatts is consistent
Previous Message David Fetter 2020-02-15 17:51:05 Re: Parallel copy