Bug#1062001: RFS: trader/7.20-1 -- simple game of interstellar trading

[Tobias Frost <tobi@debian.org>]

Control: tags -1 moreinfo

Hi John,

thanks for the fixes you've already applied, but the package needs more
work to be Debian compliant.

On Wed, Feb 28, 2024 at 11:10:58AM +1100, John Zaitseff wrote:
> Control: retitle -1 trader/7.20-2 -- simple game of interstellar trading
> Control: tags -1 - moreinfo
> 
> Dear Tobias,
> 
> Thank you very much for your feedback on my RFS for Star Traders,
> and apologies for not replying sooner -- it's been a bit of a crazy
> week around here...
> 
> You (or anyone else) can see my latest Git tree at the following
> URL; this tree reflects the changes I've made as a result of your
> email:
> 
>   https://www.zap.org.au/git-browser/trader.git/tree/?h=with-debian
>   git clone https://git.zap.org.au/git/trader.git -b with-debian
> 
> I do have a few questions and observations about your comments:
> 
> > d/control:
> > - Please remove all versions from the versioned build depends that
> >   are fulfiled already since oldstable, e.g gettext and automake.
> 
> Good catch!  This is what happens when one maintains a Debian
> package outside the distro since 2011...
> 
> Given that the Git tag for package version 7.20-1 already exists
> (and has been distributed!), I've released the new package as
> 7.20-2, with the entry for 7.20-1 removed as it has not been
> uploaded to Debian. 

Well, the first Debian revision should be -1, and I wonder why
you are repeating that mistake with -3? At least that one should have
been kept at -2. 

To avoid confusion, a "-1" with the suite "UNRELEASED" could been added
to the changelog, so that people know what's happening.

In future, don't bump the Debian revision unless it has been sponsored.

Having two sources of packages is bad too, I'd suggest to stop
distribution your own packages, at least make sure not to use the same
exact same version/revision, so that there will be clear from looking on
the version only that this was from your archives. Otherwise, I'll
envise chaos. Make sure that the version in your archives sort earlier
than the one in Debian's as this sould be the canonical source of the
Debian packages. Such a versioning scheme is also used for backports,
you can look there how it is done.

Regarding the revision -3: You write "New upstream relases", that is wrong,
or at least you are using the terminology wrong.
A new upstream release must have a new upstream version , and a new
orig.tar corresponding to the new upstream version. Yours are the same. 

To have a way out, and as you are upstream, how's about cutting
a new upstream release, e.g 7.21 and package it as 7.21-1 ?

> I'm retitling this bug report to suit.  If you
> prefer, I can close this RFS and open a new one.

You did it right, you don't file new RFS bugs if the previous one
hasn't been sponsored.

> You can now download the updated package by running:
> 
>   dget -x https://ftp.zap.org.au/pub/debian/dists/zapgroup-sid/main/source/trader_7.20-2.dsc

(Please utilize mentors, mentors has some diagnositics that helps
analyzing/reviewing the package, e.g lintian output.)

> > - Your VCS-Git link git:// is not using an encrypted link, but the
> >   site supports https://. Please use https.
> 
> Done.
> 
> > d/copyright:
> > - the directory lib/ has files which are not documented in
> >   d/copyright; (They have a different license and copyright)
> > - same for the m4 macros
> 
> Hmm.  The files (in lib and m4) that are not my own are mostly from
> the Gnulib GNU Portability Library.  Rather tedious and quite a bit
> of work, but I've updated the debian/copyright file to suit.

> I find it interesting that you're the first Debian developer to pick
> up on this: previous sponsors of the package were okay with it the
> way it was.  Perhaps you're more thorough!  If it leads to a better
> Debian package, I don't mind.
> 
> However, I note from section 2.3 of the Debian policy manual:
> 
>   Thus, the copyright information for files in the source package
>   which are only part of its build process, such as autotools files,
>   need not be included in /usr/share/doc/PACKAGE/copyright, because
>   those files do not get installed into the binary package.
> 
> My reading of that suggests there is no need to include copyright
> stanzas for files in the m4 directory.  Is this understanding
> correct?

Well, I'm used to that that they should be also documented and probably
I'm more strict than other on this; Ok, then leave them alone...

