Skip to content

Conversation

@ojwb
Copy link
Contributor

@ojwb ojwb commented Dec 11, 2025

Xapian 1.4.12 and later have a Database::size() method which returns the number of shards, so use this instead which fixes a compilation failure with Xapian 2.0.x where the internal object is no longer a std::vector.

For older versions, we still need to peek at the internal object but assuming internal details is less problematic there at least.

@ojwb
Copy link
Contributor Author

ojwb commented Dec 11, 2025

Duplicating the old version of the code seems clumsy, but I didn't see a way to combine the conditionals - obvious options such as this fail if XAPIAN_AT_LEAST isn't defined:

#if defined(XAPIAN_AT_LEAST) && XAPIAN_AT_LEAST(1,4,12)

I tested by appending an X so the macro isn't defined and I get:

../src/suggestion.cpp:103:50: error: missing binary operator before token ‘(’
  103 | #if defined(XAPIAN_AT_LEASTX) && XAPIAN_AT_LEASTX(1,4,12)

XAPIAN_AT_LEAST was added in Xapian 1.4.2 so if you're happy to require that meson.build could probably specify it and XAPIAN_AT_LEAST just be used unconditionally. 1.4.2 was released 9 years ago now.

@ojwb
Copy link
Contributor Author

ojwb commented Dec 11, 2025

It looks like the defacto requirement is at least Xapian 1.4.0 currently (since Xapian::LatLongCoord and related classes are used unconditionally).

@kelson42 kelson42 added this to the 9.5.0 milestone Dec 12, 2025
@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.18%. Comparing base (9ca8eb0) to head (e3c39f7).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1018   +/-   ##
=======================================
  Coverage   56.18%   56.18%           
=======================================
  Files         101      101           
  Lines        4989     4989           
  Branches     2170     2170           
=======================================
  Hits         2803     2803           
  Misses        739      739           
  Partials     1447     1447           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's safe to use XAPIAN_AT_LEAST unconditionally. You provided enough justification for doing so.

I also think that we can afford requiring a minimum version of 1.4.12 (or higher) for our Xapian dependency. Ubuntu 22.04 which is the oldest build platform we currently support across the Kiwix projects has 1.4.18.
@kelson42 Do you agree? Or libzim has to provide broader guarantees for backward compatibility compared to libkiwx?

@kelson42
Copy link
Contributor

I also think that we can afford requiring a minimum version of 1.4.12

I confirm

@ojwb
Copy link
Contributor Author

ojwb commented Dec 12, 2025

I also think that we can afford requiring a minimum version of 1.4.12

I confirm

OK, I'll update the patch.

Should anyone want to try to build on an older Ubuntu, 20.04 has 1.4.14 packaged and I also provide a PPA with backports of recent Xapian releases which has xapian-core 1.4.22 for Ubuntu 16.04 and later:

https://launchpad.net/~xapian/+archive/ubuntu/backports

(Not sure why it lacks trusty (14.04) packages for xapian-core but has them for the others.)

Xapian 1.4.12 and later have a Database::size() method which returns
the number of shards, so use this instead which fixes a compilation
failure with Xapian 2.0.x where the internal object is no longer a
std::vector.

This raises the Xapian version requirement to 1.4.12 (released 2019,
available in Ubuntu 20.04 and later), which is now checked by
meson.build (previously there wasn't a checked requirement
but at least 1.4.0 was needed for e.g. Xapian::LatLongCoord).
@ojwb ojwb force-pushed the avoid-using-xapian-internals branch from c74e2d0 to e3c39f7 Compare December 12, 2025 18:55
@kelson42 kelson42 merged commit 68c717d into openzim:main Dec 13, 2025
26 of 27 checks passed
@ojwb ojwb deleted the avoid-using-xapian-internals branch December 13, 2025 19:21
@kelson42 kelson42 modified the milestones: 9.5.0, 9.4.1 Jan 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants