Обсуждение: 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
Dear all (cc MobilityDB Team and Regina Obe)
However, as this release was not running on ARM, we were waiting to produce a v1.1.1 release with small bug fixes that allows MobilityDB to run on the ARM architecture. This is now almost complete and we are thus planning to release and announce MobilityDB 1.1.1 either at the end of this week or next week.
The MobilityDB 1.1 version was released two weeks ago.
See: https://github.com/MobilityDB/MobilityDB/releases/tag/v1.1.0However, as this release was not running on ARM, we were waiting to produce a v1.1.1 release with small bug fixes that allows MobilityDB to run on the ARM architecture. This is now almost complete and we are thus planning to release and announce MobilityDB 1.1.1 either at the end of this week or next week.
Regards
Esteban
------------------------------------------------------------
Prof. Esteban Zimanyi
Department of Computer & Decision Engineering (CoDE) CP 165/15
Universite Libre de Bruxelles
Avenue F. D. Roosevelt 50
B-1050 Brussels, Belgium
fax: + 32.2.650.47.13
tel: + 32.2.650.31.85
e-mail: esteban.zimanyi@ulb.be
Internet: http://cs.ulb.ac.be/members/esteban/
------------------------------------------------------------
Prof. Esteban Zimanyi
Department of Computer & Decision Engineering (CoDE) CP 165/15
Universite Libre de Bruxelles
Avenue F. D. Roosevelt 50
B-1050 Brussels, Belgium
fax: + 32.2.650.47.13
tel: + 32.2.650.31.85
e-mail: esteban.zimanyi@ulb.be
Internet: http://cs.ulb.ac.be/members/esteban/
------------------------------------------------------------
On Thu, Apr 4, 2024 at 2:12 PM Christoph Berg <myon@debian.org> wrote:
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