Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. sepgsql: update TAP test to use fat comma style

  2. Convert sepgsql tests to TAP

  3. meson: Fix sepgsql installation

  1. Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2024-05-13T06:16:10Z

    The sepgsql tests have not been integrated into the Meson build system 
yet.  I propose to fix that here.

One problem there was that the tests use a very custom construction 
where a top-level shell script internally calls make.  I have converted 
this to a TAP script that does the preliminary checks and then calls 
pg_regress directly, without make.  This seems to get the job done. 
Also, once you have your SELinux environment set up as required, the 
test now works fully automatically; you don't have to do any manual prep 
work.  The whole thing is guarded by PG_TEST_EXTRA=sepgsql now.

Some comments and questions:

- Do we want to keep the old way to run the test?  I don't know all the 
testing scenarios that people might be interested in, but of course it 
would also be good to cut down on the duplication in the test files.

- Strangely, there was apparently so far no way to get to the build 
directory from a TAP script.  They only ever want to read files from the 
source directory.  So I had to add that.

- If you go through the pre-test checks in contrib/sepgsql/test_sepgsql, 
I have converted most of these checks to the Perl script.  Some of the 
checks are obsolete, because they check whether the database has been 
correctly initialized, which is now done by the TAP script anyway.  One 
check that I wasn't sure about is the

# 'psql' command must be executable from test domain

The old test was checking the installation tree, which I guess could be 
set up in random ways.  But do we need this kind of check if we are 
using a temporary installation?

As mentioned in the patch, the documentation needs to be updated.  This 
depends on the outcome of the question above whether we want to keep the 
old tests in some way.

  2. Re: Convert sepgsql tests to TAP

    Andreas Karlsson <andreas@proxel.se> — 2024-07-24T14:31:32Z

    I took a quick look at the patch and I like that we standardize things a 
bit. But one thing I am not a fan of are all the use of sed and awk in 
the Perl script. I would prefer if that logic happened all in Perl, 
especially since we have some of it in Perl (e.g. chomp). Also I wonder 
if we should not use IPC::Run to do the tests since we already depend on 
it for the other TAP tests.

I have not yet set up an VM with selinux to try the patch out for real 
but will do so later.

On 5/13/24 8:16 AM, Peter Eisentraut wrote:
> - Do we want to keep the old way to run the test?  I don't know all the 
> testing scenarios that people might be interested in, but of course it 
> would also be good to cut down on the duplication in the test files.

I cannot see why. Having two ways to run the tests seems only like a bad 
thing to me.

> - If you go through the pre-test checks in contrib/sepgsql/test_sepgsql, 
> I have converted most of these checks to the Perl script.  Some of the 
> checks are obsolete, because they check whether the database has been 
> correctly initialized, which is now done by the TAP script anyway.  One 
> check that I wasn't sure about is the
> 
> # 'psql' command must be executable from test domain
> 
> The old test was checking the installation tree, which I guess could be 
> set up in random ways.  But do we need this kind of check if we are 
> using a temporary installation?

Yeah, that does not seem necessary.

Andreas

  3. Re: Convert sepgsql tests to TAP

    Andreas Karlsson <andreas@proxel.se> — 2024-07-24T16:29:30Z

    On 7/24/24 4:31 PM, Andreas Karlsson wrote:
> I have not yet set up an VM with selinux to try the patch out for real 
> but will do so later.

I almost got the tests running but it required way too many manual steps 
to just get there and I gave up after just getting segfaults. I had to 
edit sepgsql-regtest.te because sepgsql-regtest.pp would not build 
otherwise on Debian bookworm, but after I had done that instead of 
getting test failures as I expected I just got segfaults. Maybe those 
are caused by an incorrect sepgsql-regtest.pp but this was not nice at 
all to try to get running for someone like me who does not know selinux 
well.

Peter, what did you do to get the tests running? And should we fix these 
tests to make them more user friendly?

Andreas

  4. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T16:31:28Z

    On 24.07.24 18:29, Andreas Karlsson wrote:
> On 7/24/24 4:31 PM, Andreas Karlsson wrote:
>> I have not yet set up an VM with selinux to try the patch out for real 
>> but will do so later.
> 
> I almost got the tests running but it required way too many manual steps 
> to just get there and I gave up after just getting segfaults. I had to 
> edit sepgsql-regtest.te because sepgsql-regtest.pp would not build 
> otherwise on Debian bookworm, but after I had done that instead of 
> getting test failures as I expected I just got segfaults. Maybe those 
> are caused by an incorrect sepgsql-regtest.pp but this was not nice at 
> all to try to get running for someone like me who does not know selinux 
> well.
> 
> Peter, what did you do to get the tests running? And should we fix these 
> tests to make them more user friendly?

In my experience, the tests (both the old and the proposed new) only 
work on Red Hat-like platforms.  I had also tried on Debian but decided 
that it won't work.

  5. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T16:33:53Z

    On 24.07.24 16:31, Andreas Karlsson wrote:
> I took a quick look at the patch and I like that we standardize things a 
> bit. But one thing I am not a fan of are all the use of sed and awk in 
> the Perl script. I would prefer if that logic happened all in Perl, 
> especially since we have some of it in Perl (e.g. chomp). Also I wonder 
> if we should not use IPC::Run to do the tests since we already depend on 
> it for the other TAP tests.

In principle yes, but here I tried not rewriting the tests too much but 
just port them to a newer environment.  I think the adjustments you 
describe could be done as a second step.

(I don't really have any expertise in sepgsql or selinux, I'm just doing 
this to reduce the dependency on makefiles for testing.  So I'm trying 
to use as light a touch as possible.)

  6. Re: Convert sepgsql tests to TAP

    Andreas Karlsson <andreas@proxel.se> — 2024-07-24T16:34:32Z

    On 7/24/24 6:31 PM, Peter Eisentraut wrote:
> On 24.07.24 18:29, Andreas Karlsson wrote:
>> Peter, what did you do to get the tests running? And should we fix 
>> these tests to make them more user friendly?
> 
> In my experience, the tests (both the old and the proposed new) only 
> work on Red Hat-like platforms.  I had also tried on Debian but decided 
> that it won't work.

Thanks, will try to run them on Rocky Linux when I have calmed down a 
bit. :)

Andreas

  7. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T16:36:10Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> In my experience, the tests (both the old and the proposed new) only 
> work on Red Hat-like platforms.  I had also tried on Debian but decided 
> that it won't work.

Yeah, Red Hat is pretty much the only vendor that has pushed SELinux
far enough to be usable by non-wizards.  I'm not surprised if there
are outright bugs in other distros' versions of it, as AFAIK
nobody else turns it on by default.

			regards, tom lane

  8. Re: Convert sepgsql tests to TAP

    Joe Conway <mail@joeconway.com> — 2024-07-24T16:42:11Z

    On 7/24/24 12:36, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> In my experience, the tests (both the old and the proposed new) only 
>> work on Red Hat-like platforms.  I had also tried on Debian but decided 
>> that it won't work.
> 
> Yeah, Red Hat is pretty much the only vendor that has pushed SELinux
> far enough to be usable by non-wizards.  I'm not surprised if there
> are outright bugs in other distros' versions of it, as AFAIK
> nobody else turns it on by default.

I tried some years ago to get it working on my Debian-derived Linux Mint 
desktop and gave up. I think SELinux is a really good tool on RHEL 
variants, but I don't think many people use it on anything else. As Tom 
says, perhaps there are a few wizards out there though...

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  9. Re: Convert sepgsql tests to TAP

    Andreas Karlsson <andreas@proxel.se> — 2024-07-24T19:54:37Z

    On 7/24/24 6:33 PM, Peter Eisentraut wrote:
> On 24.07.24 16:31, Andreas Karlsson wrote:
>> I took a quick look at the patch and I like that we standardize things 
>> a bit. But one thing I am not a fan of are all the use of sed and awk 
>> in the Perl script. I would prefer if that logic happened all in Perl, 
>> especially since we have some of it in Perl (e.g. chomp). Also I 
>> wonder if we should not use IPC::Run to do the tests since we already 
>> depend on it for the other TAP tests.
> 
> In principle yes, but here I tried not rewriting the tests too much but 
> just port them to a newer environment.  I think the adjustments you 
> describe could be done as a second step.

That reasoning makes a lot of sense and I am in agreement. Cleaning that 
up is best for another patch.

And managed to get the tests running on Rocky Linux 9 with both 
autotools and meson and everything work as it should.

So I have two comments:

1) As I said earlier I think we should remove the old code.

2) If we remove the old code I think the launcher script can be merged 
into the TAP test instead of being a separate shell script. But I am 
fine if you think that is also something for a separate commit.

I like this kind of clean up patch. Good work! :)

Andreas

  10. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T20:35:53Z

    Andreas Karlsson <andreas@proxel.se> writes:
> 1) As I said earlier I think we should remove the old code.

I agree that carrying two versions of the test doesn't seem great.
However, a large part of the purpose of test_sepgsql is to help
people debug their sepgsql setup, which is why it goes to great
lengths to print helpful error messages.  I'm worried that making
it into a TAP test will degrade the usefulness of that, simply
because the TAP infrastructure is pretty damn unfriendly when it
comes to figuring out why a test failed.  You have to know where
to even look for the test logfile, and then you have to ignore
a bunch of useless-to-you chatter.  I'm not sure if there is much
we can do to improve that.  (Although if we could, it would
yield benefits across the whole tree.)

OTOH, I suspect there are so few people using sepgsql that this
doesn't matter too much.  Probably most of them will be advanced
hackers who won't blink at digging through a TAP log.  We should
update the docs to explain that though.

			regards, tom lane

  11. Re: Convert sepgsql tests to TAP

    Andreas Karlsson <andreas@proxel.se> — 2024-07-24T21:03:45Z

    On 7/24/24 10:35 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> 1) As I said earlier I think we should remove the old code.
> 
> I agree that carrying two versions of the test doesn't seem great.
> However, a large part of the purpose of test_sepgsql is to help
> people debug their sepgsql setup, which is why it goes to great
> lengths to print helpful error messages.  I'm worried that making
> it into a TAP test will degrade the usefulness of that, simply
> because the TAP infrastructure is pretty damn unfriendly when it
> comes to figuring out why a test failed.  You have to know where
> to even look for the test logfile, and then you have to ignore
> a bunch of useless-to-you chatter.  I'm not sure if there is much
> we can do to improve that.  (Although if we could, it would
> yield benefits across the whole tree.)

For me personally the output from when running it with meson was good 
enough while the output when running with autotools was usable but 
annoying to work with. Meson's integration with TAP is pretty good. But 
with that said I am a power user and developer used to both meson and 
autotools. Unclear what skill we should expect from the target audience 
of test_sepgsql.

Andreas

  12. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-27T08:12:24Z

    On 24.07.24 23:03, Andreas Karlsson wrote:
> On 7/24/24 10:35 PM, Tom Lane wrote:
>> Andreas Karlsson <andreas@proxel.se> writes:
>>> 1) As I said earlier I think we should remove the old code.
>>
>> I agree that carrying two versions of the test doesn't seem great.
>> However, a large part of the purpose of test_sepgsql is to help
>> people debug their sepgsql setup, which is why it goes to great
>> lengths to print helpful error messages.  I'm worried that making
>> it into a TAP test will degrade the usefulness of that, simply
>> because the TAP infrastructure is pretty damn unfriendly when it
>> comes to figuring out why a test failed.  You have to know where
>> to even look for the test logfile, and then you have to ignore
>> a bunch of useless-to-you chatter.  I'm not sure if there is much
>> we can do to improve that.  (Although if we could, it would
>> yield benefits across the whole tree.)
> 
> For me personally the output from when running it with meson was good 
> enough while the output when running with autotools was usable but 
> annoying to work with. Meson's integration with TAP is pretty good. But 
> with that said I am a power user and developer used to both meson and 
> autotools. Unclear what skill we should expect from the target audience 
> of test_sepgsql.

Here is a new patch version.

I simplified the uses of sed and awk inside the Perl script.  I also 
fixed "make installcheck".  I noticed that meson installs sepgsql.sql 
into the wrong directory, so that's fixed also.  (Many of the 
complications in this patch set are because sepgsql is not an extension 
but a loose SQL script, of which it is now the only one.  Maybe 
something to address separately.)

I did end up deciding to keep the old test_sepgsql script, because it 
does have the documented purpose of testing existing installations.  I 
did change it so that it calls pg_regress directly, without going via 
make, so that the dependency on make is removed.

The documentation is also updated a little bit, but I kept it to a 
minimum, because I'm not really sure how up to date the existing 
documentation was.  It lists several steps in the test procedure that I 
didn't need to do.  Someone who knows more about the whole picture would 
need to look at that in more detail.

  13. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-24T12:25:29Z

    On 27.08.24 10:12, Peter Eisentraut wrote:
> Here is a new patch version.
> 
> I simplified the uses of sed and awk inside the Perl script.  I also 
> fixed "make installcheck".  I noticed that meson installs sepgsql.sql 
> into the wrong directory, so that's fixed also.  (Many of the 
> complications in this patch set are because sepgsql is not an extension 
> but a loose SQL script, of which it is now the only one.  Maybe 
> something to address separately.)
> 
> I did end up deciding to keep the old test_sepgsql script, because it 
> does have the documented purpose of testing existing installations.  I 
> did change it so that it calls pg_regress directly, without going via 
> make, so that the dependency on make is removed.

This has been committed.  And I understand there is a buildfarm client 
update available for the affected buildfarm members.

  14. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T14:00:36Z

    On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
> On 27.08.24 10:12, Peter Eisentraut wrote:
>> Here is a new patch version.
>>
>> I simplified the uses of sed and awk inside the Perl script.  I also 
>> fixed "make installcheck".  I noticed that meson installs sepgsql.sql 
>> into the wrong directory, so that's fixed also. (Many of the 
>> complications in this patch set are because sepgsql is not an 
>> extension but a loose SQL script, of which it is now the only one.  
>> Maybe something to address separately.)
>>
>> I did end up deciding to keep the old test_sepgsql script, because it 
>> does have the documented purpose of testing existing installations.  
>> I did change it so that it calls pg_regress directly, without going 
>> via make, so that the dependency on make is removed.
>
> This has been committed.  And I understand there is a buildfarm client 
> update available for the affected buildfarm members.



This should only be rhinoceros. Joe can pull this fix: 
https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db 
(or just copy the whole file from 
https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm)


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  15. Re: Convert sepgsql tests to TAP

    Joe Conway <mail@joeconway.com> — 2025-01-24T14:11:34Z

    On 1/24/25 09:00, Andrew Dunstan wrote:
> 
> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>> On 27.08.24 10:12, Peter Eisentraut wrote:
>>> Here is a new patch version.
>>>
>>> I simplified the uses of sed and awk inside the Perl script.  I also 
>>> fixed "make installcheck".  I noticed that meson installs sepgsql.sql 
>>> into the wrong directory, so that's fixed also. (Many of the 
>>> complications in this patch set are because sepgsql is not an 
>>> extension but a loose SQL script, of which it is now the only one.  
>>> Maybe something to address separately.)
>>>
>>> I did end up deciding to keep the old test_sepgsql script, because it 
>>> does have the documented purpose of testing existing installations.  
>>> I did change it so that it calls pg_regress directly, without going 
>>> via make, so that the dependency on make is removed.
>>
>> This has been committed.  And I understand there is a buildfarm client 
>> update available for the affected buildfarm members.
> 
> 
> 
> This should only be rhinoceros. Joe can pull this fix:
> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db
> (or just copy the whole file from
> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm)

Sure, I am delinquent on updating my buildfarm client so will do that as 
well as pull the fix. It might be the weekend before I have the time though.

This reminds me though that rhino is still running on CentOS 7 -- I need 
to upgrade that one of these days...

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  16. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T15:57:30Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>> This has been committed.  And I understand there is a buildfarm client 
>> update available for the affected buildfarm members.

> This should only be rhinoceros. Joe can pull this fix: 
> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db 
> (or just copy the whole file from 
> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm)

Looks like alligator needs some help here too.

			regards, tom lane

  17. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T19:44:54Z

    On 2025-01-24 Fr 10:57 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>>> This has been committed.  And I understand there is a buildfarm client
>>> update available for the affected buildfarm members.
>> This should only be rhinoceros. Joe can pull this fix:
>> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db
>> (or just copy the whole file from
>> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm)
> Looks like alligator needs some help here too.
>
> 			



That's an issue with the new TAP test - alligator isn't running the 
TestSepgsql module. lapwing has also had a TAP test failure.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  18. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T21:07:40Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-01-24 Fr 10:57 AM, Tom Lane wrote:
>> Looks like alligator needs some help here too.

> That's an issue with the new TAP test - alligator isn't running the 
> TestSepgsql module. lapwing has also had a TAP test failure.

Hmm.  Neither of those animals should be trying to run the sepgsql
test; they are not configured --with-selinux, and probably don't
even have libselinux installed.

Looking at the buildfarm client script, it looks to me like it
will unconditionally try to run TAP tests in every contrib directory
that has a "t" subdirectory.  Up to now, none of those needed to
be conditional ... but now we need some more awareness.  However,
if this theory is right, then pretty much every BF animal except
rhinoceros should be failing.  So there's some additional moving
part that I'm not seeing.  In any case, I think there's a BF
client script bug here somewhere, or else these animals are
running an old script version that does the wrong thing.

			regards, tom lane

  19. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T22:16:25Z

    On 2025-01-24 Fr 4:07 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net> writes:
>> On 2025-01-24 Fr 10:57 AM, Tom Lane wrote:
>>> Looks like alligator needs some help here too.
>> That's an issue with the new TAP test - alligator isn't running the
>> TestSepgsql module. lapwing has also had a TAP test failure.
> Hmm.  Neither of those animals should be trying to run the sepgsql
> test; they are not configured --with-selinux, and probably don't
> even have libselinux installed.
>
> Looking at the buildfarm client script, it looks to me like it
> will unconditionally try to run TAP tests in every contrib directory
> that has a "t" subdirectory.  Up to now, none of those needed to
> be conditional ... but now we need some more awareness.  However,
> if this theory is right, then pretty much every BF animal except
> rhinoceros should be failing.  So there's some additional moving
> part that I'm not seeing.  In any case, I think there's a BF
> client script bug here somewhere, or else these animals are
> running an old script version that does the wrong thing.



The new TAP test has:

+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsepgsql\b/)
+{
+   plan skip_all =>
+     'Potentially unsafe test sepgsql not enabled in PG_TEST_EXTRA';
+}


It looks like the error on alligator is coming from a NON TAP test:

rm -rf '/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build'/tmp_install
/usr/bin/mkdir -p '/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build'/tmp_install/log
make -C '../..' DESTDIR='/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build'/tmp_install install >'/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
make -j1  checkprep >>'/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
make: *** [../../src/Makefile.global:424: temp-install] Error 2


But why is it doing that? On my Ubuntu 22.04 dev instance, this test shows this as expected:

echo "# +++ tap check in contrib/sepgsql +++" && rm -rf '/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql'/tmp_check && /usr/bin/mkdir -p '/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql'/tmp_check && cd /home/andrew/pgl/pg_head/contrib/sepgsql && TESTLOGDIR='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql/tmp_check/log' TESTDATADIR='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql/tmp_check' PATH="/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/tmp_install/home/andrew/pgl/pg_head/root/HEAD/inst/bin:/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql:$PATH" LD_LIBRARY_PATH="/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/tmp_install/home/andrew/pgl/pg_head/root/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build'/tmp_install/initdb-template  PGPORT='65678' top_builddir='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql/../..' PG_REGRESS='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql/../../src/test/regress/pg_regress' share_contrib_dir='/home/andrew/pgl/pg_head/root/HEAD/pgsql.build/tmp_install/home/andrew/pgl/pg_head/root/HEAD/inst/share/postgresql/contrib' /usr/bin/prove -I /home/andrew/pgl/pg_head/src/test/perl/ -I /home/andrew/pgl/pg_head/contrib/sepgsql --timer t/*.pl
# +++ tap check in contrib/sepgsql +++
[17:06:18] t/001_sepgsql.pl .. skipped: Potentially unsafe test sepgsql not enabled in PG_TEST_EXTRA
[17:06:18]
Files=1, Tests=0,  0 wallclock secs ( 0.00 usr  0.00 sys +  0.03 cusr  0.00 csys =  0.03 CPU)
Result: NOTESTS
log files for step contrib-sepgsqlCheck:
==~_~===-=-===~_~== /home/andrew/pgl/pg_head/root/HEAD/pgsql.build/contrib/sepgsql/tmp_check/log/regress_log_001_sepgsql ==~_~===-=-===~_~==
[17:06:18.231](0.004s) 1..0 # SKIP Potentially unsafe test sepgsql not enabled in PG_TEST_EXTRA


As of now I'm confused ...


cheers

andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  20. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T22:29:27Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-01-24 Fr 4:07 PM, Tom Lane wrote:
>> Looking at the buildfarm client script, it looks to me like it
>> will unconditionally try to run TAP tests in every contrib directory
>> that has a "t" subdirectory.  Up to now, none of those needed to
>> be conditional ... but now we need some more awareness.

> The new TAP test has:

> +if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsepgsql\b/)
> +{
> +   plan skip_all =>
> +     'Potentially unsafe test sepgsql not enabled in PG_TEST_EXTRA';
> +}

Yeah, but to get to that point you have to get past "make install",
which requires compiling the code, which will absolutely not work
on these platforms.  alligator and lapwing are not reporting the
relevant log file, but what we do see is an install failure that
could well be down to a compile failure.

> But why is it doing that? On my Ubuntu 22.04 dev instance, this test shows this as expected:

I don't understand how you're compiling on Ubuntu ... does it
have selinux installed?  On a Mac for instance,

$ cd pgsql/contrib/sepgsql/
$ make install
...
In file included from database.c:21:
./sepgsql.h:17:10: fatal error: 'selinux/selinux.h' file not found
   17 | #include <selinux/selinux.h>
      |          ^~~~~~~~~~~~~~~~~~~
1 error generated.


> As of now I'm confused ...

Me too.  The way it looks from here, the farm should be all red,
but it isn't.

			regards, tom lane

  21. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T22:46:10Z

    I wrote:
> Yeah, but to get to that point you have to get past "make install",

Oh ... wait a second.  After further code reading I see that
the BF client sets NO_TEMP_INSTALL if check_install_is_complete
succeeds.  So evidently, that is successfully suppressing
"make install" on most animals, but not these two.  How come?

			regards, tom lane

  22. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T23:08:31Z

    I wrote:
> Oh ... wait a second.  After further code reading I see that
> the BF client sets NO_TEMP_INSTALL if check_install_is_complete
> succeeds.  So evidently, that is successfully suppressing
> "make install" on most animals, but not these two.  How come?

Got it: we can see on alligator that it installs (for instance)
test_parser.so into

/usr/bin/install -c -m 755  test_parser.so '/home/postgres/proj/build-farm-17/buildroot/HEAD/inst/lib/test_parser.so'

but what check_install_is_complete is looking for is

		$tmp_loc = "$tmp_loc/$install_dir";
		$bindir = "$tmp_loc/bin";
		$libdir = "$tmp_loc/lib/postgresql";
...
	my $res =
	  (      (-d $tmp_loc)
		  && (-f "$bindir/postgres" || -f "$bindir/postgres.exe")
		  && (-f "$libdir/hstore$suffix")
		  && (-f "$libdir/test_parser$suffix"));

That is, check_install_is_complete expects to see test_parser.so
under installdir/lib/postgresql/, but it's actually getting put
into installdir/lib/ because "postgres" appears earlier in the
path.  So we're forcing a bunch of useless "make install"s,
but that was never mission-critical until today.

Unsurprisingly, lapwing is also running under /home/postgres/.
Apparently no other BF animals are.

			regards, tom lane

  23. Re: Convert sepgsql tests to TAP

    Robins Tharakan <tharakan@gmail.com> — 2025-01-24T23:11:14Z

    On Sat, 25 Jan 2025 at 08:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> alligator and lapwing are not reporting the
> relevant log file, but what we do see is an install failure that
> could well be down to a compile failure.
>

You're probably right about that.
This is what I see in the install.log
(/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build/tmp_install/log/install.log)

make[2]: Entering directory
'/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build/contrib/sepgsql'
ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations
-fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -g -O2 -fPIC
-fvisibility=hidden -I. -I. -I../../src/include  -D_GNU_SOURCE
-I/usr/include/libxml2   -c -o database.o database.c
In file included from database.c:21:
sepgsql.h:17:10: fatal error: selinux/selinux.h: No such file or directory
   17 | #include <selinux/selinux.h>
      |          ^~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [<builtin>: database.o] Error 1
make[2]: Leaving directory
'/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build/contrib/sepgsql'
make[1]: *** [../../src/Makefile.global:435: checkprep] Error 2
make[1]: Leaving directory
'/home/postgres/proj/build-farm-17/buildroot/HEAD/pgsql.build/contrib/sepgsql'


-
robins

  24. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T23:20:57Z

    On 2025-01-24 Fr 6:08 PM, Tom Lane wrote:
> I wrote:
>> Oh ... wait a second.  After further code reading I see that
>> the BF client sets NO_TEMP_INSTALL if check_install_is_complete
>> succeeds.  So evidently, that is successfully suppressing
>> "make install" on most animals, but not these two.  How come?
> Got it: we can see on alligator that it installs (for instance)
> test_parser.so into
>
> /usr/bin/install -c -m 755  test_parser.so '/home/postgres/proj/build-farm-17/buildroot/HEAD/inst/lib/test_parser.so'
>
> but what check_install_is_complete is looking for is
>
> 		$tmp_loc = "$tmp_loc/$install_dir";
> 		$bindir = "$tmp_loc/bin";
> 		$libdir = "$tmp_loc/lib/postgresql";
> ...
> 	my $res =
> 	  (      (-d $tmp_loc)
> 		  && (-f "$bindir/postgres" || -f "$bindir/postgres.exe")
> 		  && (-f "$libdir/hstore$suffix")
> 		  && (-f "$libdir/test_parser$suffix"));
>
> That is, check_install_is_complete expects to see test_parser.so
> under installdir/lib/postgresql/, but it's actually getting put
> into installdir/lib/ because "postgres" appears earlier in the
> path.  So we're forcing a bunch of useless "make install"s,
> but that was never mission-critical until today.
>
> Unsurprisingly, lapwing is also running under /home/postgres/.
> Apparently no other BF animals are.



Oh, good catch!


I'll go and fix it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  25. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T23:41:36Z

    Robins Tharakan <tharakan@gmail.com> writes:
> On Sat, 25 Jan 2025 at 08:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> alligator and lapwing are not reporting the
>> relevant log file, but what we do see is an install failure that
>> could well be down to a compile failure.

> You're probably right about that.
> This is what I see in the install.log

> In file included from database.c:21:
> sepgsql.h:17:10: fatal error: selinux/selinux.h: No such file or directory
>    17 | #include <selinux/selinux.h>
>       |          ^~~~~~~~~~~~~~~~~~~
> compilation terminated.

Yeah, that's about what I expected.  As a workaround until Andrew
updates the BF client, you could do

-		$libdir = "$tmp_loc/lib/postgresql";
+		$libdir = "$tmp_loc/lib";

at about line 429 of PGBuild/Utils.pm

			regards, tom lane

  26. Re: Convert sepgsql tests to TAP

    Robins Tharakan <tharakan@gmail.com> — 2025-01-25T00:09:57Z

    On Sat, 25 Jan 2025 at 10:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yeah, that's about what I expected.  As a workaround until Andrew
> updates the BF client, you could do
>
> -               $libdir = "$tmp_loc/lib/postgresql";
> +               $libdir = "$tmp_loc/lib";
>
> at about line 429 of PGBuild/Utils.pm
>
>
Ack. Triggered a fresh HEAD run to see how it pans out.

postgres@dell:~/proj/build-farm-17$ head -430 PGBuild/Utils.pm | tail -3
                $bindir = "$tmp_loc/bin";
                $libdir = "$tmp_loc/lib";
        }

-
robins

  27. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-25T00:50:27Z

    On 2025-01-24 Fr 7:09 PM, Robins Tharakan wrote:
