Bug#706656: [3dprinter-general] Bug#706656: ITP: cura -- Controller for 3D printers
Hi Rock,
Here are my comments, I must say I don't use the software so I only
checked the building and the packaging. I trust you are testing that
once installed all four packages perform as expected :).
Thanks for the review!
I'll comment below.
> libArcus
> ========
>
> debian/changelog
> ----------------
>
* There seems to be a line in the changelog that is too long, it'd be
nice to split it into two so it fits into the "80 character limit".
Ok, I thought this wasn't so important, so I ignored the lint warning.
Who still uses a 80-character terminal in this day and age anyway? :)
* Typically, new packages contain only a single entry with a line
similar to "Initial Release. Closes: #nnnnn". The changelog should
only contain entries for actually released revisions. In this case,
if version 2.1.3-1 never made it into Debian it should be removed
and if version 2.3.0-1 is going to be the first to get into then
this should be the one and only entry in the changelog.
There is a particular reason why packaged 2.1.3 first:
2.3.0 has several usability problems for me, such as widgets that are cut off
at the bottom and blurry font rendering. These were not present in 2.1.3.
I still need to test 2.3.1 though, perhaps these problems have been fixed already.
Also, when I submitted Cura to Debian Mentors, I hadn't tested 2.3.0 and 2.3.1
yet and wanted to get feedback on the packaging as a whole first. But I will
certainly update the changelog accordingly, when review is complete.
> debian/control
> --------------
>
* Since "3.0 (quilt)" souce package format it is no longer needed to
list "quilt" as a build-dependency [1]. Patches can now be handled
by dpkg-source. In fact you don't even need the "--with quilt" flag
on debian/rules (I tried removing this flag and it built correctly,
please let me know if doesn't work for you)
Ok, I didn't know this. Will be corrected.
* The VCS fields should point to "where the Debian source package is
developed" [2], that is, where the changes to the debian folder are
made, which in this case would be your GitHub repository and not
upstream's.
Ok.
* Normally, the binary packages providing shared libraries are named
as "libfooX" where foo would be the name of library and X the
"major-version" [3]. In your case this would mean that the binary
package that provides libArcus.so.3 should be named "libarcus3"
instead of just "libarcus". However I don't quite get what's going
on with this library's versioning. This packages provides
"libArcus.so.1.1.0" and a link to it called "libArcus.so.3", is
there a reason for this? To my understanding the latter should be
called "libArcus.so.1" and therefore the binary package would end up
being "libarcus1". Nevertheless, I'm no expert and it seems I'm
missing something here.
Yes, I noticed the warnings as well.
However, upstream seems to prefer naming and versioning as they are now, so I
didn't want to change them.
As far as I can tell, this isn't going to be a problem, as there aren't any
other packages besides Cura that use libArcus anyway.
debian/rules
------------
* Lintian reports the tags "hardening-no-fortify-functions" and
"hardening-no-bindnow". There's an ongoing effort to "update as many
packages as possible to use security hardening build flags". You
might want to try to deal with it, sometimes it is as "simple" as
adding "export DEB_BUILD_MAINT_OPTIONS = hardening=+all".
Ok, I'll try that.
debian/watch
------------
* It'd be nice to include a watch file, some Debian tools rely on this
file to i.e. estimate the "freshness" of the Debian repository as a
whole. It should be particularly easy to write a wath file in your
case since upstream uses GitHub, check out this template [4].
Ok.
debian/patches
--------------
* Although not mandatory you might want to adhere to the "Patch
Tagging Guidelines" [5]
I'll have a look.
CuraEngine
==========
* It would be nice to include a manpage explaining what the command
CuraEngine does and the command-line options it accepts. Also it
might be necessary to rename it to "curaengine" for the sake of tab
completion and such, but I'm not sure about this right now.
This will definitely cause problems with Cura, as it expects the program to be
named "CuraEngine" and I'd prefer not to modify the upstream sources unless
it's strictly necessary.
Also, CuraEngine is a core component of Cura, and while I assume it can be
used standalone, it's usually not meant to be executed from the command line.
But I can take a look and provide a simple man page, if that's desired.
Cura
====
* This one I haven't been able to build. I'm attaching the build log.
It might be an error on my building tool-chain but please check it
out, just in case. Error shows up around line 583.
Woops, looks like I forgot to merge a patch back into the 2.3.0 branch. One
moment please.... It should work now.
[1] https://pkg-perl.alioth.debian.org/howto/quilt.html#The_%22Post-Mod
ern%22_Way_%28%223.0_%28quilt%29%22%29
[2] https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-
VCS-fields
[3] https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#s-share
dlibs-runtime
[4] https://wiki.debian.org/debian/watch#GitHub
[5] http://dep.debian.net/deps/dep3/
Thanks,
Greg
Reply to: