Re: sfcgal package for postgis and others
Hi Sven,
Thanks for your packaging improvements, those are very welcome changes.
Next please a commit for every logical change to ease review of the
changes. See also:
http://pkg-grass.alioth.debian.org/policy/packaging.html#git-commit-policy
On 06/13/2015 12:50 AM, Sven Geggus wrote:
> Sebastiaan Couwenberg <sebastic@xs4all.nl> wrote:
>
>> The problematic quertbts is not used if you use reportbug with the
>> options I gave earlier:
>>
>> reportbug -I -b wnpp
>
> Ah OK.
Thanks for the ITP, I saw it debian-devel@. It's usually a good idea to
include one of the team lists in the CC. You can do that with the 's'
option in reportbug.
>> Please file the ITP for sfcgal and add the "(closes: #XXXXXX)" to the
>> package changelog to close the ITP when the package enters the archive.
>
> This will be easy then...
But since the ITP number is now included in the changelog we can find
the bugreport is easily found. :-)
>> Thanks for the push, I've reviewed the package and it needs some
>> improvements before it can be uploaded to the archive.
>
> I already expected this but looks lile it's even more bureaucracy
> than I expected :( I will try to fix that stuff and push a corrected
> version, thanks for your patience.
>
> Working on the package while writing the answer to this mail. Looks
> like this seems to get a debian packaging journey for beginners who
> thought that they already knew how to do this ;)
You can expect to learn more package maintenance and its best practices.
It takes some time and practice to refine like any other skill.
>> - debian/changelog:
>>
>> The changelog entry still needs to close the ITP bug as mentioned before.
>>
>> You should also consider changing the urgency to medium so the package
>> will migrate to testing after 5 days instead of 10.
>
> Do I also need to change the distribution field from UNRELEASED to
> unstable?
Not yet. Changing the distribution (with `dch -r`) is the last thing you
do before uploading the package to mentors or the archive.
First you need to make sure any issues are resolved. That usually takes
a couple of tries to get everything right.
With the respect to the changelog file, you didn't need to change the
author of the changelog entry, that's still you, not the team.
The requirements for the changelog file are documented in section 4.4 of
the Debian Policy, to quote the relevant paragraph:
"
The maintainer name and email address used in the changelog should be
the details of the person who prepared this release of the package.
They are not necessarily those of the uploader or usual package
maintainer.[18] The information here will be copied to the Changed-By
field in the .changes file (see Changed-By, Section 5.6.4), and then
later used to send an acknowledgement when the upload has been
installed.
"
https://www.debian.org/doc/debian-policy/ch-source.html#s-dpkgchangelog
This was recently changed in Policy Version 3.9.6.0, see:
https://www.debian.org/doc/debian-policy/upgrading-checklist.html#s-3.9.6.0
>> The control file should be reformatted using the cme tool to have
>> consistent formatting of the control file. See:
>>
>> http://pkg-grass.alioth.debian.org/policy/policy.html#cme
>
> OK, cme seems to be fine for now.
Yes. Recently cme changes the formatting to no longer put dependencies
on a separate line. This gives cleaner diffs when changing dependencies
making it easier to review.
>> The duplicate descriptions need to be fixed too, lintian complains about
>> these.
>
> lintian didn't complain about anything, I will retry using --pedantic
The duplicate description issues are at the info level, so you need to
at least use the lintian -I option, the pbuilder hook does this by
default and the hook changes documented in the Debian GIS Policy enables
all lintian options.
http://pkg-grass.alioth.debian.org/policy/packaging.html#git-pbuilder-hooks
The pedantic and extra tags are also shown because it's good to be aware
of these issues even if it's not required to fix these before uploading
the package.
>> You should also add a Files section for debian/* for your packaging
>> license & copyright.
>
> I would consider them a contribution to the original sources and thus
> of course also GPL-2+.
Thanks good, and make sense.
While it's not required to use the same license as the upstream code for
the Debian packaging it's important to use a compatible license at least.
>> Also consider using stand-alone license specifications, because you'll
>> likely use the same license for your Debian packaging. See:
>>
>> https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#stand-alone-license-paragraph
>
> Project Homepage states the following:
>
> "SFCGAL is distributed under the terms of the GNU Lesser General Public License 2+."
>
> So I went for "Example 3. Simple"
> https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#examples
>
> cme check dpkg-copyright is fine with this now.
>
>> The licensecheck tool reports several files licensed under the LGPL-2+ &
>> LGPL-3+, you need to document these files in the copyright separately.
>
> Those files in "patches" subdir are not part of sfcgal at all but
> patches for older Versions of CGAL (4.2/4.3) and not used in the
> package build anyway as we are building for CGAL 4.5 (debian 8.x)
> and above.
The debian/copyright file documents the license used for the files in
the source package, not of the resulting build.
The copyright file is one of the most important (if not the most
important) to get right. If your copyright file is not in order the FTP
masters will reject the package and prevent the package from entering
the archive.
The copyright requirement are documented in the Debian Policy:
https://www.debian.org/doc/debian-policy/ch-source.html#s-dpkgcopyright
https://www.debian.org/doc/debian-policy/ch-docs.html#s-copyrightfile
https://www.debian.org/doc/debian-policy/ch-archive.html#s-pkgcopyright
See also:
https://www.debian.org/doc/manuals/maint-guide/dreq.en.html#copyright
>> - debian/docs
>>
>> Because the package is not specified for the docs, it will be applied to
>> the first package in debian/control only, you should include the README
>> in all binary packages.
>
> OK, put them into all of them
Thanks, looks good.
>> Instead of including the NEWS file in the documention, you should
>> override dh_installchangelogs in debian/rules to install the NEWS file
>> as the upstream changelog.
>
> NACK. It does not look like the NEWS file is up-to date at all. I
> would suggest to better get rid of it altogether.
There is indeed not much value to including the outdated NEWS file.
>> - debian/get-orig-source
>>
>> This script should be removed in favor of plain uscan with the
>> debian/watch file.
>>
>> A get-orig-source script is only required if uscan cannot be used to
>> download the upstream tarball.
>
> Ah, just got this from your debian-gis skeleton. I updated
> debian/watch according to your suggestions.
>
> I had no Idea of the existance of this file up to now.
>
> Is there some kind of automatism which will run uscan from time to
> time or do I have to set this up for myself?
Yes, all packages in the archive get regularly checked for new upstream
releases. You can check the results with the watch service for instance:
https://qa.debian.org/cgi-bin/watch
The Watch column in DDPO is populated with this too:
https://qa.debian.org/developer.php?login=pkg-grass-devel@lists.alioth.debian.org
There were some issues with SSL changes in jessie after the upgrade of
servers hosting these services, that's why a lot of upstream versions
are missing in DDPO and the related Dashboard service:
https://udd.debian.org/dmd.cgi?email1=pkg-grass-devel%40lists.alioth.debian.org
I use a simple script to check the watch file for all the package
repositories I have locally to keep an eye on the upstream releases
while the issues with the watch service get resolved.
>> - debian/rules
>>
>> Don't install the man page via the rules file, use
>> debian/libsfcgal-dev.manpages to have dh_installman do the work for you.
>
> Still not used to all that modern stuff. I build most of my debian
> packages back in the times whe configure; make; make install have
> been called directly from debian/rules.
Modern packaging uses debhelper with does the typical build process
automatically. The dh_auto_* tools and the dh tool are great.
In your new rules file you can still get rid of the
DPKG_EXPORT_BUILDFLAGS & include lines, as of the comments around
override_dh_install.
>> Why are the tests skipped?
>
> Because the least important one did not work :(
Instead of disabling the tests entirely it's better ignore the failures
if the tests cannot be fixed. A typical way to do that is with:
override_dh_auto_test:
dh_auto_test || echo "Ignoring test failures"
> I have an Idea, but I did not figure out why.
>
> Looking at http://oslandia.github.io/SFCGAL/dev.html we are
> talking about the "style test" This is actually calling the "astyle"
> source code beautifier and for some reason the version from debian
> stable does not seem to play well with their script:
The unstable astyle version has issues too, but the failure is easy to
ignore.
According to the upstream installation instructions the
SFCGAL_BUILD_TESTS option needs to be enabled for the regression tests
to be built.
http://oslandia.github.io/SFCGAL/installation.html
While the upstream installation instructions don't mention it, you
should consider CMAKE_BUILD_TYPE=RelWithDebInfo so you can add a debug
package with dh_strip.
The examples are not built by default, and that's okay because there is
not much value other than verifying they can be built. It still a good
idea to inlude the code in the upstream example/ directory in the
libsfcgal-dev package. Add an libsfcgal-dev.examples file to have
dh_installexamples(1) install them in usr/share/doc/<package>/examples/.
> --cut--
> $ ./script/test-style-SFCGAL.sh
>
> ...
>
> diff in ./viewer/src/SFCGAL/viewer/plugins/DataPlugin.cpp
> diff in ./viewer/src/SFCGAL/viewer/plugins/GridPlugin.cpp
> ******* 298 errors found
> --cut--
>
> The other (more important) tests passed, but I did not find a good
> way to disable only one of them.
>
> Frankly, I have been bitten by broken tests (e.g. ones not running on
> NFS home directories) way too often in recent times while building
> backports. That's why I know how to disable them in the first place :)
>
Enabling the SFCGAL_BUILD_TESTS option adds additional tests, but they
all fail because they cannot find libSFCGAL.so.1. The tests need to be
fixed to use the just built library to run.
So leaving the tests off seems sensible.
> Way to late now to fix the rest, but I consider them mostly minor or
> even ridiculous. Do you really want me to fix spelling errors in
> function calls?
Fixing the 'informations' typo in the comments is an option, fixing the
function name will change the ABI so that shouldn't be done without
discussion with upstream, but since it only affects the viewer that
shouldn't be an issue. Forwarding this kind of patches is a good way to
get in contact with your upstream, and participate as a good Free
Software citizen by sharing improvements.
The 'inconsistant' typo is in an string that can easily be fixed. For
the Googlebility of error messages is not a good idea to keep this fixes
confined to the Debian package, upstreaming your changes allows the
wider community to benefit.
Kind Regards,
Bas
--
GPG Key ID: 4096R/6750F10AE88D4AF1
Fingerprint: 8182 DE41 7056 408D 6146 50D1 6750 F10A E88D 4AF1
Reply to: