Hi Paul thanks for the review. Let’s see what I can do. > These need to be fixed before I upload this: > >> This NEW upload closes RFP #818245. > > Any particular reason you haven't retitled it to ITP and set yourself > as the owner? Done. > Since this is a fork of part of the Python stdlib, I would expect > there would be some more copyright holders listed in the script itself > and in debian/copyright. This needs to be clarified upstream. I have checked with upstream (https://github.com/jeffkaufman/icdiff/issues/68) and they have released 1.7.6 with the extra copyright added. > tests/b/1 and tests/input-3.txt looks like duplicate of a file copied > from another project. At minimum the source location, copyright and > license info for it looks like it needs to be clarified. I see. Unfortunately the copy of this file that upstream ships does not include a license; however a newer version does (https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt). I have excluded the old version from 1.7.6 using d/copyright’s Files-Excluded, and then patched the code to use the newer version I distribute myself as part of the package. That is, until upstream accepts my PR that updates these test files directly: https://github.com/jeffkaufman/icdiff/pull/70 > Please > also let the security team know that it is a fork, they track forks > through the embedded code copies stuff. > https://wiki.debian.org/EmbeddedCodeCopies Sure, I guess this is something to be done after the package entered unstable? > These would be nice to fix: > > There isn't any point in adding DEP-8 tests because test.sh only runs > icdiff from the current directory but DEP-8 is about testing the > installed package. You might be able to fix this by sending upstream a > patch for the script though. Done: https://github.com/jeffkaufman/icdiff/pull/71 I have changed the script to allow controlling the executable to call through an environment variable, which I set in my autopkgtest. > Please add more DEP-3 headers to the patch, including at minimum the > Forwarded header. Done. Since the version problem has been sorted out in the meantime, I could drop the patch from the previous version. I have added Forwarded tags for all new patches though. > I can't find the syntax right now, but there is a way for manual pages > to redirect from one to the other instead of duplicating them. That would be useful. Will research a bit or ask in #debian-mentors. > Any particular reason you used a different license to upstream for the > Debian directory? Generally it is advisable to use the same license so > everything is compatible license wise. The reason is that the original license seemed quite Python-specific, and I usually use GPL for my contributions. > Upstream might want to port this to Python 3 since version 2 will go > away eventually. AFAICS icdiff is currently compatible with both Python 2 and Python 3. > The upstream ChangeLog file looks more like a NEWS file to me, > ChangeLog is usually more like a git commit log generated with git2cl. True. I guess this is just how upstream understands this term. I can surely override dh_installchangelogs not to treat this file as a changelog if you want, but TBH I wouldn’t have always expected the upstream changelog to be a commit log. > Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata Done. The other things you mentioned are mostly things concerning upstream. I will see if I can fix some of them and supply PRs to them, but I’m not sure this is absolutely necessary for first upload. Please let me know what you think. New version uploaded to: https://mentors.debian.net/debian/pool/main/i/icdiff/icdiff_1.7.6-1.dsc Many thanks and best wishes, Sascha
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail