Обсуждение: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

Hi,

On Mon, Jun 05, 2023 at 09:47:30AM -0400, Evan Jones wrote:
> This makes "make check" work on Mac OS X. Without this patch, on Mac OS X a
> default "./configure; make; make check" fails with errors like:
> 
> dyld[65265]: Library not loaded: /usr/local/pgsql/lib/libpq.5.dylib
>   Referenced from: <59A2EAF9-6298-3112-BEDB-EA9A62A9DB53>
> /Users/evan.jones/postgresql-clean/tmp_install/usr/local/pgsql/bin/initdb
>   Reason: tried: '/usr/local/pgsql/lib/libpq.5.dylib' (no such file),
> '/System/Volumes/Preboot/Cryptexes/OS/usr/local/pgsql/lib/libpq.5.dylib'
> (no such file), '/usr/local/pgsql/lib/libpq.5.dylib' (no such file),
> '/usr/local/lib/libpq.5.dylib' (no such file), '/usr/lib/libpq.5.dylib' (no
> such file, not in dyld cache)
> 
> The reason is that at some point, Mac OS X started removing the
> DYLD_LIBRARY_PATH environment variable for "untrusted" executables [1]:
> "Any dynamic linker (dyld) environment variables, such as
> DYLD_LIBRARY_PATH, are purged when launching protected processes."
> 
> [1]
>
https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> 
> One solution is to explicitly pass the DYLD_LIBRARY_PATH environment
> variable to to the sub-process shell scripts that are run by pg_regress. To
> do this, I created an extra_envvars global variable which is set to the
> empty string "", but on Mac OS X, is filled in with "DYLD_LIBRARY_PATH=%s",
> where the %s is the current environment variable. The "make check" Makefile
> sets this environment variable to the temporary install directory, so this
> fixes the above errors.

Note that this is a known issue and a workaround is documented in the macos
specific notes at
https://www.postgresql.org/docs/current/installation-platform-notes.html#INSTALLATION-NOTES-MACOS:

> macOS's “System Integrity Protection” (SIP) feature breaks make check,
> because it prevents passing the needed setting of DYLD_LIBRARY_PATH down to
> the executables being tested. You can work around that by doing make install
> before make check. Most PostgreSQL developers just turn off SIP, though.

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Mon, Jun 05, 2023 at 09:47:30AM -0400, Evan Jones wrote:
>> This makes "make check" work on Mac OS X. Without this patch, on Mac OS X a
>> default "./configure; make; make check" fails with errors like:
>> ...
>> The reason is that at some point, Mac OS X started removing the
>> DYLD_LIBRARY_PATH environment variable for "untrusted" executables [1]:

> Note that this is a known issue

Yeah.  We have attempted to work around this before, but failed to find
a solution without more downsides than upsides.  I will be interested
to look at this patch, but lack time for it right now.  Anybody else?

            regards, tom lane

On 06.06.23 16:24, Evan Jones wrote:
> On Mon, Jun 5, 2023 at 10:33 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>      > Note that this is a known issue
>     Yeah.  We have attempted to work around this before, but failed to find
>     a solution without more downsides than upsides.  I will be interested
>     to look at this patch, but lack time for it right now.  Anybody else?
> 
> 
> Ah, I didn't find that mention in the documentation when I was trying to 
> get this working. Sorry about that!
> 
> My argument in favour of considering this patch is that making 
> "./configure; make; make check" work on current major operating systems 
> makes it easier for others to contribute in the future. I think the 
> disadvantage of this patch is it makes pg_regress harder to understand, 
> because it requires an #ifdef for this OS specific behaviour, and 
> obscures the command lines of the child processes it spawns.

This addresses only pg_regress.  What about all the other test suites? 
Per the previous discussions, you'd need to patch up other places in a 
similar way, potentially everywhere system() is called.

Evan Jones <evan.jones@datadoghq.com> writes:
> On Tue, Jun 6, 2023 at 5:23 PM Peter Eisentraut <peter@eisentraut.org>
> wrote:
>> This addresses only pg_regress.  What about all the other test suites?

> Are there instructions for how I can run these other test suites?

configure with --enable-tap-tests, then do "make check-world".

Also, adding certain additional feature arguments such as
--with-python enables more test cases.  We aren't going to be
super excited about a patch that doesn't handle all of them.

            regards, tom lane

