[3dprinter-general] [Fwd: Re: Bug#706656: ITP: cura -- Controller for 3D printers]

Tue Dec 20 14:44:56 UTC 2016

-------- Forwarded Message --------

Date: Mon, 19 Dec 2016 22:51:52 +0100
Subject: Re: [3dprinter-general] Bug#706656: ITP: cura -- Controller
for 3D printers
To: Rock Storm <rockstorm at gmx.com>, 706656 at bugs.debian.org
From: Gregor Riepl <onitake at gmail.com>
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-M
> od
> 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-sha
> re
> dlibs-runtime
> [4] https://wiki.debian.org/debian/watch#GitHub
> [5] http://dep.debian.net/deps/dep3/

Thanks,
Greg