Reviews for imagewmark
All reviews for this package from team members (across all versions).
| Reviewer | Version | Allocated | Started | Status | Completed | Comment | |
|---|---|---|---|---|---|---|---|
| awm | 0.6.0-1 | 25 days ago | 25 days ago | rejected | 25 days ago | Hi, There are a number of issues which need to be resolved, but none are particularly complex and none relate to licensing. 1. [blocker] packaging Python code imports scipy.misc, removed in SciPy >= 1.12: program cannot start and the build’s smoke test fails Files: src/common.py:3, src/embed.py:4, src/extract.py:11, src/imagewmark.py:19, debian/rules Evidence: import scipy.misc, scipy.signal, cv2 (unconditional top-level import in every one of these modules). Problem: scipy.misc was deprecated in SciPy 1.10 and removed in 1.12; Debian currently ships python3-scipy 1.17.1-5+b1, which has no scipy.misc. The imports are executed at module load, before argument parsing, so imagewmark cannot start at all, the override_dh_auto_test smoke run (./imagewmark --help) in debian/rules aborts the build, and the autopkgtest fails. Grep shows no scipy.misc.* or scipy.signal.* attribute is ever used, so the import is a latent upstream bug that only this packaging makes fatal. Fix: Ship a Debian patch that drops the unused scipy.misc (and scipy.signal) member from those import lines, or ask upstream to do so; re-run the build and autopkgtest to confirm. 2. [important] packaging Upstream defaults to clang++ but clang is not in Build-Depends Files: cxx/Makefile.mk:3, debian/control, debian/rules Evidence: CXX ?= clang++ in cxx/Makefile.mk; debian/rules runs plain dh $@ with no compiler override and control has no clang build-dependency. Problem: On a clean buildd the build chroot contains only Build-Depends plus build-essential (gcc/g++); clang++ is then command not found and every C++ target in make all fails. building the current Debian should not depend on clang being present accidentally. Fix: Either add clang to Build-Depends or set the compiler in debian/rules, e.g. pass CC=$(CC) CXX=$(CXX) (from dpkg-buildflags) to dh_auto_build. 3. [minor] packaging Upstream CXXFLAGS assignment drops Debian hardening flags Files: cxx/Makefile.mk:5 Evidence: CXXFLAGS := -Wall -O3 -g — a hard := override that clobbers whatever dpkg-buildflags exported. Problem: Because the Makefile uses :=, Debian’s default CXXFLAGS hardening (-D_FORTIFY_SOURCE=2, -fstack-protector-strong, -Wformat -Werror=format-security) never reach the compiler, which would be flagged by a build-log hardening check (blhc) and weakens the shipped C++ helpers. Fix: Pass the hardened flags through, e.g. override the build step with dh_auto_build -- CXXFLAGS="$(shell dpkg-buildflags --get CXXFLAGS) -Wall -O3 -g", preferably combined with the compiler fix above. 4. [minor] packaging Bundled CLI11 header instead of Debian’s libcli11-dev Files: cxx/3rdparty/CLI11/CLI11.hpp, cxx/Makefile.mk Evidence: Header begins CLI11: Version 2.6.2 ... generated by MakeSingleHeader.py; Debian already ships libcli11-dev 2.6.1+ds-1. Problem: POLICY §4.13: embedding a copy of third-party code that Debian already packages as a library means security and bug fixes to CLI11 do not propagate to this package; Debian’s libcli11-dev is nearly the same release. Fix: Build-Depend on libcli11-dev and include <CLI11/CLI11.hpp> (the package’s own Makefile comments already mention libcli11-dev); drop the vendored header or keep it only if the package cannot build against the system header, with justification. 5. [minor] packaging debian/watch is in the old version 4 format Files: debian/watch Evidence: version=4 Problem: Version 5 is the current format of debian/watch. Fix: Convert it to version 5; a mechanical conversion is included below. ======================================================================= Version: 5 # Compulsory line, this is a version 4 file. # GitHub hosted projects. Template: Github Dist: https://github.com/tim-janik/imagewmark ======================================================================= | View |
| awm | 0.6.0-1 | 23 days ago | 23 days ago | accepted | 23 days ago | Hi, Thanks for your changes. The LLM found a few minor issues this time, but they might be bogus - I'm still tuning the noise level so I'd appreciate knowing whether they are valid even if they are all minor. Click the link for the full review details where you can read the LLM review. Thanks! | View |