Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Andres Freund]

Hi,

On 2023-06-05 22:33:16 -0400, Tom Lane wrote:
> Julien Rouhaud  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

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Peter Eisentraut]

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 with signal 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.

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[David Zhang]

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:  
/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 macOS Ventura. However, during the process, a warning was encountered: 
"mixing declarations and code is incompatible with standards before C99 
[-Wdeclaration-after-statement]". Moving the declaration of 'result' to the beginning like below can 
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",diff --git a/src/common/exec.c b/src/common/exec.c
index f209b934df..8cf2c21a66 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -74,6 +74,12 @@ static char *pg_realpath(const char *fname);
 static BOOL GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser);
 #endif
 
+#ifdef __darwin__
+static char extra_envvars[4096];
+#else
+static const char extra_envvars[] = "";
+#endif 
+
 /*
  * validate_exec -- validate "path" as an executable file
  *
@@ -330,6 +336,15 @@ find_other_exec(const char *argv0, const char *target,
charcmd[MAXPGPATH];
charline[MAXPGPATH];
 
+#ifdef __darwin__
+int result = 1;
+result = snprintf(extra_envvars, sizeof(extra_envvars), 
"DYLD_LIBRARY_PATH=%s",
+  getenv("DYLD_LIBRARY_PATH"));
+if (result < 0 || result >= sizeof(extra_envvars))
+{   
+printf("extra_envars too small for DYLD_LIBRARY_PATH");
+}
+#endif
if (find_my_exec(argv0, retpath) < 0)
return -1;
 
@@ -344,7 +359,7 @@ 

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[David Zhang]

I have applied the patch to the latest master branch and successfully executed 
'./configure && make && make check' on macOS Ventura. However, during the 
process, a warning was encountered: "mixing declarations and code is 
incompatible with standards before C99 [-Wdeclaration-after-statement]". Moving 
the declaration of 'result' to the beginning like below can 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",

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Julien Rouhaud]

On Wed, Jun 7, 2023 at 5:44 AM Evan Jones  wrote:
>
> On Tue, Jun 6, 2023 at 5:23 PM Peter Eisentraut  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 Postgres wiki 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.

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Tom Lane]

Evan Jones  writes:
> On Tue, Jun 6, 2023 at 5:23 PM Peter Eisentraut 
> 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

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Evan Jones]

On Tue, Jun 6, 2023 at 5:23 PM Peter Eisentraut 
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 Postgres wiki Developer FAQ
[1] only seem to mention "make check". I would be happy to try to fix other
tests on Mac OS X.

Thanks!

[1]
https://wiki.postgresql.org/wiki/Developer_FAQ#How_do_I_test_my_changes.3F

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Peter Eisentraut]

On 06.06.23 16:24, Evan Jones wrote:
On Mon, Jun 5, 2023 at 10:33 PM Tom Lane > 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.

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Evan Jones]

On Mon, Jun 5, 2023 at 10:33 PM Tom Lane  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.

Thanks for considering it!

Evan

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Tom Lane]

Julien Rouhaud  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

Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Julien Rouhaud]

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.

[PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH

[Evan Jones]

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.

I tested this on Mac OS X and on Linux (Ubuntu 23.04).

Thanks!

Evan Jones


pg_regress_mac_os_x_dyld.patch
Description: Binary data