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
Attachment:
signature.asc
Description: PGP signature