Thread: Updated packaging for MobilityDB 1.1.0~beta1

Hello,

I have just pushed an update to the Debian packaging of MobilityDB to
salsa.debian.org [1] that addresses the feedback [2] from the first
attempt at packaging this extension. The build passes the GitLab CI
pipeline and I have verified that the package builds and autopkgtests
pass on arm64 (via sbuild). Below is a summary of how I have addressed
the comments/questions raised in the previous review. As time permits, I
would appreciate another review.

* I have updated debian/copyright to more accurately reflect the
  copyrights found in the source. I bootstrapped this file using cme[3]
  and then manually addressed all of the TODOs. I've also ensured that
  lintian doesn't raise any errors or warnings related to d/copyright.
* I've updated d/tests/installcheck to avoid the use of
  shared_preload_libraries when creating the test cluster
    * Tests instead set session_preload_libraries via PGOPTIONS to load
      the postgis extension. I don't think it is possible to link
      against postgis since it is not a shared library (no SONAME).
* I've updated d/control.in so that PGVERSION is no longer used --
  declaring a dependency on postgresql-postgis is sufficient

Thanks,

-- Bradford


[1]: https://salsa.debian.org/postgresql/mobilitydb/-/commit/9dfff5eee4eab17b8d20ecd408ed10b830750244
[2]: https://www.postgresql.org/message-id/ZMfMWcLtZIVxbDYj%40msg.df7cb.de
[3]: https://wiki.debian.org/CopyrightReviewTools#cme

Re: Bradford Boyle
> Hello,
> 
> I have just pushed an update to the Debian packaging of MobilityDB to
> salsa.debian.org [1] that addresses the feedback [2] from the first
> attempt at packaging this extension.

Thanks!

> * I've updated d/control.in so that PGVERSION is no longer used --
>   declaring a dependency on postgresql-postgis is sufficient

In postgresql-XX-mobility, we need the XX version of
postgresql-XX-postgis-3, not just any version, so this change isn't
correct.

The usage of "postgresql-postgis" in the Build-Depends is also fishy,
but the "update debian/control from control.in" machinery doesn't
support using PGVERSION in the Source section yet. I tried
implementing that the other week, but didn't get to finish it yet,
hopefully that will happen over the next days.

I haven't investigated the cause yet, but the pgdg build is failing:

https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/2/architecture=amd64,distribution=sid/console

14:56:48 dh_missing: warning: usr/lib/postgresql/16/lib/libMobilityDB-1.1.so exists in debian/tmp but is not installed
toanywhere
 
14:56:48 dh_missing: warning: usr/share/postgresql/16/extension/mobilitydb--1.1.0.sql exists in debian/tmp but is not
installedto anywhere
 
14:56:48 dh_missing: warning: usr/share/postgresql/16/extension/mobilitydb.control exists in debian/tmp but is not
installedto anywhere
 
14:56:48 dh_missing: error: missing files, aborting

Christoph

Re: To Bradford Boyle
> In postgresql-XX-mobility, we need the XX version of
> postgresql-XX-postgis-3, not just any version, so this change isn't
> correct.
> 
> The usage of "postgresql-postgis" in the Build-Depends is also fishy,
> but the "update debian/control from control.in" machinery doesn't
> support using PGVERSION in the Source section yet. I tried
> implementing that the other week, but didn't get to finish it yet,
> hopefully that will happen over the next days.

Hi Bradford,

I've done that now, it will be available in postgresql-common 256.

I pushed a change that uses it and also loops over the versions that
should be targeted for cmake.

Ideally, testing the extension could also use this from
debian/tests/installcheck:

pg_buildext run_installed /usr/bin/cmake -Btest-%v
pg_buildext run_installed make -C test-% test

... but that starts by recompiling the package instead of just running
the tests.

Perhaps you have an idea how to make it work? The existing
debian/tests/regress.sh file looks very complex and might break with
every little upstream change. (Of course if there's no easier way to
get the upstream testsuite to run, we have to go the complex route.)

Christoph

Hi Christoph,

> I've done that now, it will be available in postgresql-common 256.

Thank you!

> pg_buildext run_installed /usr/bin/cmake -Btest-%v
> pg_buildext run_installed make -C test-% test
>
> ... but that starts by recompiling the package instead of just running
> the tests.
>
> Perhaps you have an idea how to make it work? The existing
> debian/tests/regress.sh file looks very complex and might break with
> every little upstream change. (Of course if there's no easier way to
> get the upstream testsuite to run, we have to go the complex route.)

I agree with your assessment of d/tests/regress.sh. The reason I went
that route initially was because running cmake followed by make test was
rebuilding the package. I did see that autopkgtest has the
'build-needed' restriction [1] but its use seems to be discouraged. I
took another look at the upstream testsuite to see if we can (easily)
run its testsuite without require a rebuild and I think I have found a
way.

In short, upstream was defining a test called 'build' that would rebuild
the extension. We can instruct ctest to exclude this test by specifying
a regex. I was able to drop regress.sh altogether and have
d/tests/installcheck run

cmake -Btest-%v
make -C test-%v ARGS='-E build' test

This does require repeating the list of -dev packages in d/tests/control
so that cmake can generate the Makefiles. If this sounds like a good
approach to you, I can push the commit to s.d.o.

--Bradford

[1]: https://people.debian.org/~eriberto/README.package-tests.html

Re: Bradford Boyle
> In short, upstream was defining a test called 'build' that would rebuild
> the extension. We can instruct ctest to exclude this test by specifying
> a regex. I was able to drop regress.sh altogether and have
> d/tests/installcheck run
> 
> cmake -Btest-%v
> make -C test-%v ARGS='-E build' test
> 
> This does require repeating the list of -dev packages in d/tests/control
> so that cmake can generate the Makefiles. If this sounds like a good
> approach to you, I can push the commit to s.d.o.

Sure, that sounds sane.

Christoph

Hi Bradford,

where are we with MobilityDB? Are the remaining test failures a
problem in the source, or just artifacts of the way we try to run the
tests?

Christoph

Hi Christoph,

> where are we with MobilityDB? Are the remaining test failures a
> problem in the source, or just artifacts of the way we try to run the
> tests?

I pushed a commit to s.d.o this past weekend updating the version from
1.1.0~rc.1 to 1.1.0. Looking at the latest build on pgdgbuild [1], I see
that there is a failure for sid amd64. I've only looked at the build
output briefly but it looks like the failure is building against
postgres 12:

    meos/src/general/tsequence.c:3116:19: error: implicit declaration
of function ‘hash_bytes_uint32’;

There have been some recent commits on this file (tsequence.c) between
1.1.0~rc.1 and 1.1.0 but I'll need to investigate more to figure out a
fix.

--Bradford


[1]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/8/

Re: Bradford Boyle
> I pushed a commit to s.d.o this past weekend updating the version from
> 1.1.0~rc.1 to 1.1.0. Looking at the latest build on pgdgbuild [1], I see
> that there is a failure for sid amd64. I've only looked at the build
> output briefly but it looks like the failure is building against
> postgres 12:
> 
>     meos/src/general/tsequence.c:3116:19: error: implicit declaration
> of function ‘hash_bytes_uint32’;
> 
> There have been some recent commits on this file (tsequence.c) between
> 1.1.0~rc.1 and 1.1.0 but I'll need to investigate more to figure out a
> fix.

If it's not trivial to fix, there is always the option to exclude the
older PG versions from the build. 12 is going to be EOL soonish
anyway.

Christoph

Hi Chistoph,

I pushed a commit to s.d.o dropping support for Postgres 12 for
MobilityDB. Can you trigger another build on pgdgbuild?

Thanks,
--Bradford

Hi Christoph, Bradford,

Re: Christoph Berg
 > If it's not trivial to fix, there is always the option to exclude the
 > older PG versions from the build. 12 is going to be EOL soonish
 > anyway.

Re: Bradford Boyle
 > I pushed a commit to s.d.o dropping support for Postgres 12 for
 > MobilityDB. Can you trigger another build on pgdgbuild?

In case you still want to keep support for Postgres 12, we just 
published a bugfix release of MobilityDB (v1.1.1) which contains the fix 
needed to run on pg12.
Let us know if there is any way we can help in this packaging effort.

Best,
Maxime

I've updated the Debian package to build MobilityDB v1.1.1 and added
back Postgres 12 support.

-- Bradford

Hi Christop,

I was reviewing the latest builds for MobilityDB on pgdgbuild and it
looks like autopkgtest is still failing. The issue is that the cmake
build assumes the source tree is writable and the source tree is not
checked out in the same directory as AUTOPKGTEST_TMP. Here are two lines
from one of the failed autopkgtest runs:

    chmod -R go+rwX /tmp/autopkgtest.R3ZFXt/autopkgtest_tmp
    ...
    CMake Error: Could not open file for write in copy operation
/tmp/autopkgtest.R3ZFXt/tree/postgis/postgis_config.h.tmp

This is why recursively chmod-ing the AUTOPKGTEST_TMP didn't work. I
have a fix in mind but need to find some time to verify it.

--Bradford

Hello Maxime,

I have a development branch [1] for the Debian packaging for MobilityDB
1.1.1 that contains some fixes for the build that allow running the
autopkgtest suite for MobilityDB on pgdgbuild [2]. I needed to update a
couple of CMakeLists.txt to write generated configuration files to the
build directory instead of back to the source directory.

The linked job builds one Debian package per supported Postgres version
and then runs MobilityDB's tests against the installed package. It looks
like there are four failing tests for Postgres 12, 13, 14, 15, and 16:

* 053_tpoint_inout_tbl (Failed)
  - problem in alloc set ExprContext: detected write past chunk end in
    block 0x560f423d2de0, chunk 0x560f423d2e08
* 070_tpoint_spatialrels (Failed)
* 091_tnpoint_routeops_gin_tbl (Failed)
  - invalid memory alloc request size 2139062143
* 091_tnpoint_routeops_tbl (Failed)
  - invalid memory alloc request size 2139062143

You can view the full console output (which includes both building and
running the tests) here [3].

Regards,
-- Bradford

[1]: https://salsa.debian.org/postgresql/mobilitydb/-/tree/dev-pgdg-autopkgtest?ref_type=heads
[2]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/
[3]: https://pgdgbuild.dus.dg-i.net/job/mobilitydb-binaries/16/architecture=amd64,distribution=sid/consoleText

Hi Bradford,

We have been wanting to avoid writing the config files in the source 
tree for some time now, so the patch with the fixed for the build is 
very much appreciated.
You are welcome to send a PR to the develop branch [1]on the mobilitydb 
repository with these changes.

Concerning the failing tests, we indeed did not catch these as they only 
appear when running postgres with debug enabled.
We will update our tests to avoid missing these in the future.
In the mean time, I have fixed the issues on our develop branch [2], so 
we could produce a new bugfix release with these fixes and the build 
patch if this is the only remaining blocker.
I am attaching the patch with the fixes so you can test it out before we 
produce the release.

Let us know if there are any other issues or if we can assist you in 
producing the PR with the build patch.

Best,
Maxime

[1]: https://github.com/MobilityDB/MobilityDB/tree/develop
[2]: 

https://github.com/mobilityDB/MobilityDB/compare/942b10c47e7966f3d0b61a1d3a66e0400ee4769d...6e0e6572072c90dade2148d87ecaba6f5f072ddb

Hello,

I’ve updated the packaging for MobilityDB 1.1.1 to address the
autopkgtest issues when building on pgdg. MobilityDB does not build on
buster becuase of the old version of libproj; the package also fails to
build on i386. Additionally there are test failures on s390x. Attached
is a patch for pgapt to exclude these (buster, i386, and s390z) from the
build.

--Bradford

Re: SCHOEMANS Maxime
> Let us know if there are any other issues or if we can assist you in 
> producing the PR with the build patch.

Hi,

one more thing that would be nice: Every time you invoke `psql` during
testing, it should really be `psql -X` so it doesn't read the user's
~/.psqlrc file. In my case, the `\pset linestyle unicode` and
`\timing` in there cause a lot of noise in the test results.

Re: Bradford Boyle
> I’ve updated the packaging for MobilityDB 1.1.1 to address the
> autopkgtest issues when building on pgdg. MobilityDB does not build on
> buster becuase of the old version of libproj; the package also fails to
> build on i386. Additionally there are test failures on s390x. Attached
> is a patch for pgapt to exclude these (buster, i386, and s390z) from the
> build.

I've finally found the time to get back to this; the build is running.

Sorry, sometimes I'm overwhelmed by TODO items and then simply drop
some of them indefinitely.

Cheers,
Christoph

Hi,

Thanks a lot for your work.

 > one more thing that would be nice: Every time you invoke `psql` during
 > testing, it should really be `psql -X` so it doesn't read the user's
 > ~/.psqlrc file. In my case, the `\pset linestyle unicode` and
 > `\timing` in there cause a lot of noise in the test results.

Indeed, this is something we did not think of.
I changed this on both master and stable-1.1, so it will be fixed in all 
upcoming releases.

Best,
Maxime

On 07/05/2024 17:13, Christoph Berg wrote:
> Re: SCHOEMANS Maxime
>> Let us know if there are any other issues or if we can assist you in
>> producing the PR with the build patch.
> Hi,
>
> one more thing that would be nice: Every time you invoke `psql` during
> testing, it should really be `psql -X` so it doesn't read the user's
> ~/.psqlrc file. In my case, the `\pset linestyle unicode` and
> `\timing` in there cause a lot of noise in the test results.
>
> Re: Bradford Boyle
>> I’ve updated the packaging for MobilityDB 1.1.1 to address the
>> autopkgtest issues when building on pgdg. MobilityDB does not build on
>> buster becuase of the old version of libproj; the package also fails to
>> build on i386. Additionally there are test failures on s390x. Attached
>> is a patch for pgapt to exclude these (buster, i386, and s390z) from the
>> build.
> I've finally found the time to get back to this; the build is running.
>
> Sorry, sometimes I'm overwhelmed by TODO items and then simply drop
> some of them indefinitely.
>
> Cheers,
> Christoph