>
> On Sat, 25 Jan 2025 at 10:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>     Yeah, that's about what I expected.  As a workaround until Andrew
>     updates the BF client, you could do
>
>     -               $libdir = "$tmp_loc/lib/postgresql";
>     +               $libdir = "$tmp_loc/lib";
>
>     at about line 429 of PGBuild/Utils.pm
>
>
> Ack. Triggered a fresh HEAD run to see how it pans out.
>
> postgres@dell:~/proj/build-farm-17$ head -430 PGBuild/Utils.pm | tail -3
>                 $bindir = "$tmp_loc/bin";
>                 $libdir = "$tmp_loc/lib";
>         }
>

Here's the hot fix (which passed my test with a directory with pgsql in 
its path):


https://github.com/PGBuildFarm/client-code/commit/f6c6dd52d2959814452454890fb9838429c5c3e8


cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  28. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-25T01:27:02Z

    On 2025-01-24 Fr 7:50 PM, Andrew Dunstan wrote:
>
>
> Here's the hot fix (which passed my test with a directory with pgsql 
> in its path):
>
>
> https://github.com/PGBuildFarm/client-code/commit/f6c6dd52d2959814452454890fb9838429c5c3e8
>
>
>


Oops, wrong address on previous email. Julien, please update the address 
for your animal using the update_personality.pl script, and apply the 
fix above.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  29. Re: Convert sepgsql tests to TAP

    Robins Tharakan <tharakan@gmail.com> — 2025-01-25T11:18:13Z

    On Sat, 25 Jan 2025 at 11:57, Andrew Dunstan <andrew@dunslane.net> wrote:

> On 2025-01-24 Fr 7:50 PM, Andrew Dunstan wrote:
> > Here's the hot fix (which passed my test with a directory with pgsql
> > in its path):

>
> >
> https://github.com/PGBuildFarm/client-code/commit/f6c6dd52d2959814452454890fb9838429c5c3e8
>
>
Switching alligator's bf client (from v17 release tarball) to git; I see
HEAD passed fine.

-
robins

  30. Re: Convert sepgsql tests to TAP

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-25T21:55:15Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> This has been committed.  And I understand there is a buildfarm client 
> update available for the affected buildfarm members.

BTW, shouldn't the CF entry for this get closed now?

			regards, tom lane

  31. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-26T10:26:35Z

    On 25.01.25 22:55, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> This has been committed.  And I understand there is a buildfarm client
>> update available for the affected buildfarm members.
> 
> BTW, shouldn't the CF entry for this get closed now?

done

  32. Re: Convert sepgsql tests to TAP

    Joe Conway <mail@joeconway.com> — 2025-01-26T15:29:34Z

    On 1/24/25 09:00, Andrew Dunstan wrote:
> 
> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>> On 27.08.24 10:12, Peter Eisentraut wrote:
>>> Here is a new patch version.
>>>
>>> I simplified the uses of sed and awk inside the Perl script.  I also 
>>> fixed "make installcheck".  I noticed that meson installs sepgsql.sql 
>>> into the wrong directory, so that's fixed also. (Many of the 
>>> complications in this patch set are because sepgsql is not an 
>>> extension but a loose SQL script, of which it is now the only one.  
>>> Maybe something to address separately.)
>>>
>>> I did end up deciding to keep the old test_sepgsql script, because it 
>>> does have the documented purpose of testing existing installations.  
>>> I did change it so that it calls pg_regress directly, without going 
>>> via make, so that the dependency on make is removed.
>>
>> This has been committed.  And I understand there is a buildfarm client 
>> update available for the affected buildfarm members.
> 
> This should only be rhinoceros. Joe can pull this fix:
> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db
> (or just copy the whole file from
> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm)


Out of curiosity, what should have changed? I have so far done nothing 
to rhino, yet it continues to return OK even after Peter's change...

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  33. Re: Convert sepgsql tests to TAP

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-26T16:36:54Z

    On 2025-01-26 Su 10:29 AM, Joe Conway wrote:
> On 1/24/25 09:00, Andrew Dunstan wrote:
>>
>> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>>> On 27.08.24 10:12, Peter Eisentraut wrote:
>>>> Here is a new patch version.
>>>>
>>>> I simplified the uses of sed and awk inside the Perl script.  I 
>>>> also fixed "make installcheck".  I noticed that meson installs 
>>>> sepgsql.sql into the wrong directory, so that's fixed also. (Many 
>>>> of the complications in this patch set are because sepgsql is not 
>>>> an extension but a loose SQL script, of which it is now the only 
>>>> one.  Maybe something to address separately.)
>>>>
>>>> I did end up deciding to keep the old test_sepgsql script, because 
>>>> it does have the documented purpose of testing existing 
>>>> installations.  I did change it so that it calls pg_regress 
>>>> directly, without going via make, so that the dependency on make is 
>>>> removed.
>>>
>>> This has been committed.  And I understand there is a buildfarm 
>>> client update available for the affected buildfarm members.
>>
>> This should only be rhinoceros. Joe can pull this fix:
>> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db 
>>
>> (or just copy the whole file from
>> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm) 
>>
>
>
> Out of curiosity, what should have changed? I have so far done nothing 
> to rhino, yet it continues to return OK even after Peter's change...



