[pawank925]

Split writer/reader parts to support libzim built without writer (e.g. Android/reader-only builds).

Fixes #287

[codecov]

Codecov Report

❌ Patch coverage is 95.56962% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.29%. Comparing base (fabc288) to head (574dbf0).

Files with missing lines	Patch %	Lines
libzim/libzim.pyx	95.56%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   93.70%   95.29%   +1.59%     
==========================================
  Files           1        1              
  Lines         635      638       +3     
==========================================
+ Hits          595      608      +13     
+ Misses         40       30      -10     

☔ View full report in Codecov by Harness.
📢 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.

[pawank925]

@benoit74 once look into this please thanks!

[benoit74]

Automated review of this PR's reader/writer split. Findings below, each with a suggested fix where one is mechanical; the setup.py detection and CI-coverage ones are judgment calls for you to evaluate rather than blind applies.

[benoit74]

Reading all modifications in setup.py twice, I realize that automated detection seems too fragile IMHO ; I would rather prefer we resort only on an env variable LIBZIM_WITHOUT_WRITER which makes the choice explicit in all cases. Pretty concerned that we will have to force detection in some cases, and not build with writer in cases where we would have expected to.

This also makes codes much simpler, which is always good from a maintenance perspective

WDYT?

[pawank925]

@benoit74 agreed, and done — detection is gone entirely.

• detect_without_writer() and its header/pkg-config scanning are removed; LIBZIM_WITHOUT_WRITER is now the only switch, read once in Config (setup.py) and applied to both the C macro and the Cython compile-time env.
• The CI job now sets LIBZIM_WITHOUT_WRITER=1 instead of the temporary FORCE_ variable, and the README documents the variable instead of the heuristics.

The failure mode is now the one you want: if a reader-only build forgets the variable, the build tries to compile the writer bindings against a writer-less libzim and fails loudly at compile time, rather than silently producing a wheel without writer support. Nothing can silently downgrade a writer build either.

Verified after the change: default build 180 passed; LIBZIM_WITHOUT_WRITER=1 build 37 passed / 141 skipped, import libzim.writer

[benoit74]

LGTM

@rgaudin do you wanna have a second look now that PR seems ready to me?

[rgaudin]

Just skimmed through it. Looks good 👍

What bothers me is that we'll be publishing the very same libzim package on PyPi but the library is quite different on android, for reasons that are entirely Kiwix.

Now that the work for a reader-only libzim has been done, I think we should discuss (outside this PR) how we want to distribute this. I'd be happy to pull a read-only libzim in many scenarios. The extra scheme makes the most sense I believe but it could also be a different package.
If we keep it as it is, it mandates updates to the documentation.

On a personal note, I'd find LIBZIM_WITH_WRITER=0 better than LIBZIM_WITHOUT_WRITER=1

[benoit74]

Thank you @rgaudin

What bothers me is that we'll be publishing the very same libzim package on PyPi but the library is quite different on android, for reasons that are entirely Kiwix.

This is unfortunately the decision which has already been taken at libzim level, python-libzim can only cope with that decision and report concern upstream. I will do so.

Now that the work for a reader-only libzim has been done, I think we should discuss (outside this PR) how we want to distribute this. I'd be happy to pull a read-only libzim in many scenarios. The extra scheme makes the most sense I believe but it could also be a different package.

Same problem ; we do not have split between read-only + write-only libzim binaries on most platforms, so we can't build a read-only python-libzim with a "writer" extra ; this requires to have libzim-core + libzim-writer binaries. Otherwise we just remove the writer part of python-libzim while still pulling the whole libzim binaries ... which does not really makes sense (no strong added value IMHO).

If we keep it as it is, it mandates updates to the documentation.

Indeed ; since there is no platform building without writer in this PR, it does not seems reasonable to do in this PR ; but we clearly need to document in #283 that Android platform is available but without writer module.

On a personal note, I'd find LIBZIM_WITH_WRITER=0 better than LIBZIM_WITHOUT_WRITER=1

LIBZIM_WITHOUT_WRITER is mimicking libzim --without-writer flag ; not saying we must follow all libzim conventions, but here it seems reasonable IMHO.

[benoit74]

Opened openzim/libzim#1139 to discuss the "extra" approach.