[Git][debian-gis-team/libapache2-mod-tile][master] 3 commits: added systemd.service

[Sebastiaan Couwenberg]

On 8/26/20 10:55 PM, Felix Delattre wrote:
> Hi Bas,
> 
> Thanks for the check-in! Please find my answers in between lines:
> 
> On 8/26/20 4:19 PM, Sebastiaan Couwenberg wrote:
>> On 8/26/20 5:39 PM, Felix Delattre wrote:
>>> Felix Delattre pushed to branch master at Debian GIS Project / libapache2-mod-tile
>>> Commits:
>>> 24720515 by Felix Delattre at 2020-08-25T20:24:18+00:00
>>> added systemd.service
>>>
>>> - - - - -
>>> 2757a3eb by Felix Delattre at 2020-08-26T15:18:33+00:00
>>> moved helper scripts to a suitable location
>>>
>>> - - - - -
>>> 77d80483 by Felix Delattre at 2020-08-26T15:27:34+00:00
>>> prepared libapache2-mod-tile to work equally with renderd and tirex
>>>
>>> - - - - -
>>
>> Commit messages are sentences, they start with a capital and end with a
>> period.
> 
> 
> Is this a suggestion? Or a rule of the team?

It's neither, it's a requirement for good commit messages.

>>> =====================================
>>> debian/control
>>> =====================================
>>> @@ -1,7 +1,7 @@
>>>  Source: libapache2-mod-tile
>>>  Maintainer: Debian GIS Project <pkg-grass-devel at lists.alioth.debian.org>
>>>  Uploaders: Felix Delattre <debian at xama.nu>
>>> -Section: science
>>> +Section: web
>>
>> Don't change the section of the source package.
>>
>>>  Priority: optional
>>>  Build-Depends: debhelper (>= 10~),
>>>                 dh-apache2,
>>> @@ -18,11 +18,12 @@ Homepage: https://wiki.openstreetmap.org/wiki/Mod_tile
>>>  
>>>  Package: libapache2-mod-tile
>>>  Architecture: any
>>> -Section: httpd
>>> -Depends: libjs-openlayers,
>>> -         renderd | tirex-master,
>>> -         ${shlibs:Depends},
>>> +Section: web
>>> +Depends: ${shlibs:Depends},
>>
>> Especially when you duplicate that same section for a binary package.
> 
> I assumed they should be the same. And I can't really see why this should fit into "science".
> Can you give me some more context please, why do they have to be different? And why wouldn't it be better to change it to "web"?

Why are you changing the source package Section in the first place?

It's not making the package function better.

GIS provides tools for the geography science, hence this choice for our
source packages as documented in the team policy:

 https://debian-gis-team.pages.debian.net/policy/policy.html#debian-control

>>> =====================================
>>> debian/libapache2-mod-tile.apache2
>>> =====================================
>>> @@ -1,3 +1,2 @@
>>>  mod src/.libs/mod_tile.so
>>>  mod debian/tile.load
>>> -conf debian/libapache2-mod-tile.conf
>>
>> Why do you remove the config?
> 
> It is not removed. It is renamed and handled in the `renderd` package. Thus this prepares `libapache2-mod-tile` for opening to be used by either `renderd` or `tirex`.

That's not clear for your commit messages.

>>> =====================================
>>> debian/libapache2-mod-tile.install
>>> =====================================
>>> @@ -1,5 +1,3 @@
>>>  debian/tile.load                  etc/apache2/mods-available
>>>  slippymap.html                    usr/share/libapache2-mod-tile
>>>  munin/mod_tile*                   usr/share/munin/plugins
>>> -openstreetmap-tiles-update-expire usr/bin
>>> -osmosis-db_replag                 usr/bin
>>
>> Why aren't these binaries installed?
> 
> Have you seen them? :) I moved them to via package.examples into `/usr/share/doc/libapache2-mod-tile/examples`
> I don't think they are even close to be responsibly usable in `/usr/bin`.

As long as they are not needed for common usage scenarios that's fine.

>>> =====================================
>>> debian/libapache2-mod-tile.links deleted
>>> =====================================
>>> @@ -1 +0,0 @@
>>> -usr/share/javascript/openlayers usr/share/libapache2-mod-tile/openlayers
>>
>> Why is this removed?
> 
> The example map should not be created in `libapache2-mod-tile`. It is too specific and is going to be factored into its own package.

Why not?

How will installing libapache2-mod-tile have a usable default configuration?

>>> =====================================
>>> debian/libapache2-mod-tile.conf → debian/libapache2-renderd.conf
>>> =====================================
>>> @@ -1,6 +1,6 @@
>>> -Alias /mod_tile /usr/share/libapache2-mod-tile
>>> +Alias /mod_tile /var/lib/mod_tile
>>>  
>>> -<Directory /usr/share/libapache2-mod-tile>
>>> +<Directory  /var/lib/mod_tile>
>>>      Options Indexes FollowSymLinks MultiViews
>>>      AllowOverride None
>>
>> Why this change?
>>
>> /usr/share/<package> is the appropriate place for webapps.
> 
> The whole package expects currently things to be in `/var/lib/mod_tile`. This adjusts it for the sake of consistency.
> 
> I'm happy to move (all of) it to another path. But I don't think `/usr/share/<package>` is appropriate. As this is not any web app living there! But rather the generated tiles used for serving. From my (naive) understanding it should go under `/var` and instead of `/var/lib/<package>` I would preferably choose `/var/cache/<package>`. What do you think?

/usr/share is appropriate for the webapp, i.e. the openlayers bit that's
served by apache.

/var/cache is indeed appropriate for generated tiles, that's what
mapcache uses too for example.

>> https://wiki.debian.org/Apache/PackagingFor24
>>
>> Please motivate your changes better.
> 
> Please excuse. There are a lot of moving pieces in order to get this to work. The original files are not in a good shape and many things have to change. Explaining everything through mail would be tedious. Please look at the commits and their messages. I usually bundle everything into logical steps. And I'm always happy to answer questions and react to your comments.

When the commits and their messages don't explain the changes
sufficiently you get questions like these.

Since you mentioned that you have no real experience in packaging we
cannot assume that the changes you're making are correct.

Kind Regards,

Bas

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