> This is what I've added to debian/copyright -- is this okay?
> 
>   Files: lib/*
>   Copyright: 1987-2024, Free Software Foundation, Inc.
>   License: GPL-3+
>   Comment:
>    Some files in this directory (from the Gnulib GNU Portability Library)
>    are licensed by the Free Software Foundation under LGPL-2.1+; other
>    files are under GPL-2+.  When combined with the remaining files from the
>    same Gnulib library, these inherit the overall GPL-3+ licence.

no, that's not ok.
you need to document the copyright *verbatim*, that is
what's written in the files, not what the effective license is.
(Policy 2.3)

> 
>   Files:
>    lib/obsolete-strings.c
>    lib/xopen-source.h
>   Copyright: 2018-2024, John Zaitseff <J.Zaitseff@zap.org.au>
>   License: GPL-3+


> > embedded code copies:
> > - gnulib is packaged for Debian, any reason why you don't use the
> >   packaged version?
> 
> The upstream release I make (in this case, trader-7.20.tar.xz)
> includes the files from Gnulib.  And in all current cases, my
> snapshot of Gnulib is *newer* than what is included in the gnulib
> Debian package (currently 20240117+stable-1 in Debian Sid, but
> 20210102~ebaa53c-1 in Debian Bullseye/oldstable -- for Star Traders
> 7.20, my snapshot is from Tue Jan 30 17:07:32 2024 +0100).
> 
> I believe my code depends on the newer Gnulibs to compile.  But even
> if it didn't, the upstream release *must* contain Gnulib as it is
> compiled on distributions and operating systems where Gnulib is not
> packaged.
> 
> Rerunning "gnulib-tool --update" as part of the build process is
> possible, I suppose, but given that my snapshots are newer, and
> there are API-related changes compared to the Debian oldstable and
> stable snapshots of Gnulib, I'm not sure that it's worth the hassle
> to even try using those Debian snapshots...

You're packaging for unstable, so oldstable and stable is not relevant.
Debian policy is to avoid embedded code copies, and as you are upstream,
you have every power to make that happen.

The Debian package is at version 20240117, that's merley 13 days older
and a stable release from upstream, so this begs this question:
What is the minimum required version you need and what are the actual
reasons why you need those? Believing that you need a newer one version
is not a strong enough argument to violate the no embedded code copies
rule.

diffing your version with the packaged ones shows only minor differences.

the gperf files should be re-generated during build.

Note: you don't need to repack, you can delete the libraries files in d/clean.
This is usually done to make sure that the files aren't used by
accident.

> > - there are m4 macros from autoconf-archive. Please check if you
> >   can use the ones from the package autoconf-archive.
> 
> Unfortunately, I can't.  Some of the M4 macros (particularly
> m4/ax_compiler_vendor.m4) contain changes that I've submitted
> upstream but have not yet been included. 

This seems not to be a correct statement.

The only change I see is in ax_cflags_warn_all.m4, and those change
seems to be not to be relevant. [1]

m4/ax_compiler_vendor.m4:
$ cmp ../autoconf-archive-20220903/m4/ax_compiler_vendor.m4 m4/ax_compiler_vendor.m4 && echo "they are identical."
they are identical.

> Besides, the
> autoconf-archive package does not seem to be part of Debian Sid
> anymore.

That's not true
$ rmadison -s sid autoconf-archive
autoconf-archive | 20220903-3    | unstable   | source, all

outdated m4 files are PITA, so pretty please, use autoconf-archive
and the m4 macros from the packaged gnulib.

> > Please add some upstream metadata:
> > https://wiki.debian.org/UpstreamMetadata
> 
> This one was new to me.  I've added a simple file, although the
> fields are copies of what is in debian/control already:
> 
>   Repository: https://git.zap.org.au/data/git/trader.git
>   Repository-Browse: https://www.zap.org.au/git-browser/trader.git/
> 
> I don't have a bug database or a separate bug submission URL
> (Upstream-Contact from debian/copyright is what I ask people to
> use), the documentation is the home page of the project, etc.  Happy
> to receive suggestions as to what else should be in this file.
> 
> > d/changelog
> > - d/changelog's purpose is to document changed to the packageing,
> >   generaly not to document upstream changes [...]
> 
> You are, of course, correct.  As a Debian *user*, I do, however,
> find a summary of the upstream changes to be quite helpful -- saves
> one from having to go to a package's homepage to find a NEWS file
> before upgrading!  So if possible, I'd like to continue to provide a
> quick summary of upstream changes.

This is not Policy compliant. Upstream changes belong to the upstream
changelog; it is generally installed into the package as reference
for the user. 
Said that, d/changelog can list *important* upstream changes, but
looking at the -3, those aren't.

You've got NEWS in your package, that's your upstream changelog, see
Policy 12.7 for more details, and it will be installed at a place
where users can expect them.

> > - lintian findings:
> > W: trader: groff-message troff:<standard input>:133: warning: macro 'mR' not defined [usr/share/man/man6/trader.6.gz:1]
> 
> Fixed as a patch to doc/trader.6.  I'll make a new upstream release
> at some point incorporating this change -- once I have other things
> (such as an updated translation) worthy of a release.
> 
> > I: trader source: public-upstream-key-not-minimal has 16 extra signature(s) for keyid 0D254111C4EE569B [debian/upstream/signing-key.asc]
> 
> Happy to solve this if I could but find instructions on how to
> remove those extra signatures *without* modifying the underlying
> key...
> 
> > I: trader source: vcs-field-uses-insecure-uri Git git://git.zap.org.au/data/git/trader.git -b with-debian
> 
> Fixed above.
> 
> > P: trader source: source-contains-autogenerated-gperf-data [lib/iconv_open-hpux.h]
> > P: trader source: source-contains-autogenerated-gperf-data [lib/iconv_open-irix.h]
> > P: trader source: source-contains-autogenerated-gperf-data [lib/iconv_open-osf.h]
> > P: trader source: source-contains-autogenerated-gperf-data [lib/iconv_open-solaris.h]
> > P: trader source: source-contains-autogenerated-gperf-data [lib/iconv_open-zos.h]
> 
> These files are required as gperf(1) is not easily available on all
> systems supported by my upstream package.

You're packaging for Debian, so you need to follow Debian policy. Gperf
is available, you need to regenerate the files at build time.

> Once again, thank you for your feedback.  I look forward to hearing
> from you or from anyone else.
> 
> Yours truly,
> 
> John Zaitseff
> 
> -- 
> John Zaitseff   ╭───╮   Email: J.Zaitseff@zap.org.au
> The ZAP Group   │ Z │   GnuPG: 0x0D254111C4EE569B
> Australia Inc.  ╰───╯   https://www.zap.org.au/~john/


[1]
~/workspace/deb/mentors/JohnZaitseff/trader/new/trader-7.20$ diff -Naur ../autoconf-archive-20220903/m4/ax_cflags_warn_all.m4 m4/ax_cflags_warn_all.m4
--- ../autoconf-archive-20220903/m4/ax_cflags_warn_all.m4	2022-09-03 00:00:00.000000000 +0200
+++ m4/ax_cflags_warn_all.m4	2024-01-30 20:09:53.000000000 +0100
@@ -51,7 +51,7 @@
 #
 #   Copyright (c) 2008 Guido U. Draheim <guidod@gmx.de>
 #   Copyright (c) 2010 Rhys Ulerich <rhys.ulerich@gmail.com>
-#   Copyright (c) 2018 John Zaitseff <J.Zaitseff@zap.org.au>
+#   Copyright (c) 2018-24 John Zaitseff <J.Zaitseff@zap.org.au>
 #
 #   This program is free software; you can redistribute it and/or modify it
 #   under the terms of the GNU General Public License as published by the
@@ -79,7 +79,7 @@
 #   modified version of the Autoconf Macro, you may extend this special
 #   exception to the GPL to apply to your modified version as well.
 
-#serial 25
+#serial 26
 
 AC_DEFUN([AX_FLAGS_WARN_ALL], [
     AX_REQUIRE_DEFINED([AX_PREPEND_FLAG])dnl
@@ -102,6 +102,7 @@
 	    [fujitsu],		[],
 	    [sdcc],		[],
 	    [sx],		[VAR="-pvctl[,]fullmsg"],
+	    [nvhpc],		[VAR="-Wall"],
 	    [portland],		[],
 	    [gnu],		[VAR="-Wall"],
 	    [sun],		[VAR="-v"],

>