On Wed, Jun 7, 2023 at 5:44 AM Evan Jones <evan.jones@datadoghq.com> wrote:
>
> On Tue, Jun 6, 2023 at 5:23 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> This addresses only pg_regress.  What about all the other test suites?
>> Per the previous discussions, you'd need to patch up other places in a
>> similar way, potentially everywhere system() is called.
>
>
> Are there instructions for how I can run these other test suites? The installation notes that Julien linked, and the
Postgreswiki Developer FAQ [1] only seem to mention "make check". I would be happy to try to fix other tests on Mac OS
X.

AFAIK there's no make rule that can really run everything.  You can
get most of it using make check-world (see
https://www.postgresql.org/docs/current/regress-run.html#id-1.6.20.5.5)
and making sure you added support for TAP tests (and probably also a
lot of optional dependencies) when running configure.  This won't run
everything but hopefully will hit most of the relevant infrastructure.

On Tue, Jun 06, 2023 at 09:48:24PM -0400, Tom Lane wrote:
> configure with --enable-tap-tests, then do "make check-world".
>
> Also, adding certain additional feature arguments such as
> --with-python enables more test cases.  We aren't going to be
> super excited about a patch that doesn't handle all of them.

There is a bit more to this story.  Mainly, see PG_TEST_EXTRA here:
https://www.postgresql.org/docs/devel/regress-run.html
--
Michael

I have applied the patch to the latest master branch and successfully executed './configure && make && make check' on
macOSVentura. However, during the process, a warning was encountered: "mixing declarations and code is incompatible
withstandards before C99 [-Wdeclaration-after-statement]". Moving the declaration of 'result' to the beginning like
belowcan resolve the warning, and it would be better to use a unique variable instead of 'result'. 
 

#ifdef __darwin__
static char extra_envvars[4096];
+int result = -1;
... ...
-int result = snprintf(extra_envvars, sizeof(extra_envvars), "DYLD_LIBRARY_PATH=%s",
+result = snprintf(extra_envvars, sizeof(extra_envvars), "DYLD_LIBRARY_PATH=%s",

After conducting a further investigation into this issue, I have made 
some discoveries. The previous patch successfully resolves the problem 
when running the commands `./configure && make && make check` (without 
any previous sudo make install or make install). However, it stops at 
the 'isolation check' when using the commands `./configure 
--enable-tap-tests && make && make check-world`.

To address this, I attempted to apply a similar approach as the previous 
patch, resulting in an experimental patch (attached). This new patch 
helps progress the 'make-world' process and passes the 'isolation 
check', but there are still several remaining issues that need to be 
addressed.


Currently, there is a description suggesting a workaround by running a 
'make install' command first, but I find it to be somewhat inaccurate. 
It would be better to update the existing description to provide more 
precise instructions on how to overcome this issue. Here are the changes 
I would suggest.

from:
"You can work around that by doing make install before make check. Most 
PostgreSQL developers just turn off SIP, though."

to:
"You can execute sudo make install if you do not specify a prefix during 
the configure step, or make install without sudo if you do specify a 
prefix (assuming proper permissions) before make check. Most PostgreSQL 
developers just turn off SIP, though."

Otherwise, following the current description, if you run `./configure  
&& make install` you will get error: "mkdir: /usr/local/pgsql: 
Permission denied"


Below are the steps I took that led to the discovery of additional issues.

git apply  pg_regress_mac_os_x_dyld.patch
./configure
make
make check

... ...
# All 215 tests passed.


./configure --enable-tap-tests
make
make check-world

... ...
echo "# +++ isolation check in src/test/isolation +++"
... ...
dyld[32335]: Library not loaded: /usr/local/pgsql/lib/libpq.5.dylib
   Referenced from: <EB3758C5-A87B-36C5-AA29-C1E31AD89E70> 
/Users/david/hg/sandbox/postgres/src/test/isolation/isolationtester
   Reason: tried: '/usr/local/pgsql/lib/libpq.5.dylib' (no such file), 
'/System/Volumes/Preboot/Cryptexes/OS/usr/local/pgsql/lib/libpq.5.dylib' 
(no such file), '/usr/local/pgsql/lib/libpq.5.dylib' (no such file), 
'/usr/local/lib/libpq.5.dylib' (no such file), '/usr/lib/libpq.5.dylib' 
(no such file, not in dyld cache)
no data was returned by command 
""/Users/david/hg/sandbox/postgres/src/test/isolation/isolationtester" -V"



git apply pg_regress_mac_os_x_dyld_isolation_check_only.patch
./configure --enable-tap-tests
make
make check-world

... ...
# All 215 tests passed.
... ...
# +++ isolation check in src/test/isolation +++
... ...
# All 112 tests passed.

echo "# +++ tap check in src/test/modules/brin +++"
... ...
# +++ tap check in src/test/modules/brin +++
t/01_workitems.pl ........ Bailout called.  Further testing stopped:  
command "initdb -D 
/Users/david/hg/sandbox/postgres/src/test/modules/brin/tmp_check/t_01_workitems_tango_data/pgdata 
-A trust -N" died with signal 6
t/01_workitems.pl ........ Dubious, test returned 255 (wstat 65280, 0xff00)
No subtests run


Any thoughts ?

Thank you

David

On 2023-06-16 2:25 p.m., David Zhang wrote:
> I have applied the patch to the latest master branch and successfully executed './configure && make && make check' on
macOSVentura. However, during the process, a warning was encountered: "mixing declarations and code is incompatible
withstandards before C99 [-Wdeclaration-after-statement]". Moving the declaration of 'result' to the beginning like
belowcan resolve the warning, and it would be better to use a unique variable instead of 'result'.
 
>
> #ifdef __darwin__
> static char extra_envvars[4096];
> +int result = -1;
> ... ...
> -int result = snprintf(extra_envvars, sizeof(extra_envvars), "DYLD_LIBRARY_PATH=%s",
> +result = snprintf(extra_envvars, sizeof(extra_envvars), "DYLD_LIBRARY_PATH=%s",

On 22.06.23 21:08, David Zhang wrote:
> Currently, there is a description suggesting a workaround by running a 
> 'make install' command first, but I find it to be somewhat inaccurate. 
> It would be better to update the existing description to provide more 
> precise instructions on how to overcome this issue. Here are the changes 
> I would suggest.
> 
> from:
> "You can work around that by doing make install before make check. Most 
> PostgreSQL developers just turn off SIP, though."
> 
> to:
> "You can execute sudo make install if you do not specify a prefix during 
> the configure step, or make install without sudo if you do specify a 
> prefix (assuming proper permissions) before make check. Most PostgreSQL 
> developers just turn off SIP, though."
> 
> Otherwise, following the current description, if you run `./configure && 
> make install` you will get error: "mkdir: /usr/local/pgsql: Permission 
> denied"

I think you should interpret "doing make install" as "running make 
install or a similar command as described earlier in this chapter". 
Note also that the installation instructions don't use "sudo" anywhere 
right now, so throwing it in at this point would be weird.

> echo "# +++ tap check in src/test/modules/brin +++"
> ... ...
> # +++ tap check in src/test/modules/brin +++
> t/01_workitems.pl ........ Bailout called.  Further testing stopped: 
> command "initdb -D 
> /Users/david/hg/sandbox/postgres/src/test/modules/brin/tmp_check/t_01_workitems_tango_data/pgdata -A trust -N" died
withsignal 6
 
> t/01_workitems.pl ........ Dubious, test returned 255 (wstat 65280, 0xff00)
> No subtests run

As I mentioned earlier, you would need to find all uses of system() in 
the PostgreSQL source code and make your adjustments there.  IIRC, the 
TAP tests require pg_ctl, so maybe look there.

Hi,

On 2023-06-05 22:33:16 -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > On Mon, Jun 05, 2023 at 09:47:30AM -0400, Evan Jones wrote:
> >> This makes "make check" work on Mac OS X. Without this patch, on Mac OS X a
> >> default "./configure; make; make check" fails with errors like:
> >> ...
> >> The reason is that at some point, Mac OS X started removing the
> >> DYLD_LIBRARY_PATH environment variable for "untrusted" executables [1]:
> 
> > Note that this is a known issue
> 
> Yeah.  We have attempted to work around this before, but failed to find
> a solution without more downsides than upsides.  I will be interested
> to look at this patch, but lack time for it right now.  Anybody else?

FWIW, I have a patch, which I posted originally as part of the meson thread,
that makes the meson build work correctly even with SIP enabled. The trick is
basically to change the absolute references to libraries to relative ones.

Except for a small amount of complexity during install, I don't think this has
a whole lot of downsides. Making the install relocatable imo is pretty nice.

I guess I should repost that for 17...

Greetings,

Andres Freund