Bug#836903: RFS: qevercloud/3.0.2+ds-1 [ITP] -- Unofficial Evernote Cloud API library for Qt
Hello,
2016-09-17 22:31 GMT+08:00 Sean Whitton <spwhitton@spwhitton.name>:
> 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.
Thank you for your detailed review!
All the statements below are fixed/adjusted.
> Must-fixes:
> -----------
>
> 1. The changelog entry won't close the ITP unless you put a # in front of
> the bug number!
That is indeed my mistake. Fixed.
> 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?
The new version (2.0~beta9+dfsg-1) was pushed to GitHub, mentors
and debomatic-amd64.
Source package changed (ds -> dfsg) due to unclear license status (as
discussed previously in nixnote2 RFS).
> 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]
It was indeed because of architecture-dependent symbols of C++.
>
> 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.
Anyway I switched to shlibs using dh_makeshlibs. It is reasonable for
this package to depend on upstream to keep ABI comaptibility, but I
will also try to test symbol files on each release (even it is not included
in the tarball, it can be stored in the git history).
> Suggestions:
> ------------
>
> 1. Please add Forwarded: headers to the patches.
Added (mostly Forwarded: not-needed).
> 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.
I was trying to bypass problem No.12 using this patch but failed. After that
I forgot to remove this patch (since it will not harm anything).
This patch is removed now.
> 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.
The new qevercloud-doc added.
I dropped the tex / pdf / postscript files in the -doc package because
they are not that
useful when html files are provided as well.
> 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.
Now I declared the Section in the source package section and removed
the duplicated Section:libs in binary packages.
> 5. Since debhelper 10 has just been released, consider using compat
> level 10.
Just switched.
> 6. Are you sure you need the debian/*.dirs files? Have you tried
> building without them? They are very rarely necessary these days.
Those files are leftovers from original upstream debian packaging
scripts and are (of course) not useful at all in this situation.
Now removed.
> 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."
That is reasonable indeed. Switched to call $(MAKE).
> 8. Perhaps rename custom_regenerate_source_code to include the name of
> what you're regenerating, e.g. custom_regenerate_from_thrift.
A meaningful name can be helpful. Fixed.
> 9. Your rules file contains some very long lines. Consider inserting
> line breaks between long arguments.
Lines has been wrapped inside 80 chars.
> 10. Do you need the --list-missing override? That's useful for
> debugging but possibly confusing in a source package you want to upload.
Again some leftovers for debugging. Fixed.
> 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.
I thought dh_installdocs would install it (as stated in the man page) but it
didn't. Wrote it into docs file explicitly and patched now.
> 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.
Sure. I will subscribe to this bug.
> Remember to run dch -r to refresh the changelog timestamp after making
> changes.
Done.
The Git repository on Github has been update, and the new packages are
uploaded to mentors and debomatic-amd64. The binary packages on debomatic
suggests that nixnote2 successfully recognized the libqt5qevercloud3
shlib version
requirement.
--
Thanks,
Boyuan Yang
Reply to: