tirex review

[Sebastiaan Couwenberg]

On 9/3/21 8:40 PM, Felix Delattre wrote:
> On 9/3/21 8:54 AM, Sebastiaan Couwenberg wrote:
>>
>>  * debian/copyright:
>>
> 
> Done, what has been mentioned on this file.

public-domain is a special shortname, see:

 https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#license-short-name
 https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#public-domain

License paragraphs are not double quoted, and only indented with a
single space.

>>  * debian/etc/tirex/renderer/mapnik.conf
>>
>>    Might want to make this a template and set plugindir based on the
>>    version of mapnik the package it built with.

Why keep the mapnik version hardcoded? This requires source changes when
the major or minor version of mapnik is incremented (e.g. 3.2 or 4.0).
Using the version detected a build time makes rebuilding the package
with a binNMU sufficient.

>>  *
>> debian/patches/0002-Disconnect-configuration-overlap-between-build-and-d.patch
>>
>>    Could be marked as Forwarded: not-needed
>>

The Origin and Bug headers were good and needn't be removed.

I did not register that the upstream issue also changed the packaging
there. The patch headers were better as they were.

Patches that are specific to the Debian packaging (e.g. using a Debian
specific path) generally don't need to be forwarded upstream. That's not
really the case here.

>>
>>  * debian/tirex.install
>>
>>    Use usr/lib/systemd instead of lib/systemd.
> 
> When changing this I get `E: tirex: systemd-service-file-outside-lib` (related to the point below you mentioned).
> 
> Can you confirm that this is still the way to go?

Yes, usr/lib/systemd is the new path. See the discussion on -devel:

 https://lists.debian.org/debian-devel/2021/08/msg00256.html

>>
>>  * lintian:
>>
>>    systemd-service-file-outside-lib
>>    Can be ignored, check not updated yet for new path.
> 
> See question above about a second confirmation that this error is to good to be ignored.

Yes, this lintian issue can be ignored an no override should be added.
It will go away once lintian is updated.

>>
>>    debug-file-with-no-debug-symbols
>>    Not sure why mapnik backend has not debug symbols.
> 
> I was able to add a patch for this: https://salsa.debian.org/debian-gis-team/tirex/-/blob/master/debian/patches/0003-Add-debug-information-to-tirex-backend-manager.patch
> It doesn't seem to be possible to give the Makefile any variables. I'm not sure if this is the best way on how this should be done. Some input would be helpful.

Makefiles use environment variables by default.

One problem with backend-mapnik/Makefile is that it sets CXXFLAGS &
LDFLAGS instead of appending to it. This overwrites the flags set in the
environment by dpkg-buildflags.

See also: https://www.gnu.org/software/make/manual/make.html#Flavors

dpkg-buildflags sets a bunch of *FLAGS so they don't need to be set in
the Makefile, this includes -g:

 $ DEB_BUILD_MAINT_OPTIONS=hardening=+all dpkg-buildflags
 CFLAGS=-g -O2 \
        -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
        -fstack-protector-strong -Wformat -Werror=format-security
 CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2
 CXXFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
          -fstack-protector-strong -Wformat -Werror=format-security
 DFLAGS=-frelease
 FCFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
         -fstack-protector-strong
 FFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
        -fstack-protector-strong
 GCJFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
          -fstack-protector-strong
 LDFLAGS=-Wl,-z,relro -Wl,-z,now
 OBJCFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
           -fstack-protector-strong -Wformat -Werror=format-security
 OBJCXXFLAGS=-g -O2 -ffile-prefix-map=/home/bas/git/pkg-grass/tirex=. \
             -fstack-protector-strong -Wformat -Werror=format-security

>>
>>    package-supports-alternative-init-but-no-init.d-script
>>    Adding an init script is optional if systemd service is used,
>>    add an init script or lintian override.
>>
> Added lintian override.

Lintian overrides should have a comment explaining why.

Kind Regards,

Bas

-- 
 GPG Key ID: 4096R/6750F10AE88D4AF1
Fingerprint: 8182 DE41 7056 408D 6146  50D1 6750 F10A E88D 4AF1