Hello Boyuan, On Wed, Sep 14, 2016 at 12:23:48PM +0800, Boyuan Yang wrote: > Yes, they are up-to-date now. The package on debian-mentors is also > updated.
The packaging is in great shape. Here's a review for you.
Must-fixes:
-----------
1. The changelog entry won't close the ITP unless you put a # in front of
the bug number!
2. We need to test building something against your new shared library,
and we'll need to confirm that the right dependencies get generated for
it by dpkg-shlibdeps. If you haven't done so already, could you update
your nixnote packaging to use your new qevercloud shlib, please?
3. In a recent commit you renamed debian/{*.symbols => *.symbols.amd64}.
So now you are not providing any symbols files for other architectures.
But for a shared library, you need to provide a symbols or shlibs file
for all the reasons described in ch. 8 of the Debian Policy Manual.
I assume that you did this rename because the symbols files is
architecture-dependent. That probably means it's very fragile: changes
which don't break the ABI might change the symbols file. This is a
known issue with C++ shared libs.[1]
You need to make the symbols file less fragile (some suggestions
involving sed(1) here[2]) so that it will work for all archs, or switch
to a shlibs file instead. README.md suggests that upstream is aware of
the issue of ABI compatibility, so shlibs files might be enough.
Suggestions:
------------
1. Please add Forwarded: headers to the patches.
2. It seems like debhelper's cmake buildsystem
(/usr/share/perl5/Debian/Debhelper/Buildsystem/cmake.pm) should handle
what your 0001 patch does. Are there situations where we wouldn't want
GNUInstallDirs? If not, please submit a bug against debhelper.
3. README.md suggests that there is doxygen documentation available for
generation and install. Please consider installing this in a new
qevercloud-doc binary package.
4. Since you added a "Section:" field to each binary package, the
"Section: libs" at the top of the file does nothing. Better to remove
it.
5. Since debhelper 10 has just been released, consider using compat
level 10.
6. Are you sure you need the debian/*.dirs files? Have you tried
building without them? They are very rarely necessary these days.
7. In your rules file you make a lot of explicit calls to dh_auto_*.
When you do something like this
override_dh_auto_clean:
dh_auto_clean
rm -rf $(_QEVERCLOUD_QT5_BUILDDIR)
it's fine, because it's clear to a reader that you are appending to an
automatic command. However, when you do this:
custom_regenerate_source_code:
(cd $(_QEVERCLOUD_GENERATOR_DIR); cmake .;)
dh_auto_build --buildsystem=makefile -- -C $(_QEVERCLOUD_GENERATOR_DIR)
mkdir -p QEverCloud/src/generated
etc.
the dh_auto_build line is quite hard to understand -- someone would need
to look up the makefile buildsystem. It would be better to replace that
with an explicit call to $(MAKE). In dh_auto_build(1) it says "If it
doesn't work, you're encouraged to skip using dh_auto_build at all, and
just run the build process manually."
8. Perhaps rename custom_regenerate_source_code to include the name of
what you're regenerating, e.g. custom_regenerate_from_thrift.
9. Your rules file contains some very long lines. Consider inserting
line breaks between long arguments.
10. Do you need the --list-missing override? That's useful for
debugging but possibly confusing in a source package you want to upload.
11. Upstream's README.md seems like a useful file. Consider installing
it to /usr/share/doc in both your -dev packages. You should patch it to
remove the stuff about building and installing the library, though.
12. Once #833789 is fixed, you can probably remove the
-DCMAKE_INSTALL_LIBDIR arguments from d/rules. It would be nice to have
a comment in d/rules referencing that bug, and explaining that it should
be possibly to simplify things in the future, to remind yourself (or a
future package maintainer). You also might want to subscribe to that bug.
Remember to run dch -r to refresh the changelog timestamp after making
changes.
[1] https://www.eyrie.org/~eagle/journal/2012-01/008.html
[2] https://wiki.debian.org/UsingSymbolsFiles
--
Sean Whitton
signature.asc
Description: PGP signature
