debian: Mitigate bulk of Lintian issues

  - 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)'.

- 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.

- d/control: Move libloc1 to 'libs' section
  (lintian: wrong-section-according-to-package-name).
- d/control: Set 'Multi-Arch: foreign' hint for location-importer and
  location-python, due to py3compile via dh-python
  (lintian: multi-arch-same-package-calls-pycompile).

- 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

- d/location-python.examples: Add the examples/ to documentation
  (lintian: package-does-not-install-examples).

- d/rules: Remove _location.la from location-python package
  (lintian: unknown-file-in-python-module-directory,
  incorrect-libdir-in-la-file, non-empty-dependency_libs-in-la-file).
  Considering the dependency library is not being installed, the la
  file does not appear too useful. Additionally, dh-python moves the
  site-packages files dist-packages resulting in a broken libdir,
  and unexpected .la files being added to Python root.
- d/rules: Remove location/{database,importer}.py from location-python
  package. These files get installed to location-importer package
  (lintian: binaries-have-file-conflict).

- d/watch: Add uscan configuration, as expected for '3.0 (quilt)' format
  (lintian: debian-watch-file-is-missing).

- src/systemd/location-update.service.in: Add a generic Documentation=
  linking to the manual page of location, for `systemctl help <name>`
  (lintian: systemd-service-file-missing-documentation-key).

Following this, the only complaints from Lintian are about:
- `location-importer` not having a manpage.
- Short package Descriptions, not explaining what they do in detail.
- An out-dated Standards-Version.
- An old debhelper compatibility level.
- Lack of an autopkgtest testsuite.

Signed-off-by: Valters Jansons <valter.jansons@gmail.com>
---
 debian/.gitignore                      |   8 +-
 debian/clean                           |   2 +
 debian/control                         |   5 +-
 debian/copyright                       |   2 +-
 debian/libloc1.symbols                 | 134 +++++++++++++++++++++++++
 debian/location-python.examples        |   3 +
 debian/rules                           |   8 ++
 debian/watch                           |   3 +
 src/systemd/location-update.service.in |   1 +
 9 files changed, 159 insertions(+), 7 deletions(-)
 create mode 100644 debian/clean
 create mode 100644 debian/libloc1.symbols
 create mode 100644 debian/location-python.examples
 create mode 100644 debian/watch
  

Hello,

> On 15 Apr 2021, at 11:30, Valters Jansons <valter.jansons@gmail.com> 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.

> - 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?

> - d/control: Move libloc1 to 'libs' section
>  (lintian: wrong-section-according-to-package-name).

Okay.

> - d/control: Set 'Multi-Arch: foreign' hint for location-importer and
>  location-python, due to py3compile via dh-python
>  (lintian: multi-arch-same-package-calls-pycompile).

Okay.

> - 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?

> - 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.

> - d/rules: Remove _location.la from location-python package
>  (lintian: unknown-file-in-python-module-directory,
>  incorrect-libdir-in-la-file, non-empty-dependency_libs-in-la-file).
>  Considering the dependency library is not being installed, the la
>  file does not appear too useful. Additionally, dh-python moves the
>  site-packages files dist-packages resulting in a broken libdir,
>  and unexpected .la files being added to Python root.

I believe la files never have any use :)

> - d/rules: Remove location/{database,importer}.py from location-python
>  package. These files get installed to location-importer package
>  (lintian: binaries-have-file-conflict).

Okay.

> - d/watch: Add uscan configuration, as expected for '3.0 (quilt)' format
>  (lintian: debian-watch-file-is-missing).

What does this do?

> - src/systemd/location-update.service.in: Add a generic Documentation=
>  linking to the manual page of location, for `systemctl help <name>`
>  (lintian: systemd-service-file-missing-documentation-key).
> 
> Following this, the only complaints from Lintian are about:
> - `location-importer` not having a manpage.

Good question if it is worth writing one. Is anyone up for this?

> - Short package Descriptions, not explaining what they do in detail.
> - An out-dated Standards-Version.

?

> - An old debhelper compatibility level.

Can we not change this?

> - Lack of an autopkgtest testsuite.

?

-Michael

> 
> Signed-off-by: Valters Jansons <valter.jansons@gmail.com>
> ---
> debian/.gitignore                      |   8 +-
> debian/clean                           |   2 +
> debian/control                         |   5 +-
> debian/copyright                       |   2 +-
> debian/libloc1.symbols                 | 134 +++++++++++++++++++++++++
> debian/location-python.examples        |   3 +
> debian/rules                           |   8 ++
> debian/watch                           |   3 +
> src/systemd/location-update.service.in |   1 +
> 9 files changed, 159 insertions(+), 7 deletions(-)
> create mode 100644 debian/clean
> create mode 100644 debian/libloc1.symbols
> create mode 100644 debian/location-python.examples
> create mode 100644 debian/watch
> 
> diff --git a/debian/.gitignore b/debian/.gitignore
> index 0faf920..4a7eb9a 100644
> --- a/debian/.gitignore
> +++ b/debian/.gitignore
> @@ -2,9 +2,9 @@
> /autoreconf.*
> /debhelper-build-stamp
> /files
> -/libloc/
> -/libloc-dev/
> -/libloc-perl/
> -/tmp
> +/*/
> *.debhelper
> +*.log
> *.substvars
> +!/patches/
> +!/source/
> diff --git a/debian/clean b/debian/clean
> new file mode 100644
> index 0000000..54a6877
> --- /dev/null
> +++ b/debian/clean
> @@ -0,0 +1,2 @@
> +m4/intltool.m4
> +po/Makefile.in.in
> diff --git a/debian/control b/debian/control
> index dc40927..ec27f92 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -23,6 +23,7 @@ Vcs-Browser: https://git.ipfire.org/pub/git/location/libloc.git
> 
> Package: libloc1
> Architecture: any
> +Section: libs
> Pre-Depends:
>  ${misc:Pre-Depends}
> Depends:
> @@ -67,7 +68,7 @@ Depends:
>  location-python (= ${binary:Version}),
>  ${misc:Depends},
>  ${python3:Depends}
> -Multi-Arch: same
> +Multi-Arch: foreign
> Description: Tools to author location databases
>  This package contains tools that are required to build location databases
> 
> @@ -80,6 +81,6 @@ Depends:
>  ${misc:Depends},
>  ${python3:Depends},
>  ${shlibs:Depends}
> -Multi-Arch: same
> +Multi-Arch: foreign
> Description: Python modules for libloc
>  This package contains Python bindings for libloc
> diff --git a/debian/copyright b/debian/copyright
> index 636af48..3bd7654 100644
> --- a/debian/copyright
> +++ b/debian/copyright
> @@ -1,4 +1,4 @@
> -Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> Upstream-Name: libloc
> Upstream-Contact: Michael Tremer <michael.tremer@ipfire.org>
> Source: https://location.ipfire.org/download
> diff --git a/debian/libloc1.symbols b/debian/libloc1.symbols
> new file mode 100644
> index 0000000..74b70b5
> --- /dev/null
> +++ b/debian/libloc1.symbols
> @@ -0,0 +1,134 @@
> +libloc.so.1 libloc1 #MINVER#
> +* Build-Depends-Package: libloc-dev
> + LIBLOC_1@LIBLOC_1 0.9.4
> + LIBLOC_PRIVATE@LIBLOC_PRIVATE 0.9.4
> + loc_as_cmp@LIBLOC_1 0.9.4
> + loc_as_get_name@LIBLOC_1 0.9.4
> + loc_as_get_number@LIBLOC_1 0.9.4
> + loc_as_list_append@LIBLOC_1 0.9.5
> + loc_as_list_clear@LIBLOC_1 0.9.5
> + loc_as_list_contains@LIBLOC_1 0.9.5
> + loc_as_list_contains_number@LIBLOC_1 0.9.5
> + loc_as_list_empty@LIBLOC_1 0.9.5
> + loc_as_list_get@LIBLOC_1 0.9.5
> + loc_as_list_new@LIBLOC_1 0.9.5
> + loc_as_list_ref@LIBLOC_1 0.9.5
> + loc_as_list_size@LIBLOC_1 0.9.5
> + loc_as_list_unref@LIBLOC_1 0.9.5
> + loc_as_new@LIBLOC_1 0.9.4
> + loc_as_ref@LIBLOC_1 0.9.4
> + loc_as_set_name@LIBLOC_1 0.9.4
> + loc_as_unref@LIBLOC_1 0.9.4
> + loc_country_cmp@LIBLOC_1 0.9.4
> + loc_country_code_is_valid@LIBLOC_1 0.9.4
> + loc_country_get_code@LIBLOC_1 0.9.4
> + loc_country_get_continent_code@LIBLOC_1 0.9.4
> + loc_country_get_name@LIBLOC_1 0.9.4
> + loc_country_list_append@LIBLOC_1 0.9.5
> + loc_country_list_clear@LIBLOC_1 0.9.5
> + loc_country_list_contains@LIBLOC_1 0.9.5
> + loc_country_list_contains_code@LIBLOC_1 0.9.5
> + loc_country_list_empty@LIBLOC_1 0.9.5
> + loc_country_list_get@LIBLOC_1 0.9.5
> + loc_country_list_new@LIBLOC_1 0.9.5
> + loc_country_list_ref@LIBLOC_1 0.9.5
> + loc_country_list_size@LIBLOC_1 0.9.5
> + loc_country_list_unref@LIBLOC_1 0.9.5
> + loc_country_new@LIBLOC_1 0.9.4
> + loc_country_ref@LIBLOC_1 0.9.4
> + loc_country_set_continent_code@LIBLOC_1 0.9.4
> + loc_country_set_name@LIBLOC_1 0.9.4
> + loc_country_unref@LIBLOC_1 0.9.4
> + loc_database_count_as@LIBLOC_1 0.9.4
> + loc_database_created_at@LIBLOC_1 0.9.4
> + loc_database_enumerator_get_asns@LIBLOC_1 0.9.5
> + loc_database_enumerator_get_countries@LIBLOC_1 0.9.5
> + loc_database_enumerator_new@LIBLOC_1 0.9.4
> + loc_database_enumerator_next_as@LIBLOC_1 0.9.4
> + loc_database_enumerator_next_country@LIBLOC_1 0.9.4
> + loc_database_enumerator_next_network@LIBLOC_1 0.9.4
> + loc_database_enumerator_ref@LIBLOC_1 0.9.4
> + loc_database_enumerator_set_asns@LIBLOC_1 0.9.5
> + loc_database_enumerator_set_countries@LIBLOC_1 0.9.5
> + loc_database_enumerator_set_family@LIBLOC_1 0.9.4
> + loc_database_enumerator_set_flag@LIBLOC_1 0.9.4
> + loc_database_enumerator_set_string@LIBLOC_1 0.9.4
> + loc_database_enumerator_unref@LIBLOC_1 0.9.4
> + loc_database_get_as@LIBLOC_1 0.9.4
> + loc_database_get_country@LIBLOC_1 0.9.4
> + loc_database_get_description@LIBLOC_1 0.9.4
> + loc_database_get_license@LIBLOC_1 0.9.4
> + loc_database_get_vendor@LIBLOC_1 0.9.4
> + loc_database_lookup@LIBLOC_1 0.9.4
> + loc_database_lookup_from_string@LIBLOC_1 0.9.4
> + loc_database_new@LIBLOC_1 0.9.4
> + loc_database_ref@LIBLOC_1 0.9.4
> + loc_database_unref@LIBLOC_1 0.9.4
> + loc_database_verify@LIBLOC_1 0.9.4
> + loc_discover_latest_version@LIBLOC_1 0.9.4
> + loc_get_log_priority@LIBLOC_1 0.9.4
> + loc_network_address_family@LIBLOC_1 0.9.4
> + loc_network_cmp@LIBLOC_1 0.9.5
> + loc_network_exclude@LIBLOC_1 0.9.5
> + loc_network_exclude_list@LIBLOC_1 0.9.5
> + loc_network_format_first_address@LIBLOC_1 0.9.4
> + loc_network_format_last_address@LIBLOC_1 0.9.4
> + loc_network_get_asn@LIBLOC_1 0.9.4
> + loc_network_get_country_code@LIBLOC_1 0.9.4
> + loc_network_get_first_address@LIBLOC_1 0.9.5
> + loc_network_get_last_address@LIBLOC_1 0.9.5
> + loc_network_has_flag@LIBLOC_1 0.9.4
> + loc_network_is_subnet@LIBLOC_1 0.9.5
> + loc_network_list_clear@LIBLOC_1 0.9.5
> + loc_network_list_contains@LIBLOC_1 0.9.5
> + loc_network_list_dump@LIBLOC_1 0.9.5
> + loc_network_list_empty@LIBLOC_1 0.9.5
> + loc_network_list_get@LIBLOC_1 0.9.5
> + loc_network_list_merge@LIBLOC_1 0.9.5
> + loc_network_list_new@LIBLOC_1 0.9.5
> + loc_network_list_pop@LIBLOC_1 0.9.5
> + loc_network_list_pop_first@LIBLOC_1 0.9.5
> + loc_network_list_push@LIBLOC_1 0.9.5
> + loc_network_list_ref@LIBLOC_1 0.9.5
> + loc_network_list_size@LIBLOC_1 0.9.5
> + loc_network_list_unref@LIBLOC_1 0.9.5
> + loc_network_match_address@LIBLOC_1 0.9.5
> + loc_network_match_asn@LIBLOC_1 0.9.4
> + loc_network_match_country_code@LIBLOC_1 0.9.4
> + loc_network_match_flag@LIBLOC_1 0.9.4
> + loc_network_new@LIBLOC_1 0.9.4
> + loc_network_new_from_string@LIBLOC_1 0.9.4
> + loc_network_overlaps@LIBLOC_1 0.9.5
> + loc_network_prefix@LIBLOC_1 0.9.5
> + loc_network_ref@LIBLOC_1 0.9.4
> + loc_network_set_asn@LIBLOC_1 0.9.4
> + loc_network_set_country_code@LIBLOC_1 0.9.4
> + loc_network_set_flag@LIBLOC_1 0.9.4
> + loc_network_str@LIBLOC_1 0.9.4
> + loc_network_subnets@LIBLOC_1 0.9.5
> + loc_network_unref@LIBLOC_1 0.9.4
> + loc_new@LIBLOC_1 0.9.4
> + loc_ref@LIBLOC_1 0.9.4
> + loc_set_log_fn@LIBLOC_1 0.9.4
> + loc_set_log_priority@LIBLOC_1 0.9.4
> + loc_stringpool_add@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_dump@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_get@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_get_size@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_new@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_ref@LIBLOC_PRIVATE 0.9.4
> + loc_stringpool_unref@LIBLOC_PRIVATE 0.9.4
> + loc_unref@LIBLOC_1 0.9.4
> + loc_writer_add_as@LIBLOC_1 0.9.4
> + loc_writer_add_country@LIBLOC_1 0.9.4
> + loc_writer_add_network@LIBLOC_1 0.9.4
> + loc_writer_get_description@LIBLOC_1 0.9.4
> + loc_writer_get_license@LIBLOC_1 0.9.4
> + loc_writer_get_vendor@LIBLOC_1 0.9.4
> + loc_writer_new@LIBLOC_1 0.9.4
> + loc_writer_ref@LIBLOC_1 0.9.4
> + loc_writer_set_description@LIBLOC_1 0.9.4
> + loc_writer_set_license@LIBLOC_1 0.9.4
> + loc_writer_set_vendor@LIBLOC_1 0.9.4
> + loc_writer_unref@LIBLOC_1 0.9.4
> + loc_writer_write@LIBLOC_1 0.9.4
> diff --git a/debian/location-python.examples b/debian/location-python.examples
> new file mode 100644
> index 0000000..489d2cb
> --- /dev/null
> +++ b/debian/location-python.examples
> @@ -0,0 +1,3 @@
> +examples/private-key.pem
> +examples/public-key.pem
> +examples/python/
> diff --git a/debian/rules b/debian/rules
> index 8893b7b..05b88fd 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -18,3 +18,11 @@ override_dh_perl:
> 
> override_dh_systemd_enable:
> 	dh_systemd_enable location-update.timer
> +
> +override_dh_install:
> +	dh_install
> +	# lintian: unknown-file-in-python-module-directory
> +	rm debian/location-python/usr/lib/python3*/site-packages/_location.la
> +	# linitan: binaries-have-file-conflict (d/location-importer.install)
> +	rm debian/location-python/usr/lib/python3*/site-packages/location/database.py
> +	rm debian/location-python/usr/lib/python3*/site-packages/location/importer.py
> diff --git a/debian/watch b/debian/watch
> new file mode 100644
> index 0000000..19ace6d
> --- /dev/null
> +++ b/debian/watch
> @@ -0,0 +1,3 @@
> +version=4
> +https://source.ipfire.org/releases/libloc/ \
> +    @PACKAGE@@ANY_VERSION@@ARCHIVE_EXT@ debian uupdate
> diff --git a/src/systemd/location-update.service.in b/src/systemd/location-update.service.in
> index daae2c5..1c8e116 100644
> --- a/src/systemd/location-update.service.in
> +++ b/src/systemd/location-update.service.in
> @@ -1,5 +1,6 @@
> [Unit]
> Description=Automatic Location Database Updater
> +Documentation=man:location(8) https://man-pages.ipfire.org/libloc/location.html
> Requires=network.target
> 
> [Service]
> -- 
> 2.31.1
>
  

On Thu, Apr 15, 2021 at 2:06 PM Michael Tremer
<michael.tremer@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.

> > - 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.

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.

> > - 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

> > - `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.

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

--Valters
  

Hello,

> On 15 Apr 2021, at 13:31, Valters Jansons <valter.jansons@gmail.com> wrote:
> 
> On Thu, Apr 15, 2021 at 2:06 PM Michael Tremer
> <michael.tremer@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
  

On Fri, Apr 16, 2021 at 1:31 PM Michael Tremer
<michael.tremer@ipfire.org> wrote:
> > 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”).

Understood. I initially wrote the message explaining the individual
parts and intentionally denoting which files contain particular
changes. I was also not sure if a slew of few-line changes in
separated patches would really be preferred.

I will split and send in the individual fixes as separate patches,
based on the concept of 'smallest logical change' to allow
bisecting/provide a cleaner trail, and keep it in mind for future.

> >>> - 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?

Yes - I don't see why not, if so preferred. Will poke around with this one then.

> >>> - 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?

Yes - a short script can easily be done to facilitate the updating
process. Do you want a separate script in debian/ to do this, or to be
included as part of some already existing tooling?

> >>> - 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.

It could probably be set up to run the same test suite direct
invocation (not via Makefile). Would require a duplicated reference to
the test suite itself, but avoid duplicating the test logic itself.
That feels like the best approach to this.

--Valters
  

Hello,

> On 16 Apr 2021, at 12:33, Valters Jansons <valter.jansons@gmail.com> wrote:
> 
> On Fri, Apr 16, 2021 at 1:31 PM Michael Tremer
> <michael.tremer@ipfire.org> wrote:
>>> 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”).
> 
> Understood. I initially wrote the message explaining the individual
> parts and intentionally denoting which files contain particular
> changes. I was also not sure if a slew of few-line changes in
> separated patches would really be preferred.
> 
> I will split and send in the individual fixes as separate patches,
> based on the concept of 'smallest logical change' to allow
> bisecting/provide a cleaner trail, and keep it in mind for future.
> 
>>>>> - 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?
> 
> Yes - I don't see why not, if so preferred. Will poke around with this one then.
> 
>>>>> - 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?
> 
> Yes - a short script can easily be done to facilitate the updating
> process. Do you want a separate script in debian/ to do this, or to be
> included as part of some already existing tooling?

I do not have any existing tooling around this apart from what is in debian/. There is a build script that I use to build the package for various versions of Debian and for various architectures that I thought our users would be using. I then upload them to our server with dput where the packages are being signed and uploaded to our mirror servers.

>>>>> - 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.
> 
> It could probably be set up to run the same test suite direct
> invocation (not via Makefile). Would require a duplicated reference to
> the test suite itself, but avoid duplicating the test logic itself.
> That feels like the best approach to this.

We should not confuse the two test suites.

The Debian test suite is useful for the packager, but not for people who develop the application or compile it manually on their own distribution. For that, “make check” is supposed to find any problems with that particular build or find any regressions being introduced into the library.

-Michael

> 
> --Valters