docs: fix Linux deps and improve contribution setup instructions#291
docs: fix Linux deps and improve contribution setup instructions#291its-me-maady wants to merge 1 commit intoopenzim:mainfrom
Conversation
benoit74
left a comment
There was a problem hiding this comment.
@its-me-maady I want this PR to fix once (and hopefully for all) the list of dependencies.
With your changes, we still have inconsistent things between the list and instructions for macOS, Linux and Alpine.
We need to ensure this is well understood, documented, or fixed.
I also feel like explaining how to install Pillow is beyond the scope of what we can reasonably maintain, so I would prefer we simply consider most users will do the basic install, and then redirect anyone needing to build from source to go to Pillow documentation.
I also like the way Pillow describe which optional dependencies are required for some features: Optionally, install [defusedxml](https://pypi.org/project/defusedxml/) for Pillow to read XMP data, and [olefile](https://pypi.org/project/olefile/) for Pillow to read FPX and MIC images. I think we should strive to have the same kind of wording since dependencies are needed only for few scraperlib features (e.g. wget is used only in few functions).
|
|
||
| ## Linux | ||
|
|
||
| ## Linux |
There was a problem hiding this comment.
Why a duplicate section?
7a18d59 to
7a71230
Compare
|
Updated — removed all Pillow build-from-source packages across macOS, Linux, and Alpine, described each dependency by feature (wget, FFmpeg, gifsicle, libcairo), linked to Pillow docs for anyone needing to build from source, fixed the duplicate Linux header. |
benoit74
left a comment
There was a problem hiding this comment.
Few questions/remarks:
- I don't think it is correct to say the dependencies are install
by PyPI, they are rather installby pip(andfrom PyPiby default, but this depends on local pip configuration which might use a local mirror, or whatever else) - do we really need to install
libmagic, isn't this installed bypython-magic? - we should probably make it clearer that all dependencies are obviously needed to have all tests pass
- how did you tested you can drop all dependencies you've removed? For instance I'm a bit worried about the fact you remove
libjpeg, pretty sure the "Alpine" case has been added by someone who tried to build/test the lib on Alpine and faced a problem. But problem/dependency might be gone, hence my question about how did you tested, because it is highly possible we just don't know it is not needed anymore
7a71230 to
28d3fe8
Compare
|
Honestly, I didn't test it — the reasoning was that Pillow's pre-built wheels on macOS and Linux bundle their own image libraries, so those build deps shouldn't be needed at runtime. But I have no actual proof of this on a clean machine. |
|
We cannot just throw changes just hopping they are right. This is the exact reason we've not made any change, because it is hard to check changes are correct so that we We hence need (you or someone else) to test all these changes (in a realistic fashion, e.g. we won't test all Linux distro, a "standard" one like Debian or Ubuntu is ok) to confirm what needs to be installed manually and what is automatically installed by pip. |
- Replace obsolete libjpeg8-dev with libjpeg-dev (virtual metapackage, works on both Debian and Ubuntu); clarify that -dev headers are only needed when building Pillow from source, not for normal development - Rewrite contribution setup to use `hatch shell` (consistent with other openZIM repos like ted, warc2zim, youtube); clarify that all commands must be run from the local clone root; add note on what pre-commit install does Closes openzim#152, Closes openzim#153
28d3fe8 to
b031141
Compare
|
Tested everything properly this time with Docker on both Commands used: Debian: docker run --rm -v $(pwd):/app -w /app python:3.14-slim sh -c "
apt-get update -q && apt-get install -y -q libmagic1 wget ffmpeg gifsicle libcairo2 bash
pip install '.[dev]' -q --root-user-action=ignore
invoke coverage"Alpine: docker run --rm -v $(pwd):/app -w /app python:3.14-alpine sh -c "
apk add -q ffmpeg gifsicle libmagic wget cairo bash
pip install '.[dev]' -q --root-user-action=ignore
invoke coverage"Also noticed Only thing I couldn't test is macOS since I don't have a Mac, but |
hatch shell(consistent with other openZIM repos like ted, warc2zim, youtube); clarify that all commands must be run from the local clone root; add note on what pre-commit install doesCloses #152, Closes #153