Hello,

> On 15 Apr 2021, at 13:31, Valters Jansons <valter.jansons(a)gmail.com> wrote:
> 
> On Thu, Apr 15, 2021 at 2:06 PM Michael Tremer
> <michael.tremer(a)ipfire.org> wrote:
>>> - d/.gitignore: Ignore all temporary files and subdirectories such as
>>> debian/location-importer/ and debian/location-importer.debhelper.log
>>> with the exception of debian/source/ and potential debian/patches/
>>> which may be used for Quilt, considering the source format is set
>>> to '3.0 (quilt)'.
>> 
>> Okay. If this would have been an individual patch I would have merged this straight away.
> 
> Didn't feel like splitting things out from the debian/ cleanup here.

I would strongly recommend doing this, because it makes reviewing things a lot easier. Also parts could be reverted later if there is a problem in the patch.

This way, I struggled to find the bits that belong together and only the well-structured commit message has helped. If that wasn’t there I probably would have rejected it straight away, because generally it is too dangerous to accept patches that cannot be understood (in reasonable time).

I wrote more in this here: https://wiki.ipfire.org/devel/submit-patches (see "Separate your changes”).

> 
>>> - d/clean: Remove m4/intltool.m4 and po/Makefile.in.in autogenerated
>>> files prior to building/in-between builds. Without removal of these
>>> autogenerated files, build tooling complains about unexpected changes
>>> to the source tree.
>> 
>> Don't the scripts call “make distclean” or something similar?
> 
> Yes, `make -j$(nproc) distclean` is invoked. Those two (symlink) files
> are left dangling however after make finishes.

Maybe we should add them to CLEANFILES?

> Keep in mind those two files are currently coming in from `intltoolize
> --force --automake`, either from the override_dh_auto_configure target
> or the autogen.sh script.
> 
>>> - d/copyright: Update format link to use HTTPS instead of HTTP
>>> (lintian: insecure-copyright-format-uri).
>>> 
>>> - d/libloc1.symbols: Added symbols export file
>>> (lintian: no-symbols-control-file). For generation:
>>> $ debuild -uc -us # to easily build everything to debian/tmp/
>>> $ dpkg-gensymbols -plibloc1 -Odebian/libloc1.symbols
>>> $ sed -i -E -e 's/( [0-9\.]+)-.+$/\1/' debian/*.symbols
>> 
>> Hmm, this feels very ugly to me.
>> 
>> We already have a file that has all exported symbols. What does Debian use this for?
> 
> To quote documentation: dpkg can use symbols files in order to
> generate more accurate library dependencies for applications, based on
> the symbols from the library that are actually used by the
> application.
> 
> In essence: dpkg-shlibdeps can for example check what version a symbol
> was introduced at, and check for compatibility that way. This new file
> is like a changelog for the symbol information.

Yes, I know what it does. I would just like to avoid generating a file like this manually every time something is being updated. It is easily forgotten or goes wrong.

Can we script this?

> 
>>> - d/location-python.examples: Add the examples/ to documentation
>>> (lintian: package-does-not-install-examples).
>> 
>> I am not if it is a good idea to install the example key just in case that people start using it.
>> 
>> We have seen this in various places where the “development/testing” key suddenly “slipped” into production.
> 
> Will remove the examples change then.
> 
>>> - d/watch: Add uscan configuration, as expected for '3.0 (quilt)' format
>>> (lintian: debian-watch-file-is-missing).
>> 
>> What does this do?
> 
> Allows automatic detection of an updated upstream (source.ipfire.org)
> version having been packaged.
> 
> You can see the verbose behind-the-scenes by running:
> $ uscan --no-download --verbose --debug

Understood.

>>> - `location-importer` not having a manpage.
>> 
>> Good question if it is worth writing one. Is anyone up for this?
> 
> I will leave this one hanging for now from my side.
> Would of course be very good if someone were to pick it up.
> 
>>> - An out-dated Standards-Version.
>> 
>> ?
> 
> d/control has Standards-Version which signals compliance with the
> Policy Manual. This note just means the listed changes should be
> evaluated, and the Standards-Version updated accordingly.
> 
> I wanted this patch now to provide individual chunks that deal with
> the particular listed Lintian issues. I will try to pick
> Standards-Version up in an individual separate commit afterwards, so
> that it is clear that any changes taking place there are a part of the
> upgrade.
> 
> https://www.debian.org/doc/debian-policy/upgrading-checklist.html
> 
>>> - An old debhelper compatibility level.
>> 
>> Can we not change this?
> 
> Yes, it's perfectly doable. Buster has debhelper v12 available, and I
> will switch to debhelper-compat along the way.
> 
> There are changes associated with this upgrade -- I am aware of the
> systemd change affecting this repository, and there could be others.
> As a result, the reasoning for not doing it here is the same as for
> Standards-Version above.
> 
>>> - Lack of an autopkgtest testsuite.
>> 
>> ?
> 
> Debian has autopkgtest for Continuous Integration testing. Very
> shortly, you can specify d/tests/control with a listing of test cases,
> and then provide the particular commands along with their metadata
> (dependencies, restrictions, etc) in d/tests/ files.
> 
> During build, the Makefile's testcases are run, however autopkgtest
> additionally allows re-testing a package at a later point when the
> dependencies have changed.

Interesting. We already have a test suite and I would like to avoid duplicating more code. Loading the library probably won’t be enough.

> Another one of those things that is just nice to have configured, but
> is not 'critical' in of itself.

Agreed.

-Michael

> 
> --Valters