It will run the old test redundantly. Peter left it in place.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  34. Re: Convert sepgsql tests to TAP

    Joe Conway <mail@joeconway.com> — 2025-01-26T19:42:46Z

    On 1/26/25 11:36, Andrew Dunstan wrote:
> 
> On 2025-01-26 Su 10:29 AM, Joe Conway wrote:
>> On 1/24/25 09:00, Andrew Dunstan wrote:
>>>
>>> On 2025-01-24 Fr 7:25 AM, Peter Eisentraut wrote:
>>>> On 27.08.24 10:12, Peter Eisentraut wrote:
>>>>> Here is a new patch version.
>>>>>
>>>>> I simplified the uses of sed and awk inside the Perl script.  I 
>>>>> also fixed "make installcheck".  I noticed that meson installs 
>>>>> sepgsql.sql into the wrong directory, so that's fixed also. (Many 
>>>>> of the complications in this patch set are because sepgsql is not 
>>>>> an extension but a loose SQL script, of which it is now the only 
>>>>> one.  Maybe something to address separately.)
>>>>>
>>>>> I did end up deciding to keep the old test_sepgsql script, because 
>>>>> it does have the documented purpose of testing existing 
>>>>> installations.  I did change it so that it calls pg_regress 
>>>>> directly, without going via make, so that the dependency on make is 
>>>>> removed.
>>>>
>>>> This has been committed.  And I understand there is a buildfarm 
>>>> client update available for the affected buildfarm members.
>>>
>>> This should only be rhinoceros. Joe can pull this fix:
>>> https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db 
>>>
>>> (or just copy the whole file from
>>> https://raw.githubusercontent.com/PGBuildFarm/client-code/refs/heads/main/PGBuild/Modules/TestSepgsql.pm) 
>>>
>>
>>
>> Out of curiosity, what should have changed? I have so far done nothing 
>> to rhino, yet it continues to return OK even after Peter's change...
> 
> 
> 
> It will run the old test redundantly. Peter left it in place.


I have pulled and applied the fix:
https://github.com/PGBuildFarm/client-code/commit/60b72787036090c6bf829f5cef2b0b3e60f2a2db


-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  35. Re: Convert sepgsql tests to TAP

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-01-28T13:34:36Z

    Peter Eisentraut <peter@eisentraut.org> writes:

> On 27.08.24 10:12, Peter Eisentraut wrote:
>> Here is a new patch version.
>> I simplified the uses of sed and awk inside the Perl script.  I also 
>> fixed "make installcheck".  I noticed that meson installs sepgsql.sql
>> into the wrong directory, so that's fixed also.  (Many of the 
>> complications in this patch set are because sepgsql is not an
>> extension but a loose SQL script, of which it is now the only one. 
>> Maybe something to address separately.)
>> I did end up deciding to keep the old test_sepgsql script, because it 
>> does have the documented purpose of testing existing installations.  I
>> did change it so that it calls pg_regress directly, without going via 
>> make, so that the dependency on make is removed.
>
> This has been committed.  And I understand there is a buildfarm client
> update available for the affected buildfarm members.

This patch passed the TAP command invocation cleanup patch mid-flight,
so didn't get the memo about command usng the fat comma for line option
arguments.  Here's a patch for bringing it in line with the new
convention.  I don't have any machines with SELinux enabled, so either
someone who has would need to test it, or we can rely on the buildfarm.

- ilmari

  36. Re: Convert sepgsql tests to TAP

    Peter Eisentraut <peter@eisentraut.org> — 2025-02-04T14:56:20Z

    On 28.01.25 14:34, Dagfinn Ilmari Mannsåker wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> On 27.08.24 10:12, Peter Eisentraut wrote:
>>> Here is a new patch version.
>>> I simplified the uses of sed and awk inside the Perl script.  I also
>>> fixed "make installcheck".  I noticed that meson installs sepgsql.sql
>>> into the wrong directory, so that's fixed also.  (Many of the
>>> complications in this patch set are because sepgsql is not an
>>> extension but a loose SQL script, of which it is now the only one.
>>> Maybe something to address separately.)
>>> I did end up deciding to keep the old test_sepgsql script, because it
>>> does have the documented purpose of testing existing installations.  I
>>> did change it so that it calls pg_regress directly, without going via
>>> make, so that the dependency on make is removed.
>>
>> This has been committed.  And I understand there is a buildfarm client
>> update available for the affected buildfarm members.
> 
> This patch passed the TAP command invocation cleanup patch mid-flight,
> so didn't get the memo about command usng the fat comma for line option
> arguments.  Here's a patch for bringing it in line with the new
> convention.  I don't have any machines with SELinux enabled, so either
> someone who has would need to test it, or we can rely on the buildfarm.

committed, thanks!