Bug 901659

Summary: Review Request: cura - 3D printer control software
Product: [Fedora] Fedora Reporter: Miro Hrončok <mhroncok>
Component: Package ReviewAssignee: T.C. Hollingsworth <tchollingsworth>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: notting, ry, tchollingsworth
Target Milestone: ---Flags: tchollingsworth: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: cura-13.04-2.fc17 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-04-28 23:24:56 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 905681    
Bug Blocks:    

Description Miro Hrončok 2013-01-18 13:28:34 EST
Spec URL: https://raw.github.com/hroncok/SPECS/master/cura.spec
SRPM URL: <srpm info here>

Description:
Cura is a project which aims to be an single software solution for 3D printing.
While it is developed to be used with the Ultimaker 3D printer, it can be used
with other RepRap based designs.

Cura helps you to setup an Ultimaker, shows your 3D model, allows for scaling /
positioning, can slice the model to G-Code, with sane editable configuration
settings and send this G-Code to the 3D printer for printing.

Fedora Account System Username: churchyard
Comment 2 T.C. Hollingsworth 2013-01-19 06:46:00 EST
## Note about bundling:
# It might seem like bundling skeinforge, but author of this software has said:
# 
# Mine is different, next to a bunch of bug fixes on SF50, I also made a lot of
# changes to add features. I also changed stuff to make it better integrated
# into Cura, so you cannot drop-in an SF version without changes. But I'm
# up-to-date with SF50.
#
# https://github.com/daid/Cura/issues/231#issuecomment-9317695

This requires FPC approval:
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
Comment 3 T.C. Hollingsworth 2013-01-19 07:16:46 EST
# Remove the firmware
# It is GNU GPLv3 but has no source

This is not possible.

If this firmware is available under the GPL, then you have several avenues by which to obtain the source:
http://www.gnu.org/licenses/gpl.html#section6

Otherwise, it's either a.) not really available under the GPL or b.) being unlawfully redistributed in violation of the GPL

That being said, isn't this the source?:
https://github.com/ErikZalm/Marlin

# And it is binary, so it is prohibited in Fedora

Binary firmware is permitted in Fedora:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Binary_Firmware

Infortunately, it has to be legally redistributable, and "GNU GPLv3 but no source" isn't necessarily so.

Is this firmware useful?  You seemed pretty quick to remove it.  I'm not sure if it's worth bothering about...
Comment 4 Miro Hrončok 2013-01-19 09:06:11 EST
(In reply to comment #2)
> This requires FPC approval:
> http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
Please, do the review anyway (of course without potential approval), so I know about other issues. I will work on this with upstream and FPC.

(In reply to comment #3)
> # Remove the firmware
> # It is GNU GPLv3 but has no source
> 
> This is not possible.
> 
> If this firmware is available under the GPL, then you have several avenues
> by which to obtain the source:
> http://www.gnu.org/licenses/gpl.html#section6
> 
> Otherwise, it's either a.) not really available under the GPL or b.) being
> unlawfully redistributed in violation of the GPL
It is b). The GPL firmware is distributed without source and without any link to it or even mentioning  it's GPL.

I've been talking to the author and it is fixed in Git.
https://github.com/daid/Cura/commit/7fd1847ba897cd936685030e37b29c3af92daa4a

> That being said, isn't this the source?:
> https://github.com/ErikZalm/Marlin
https://github.com/Ultimaker/Marlin (forked ErikZalm)

> Binary firmware is permitted in Fedora:
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Binary_Firmware
"The files must be necessary for the functionality of open source code being included in Fedora OR to enable Fedora to boot on a specific device, where no other reliable and supported mechanisms exist."
This is not the case.

> Infortunately, it has to be legally redistributable, and "GNU GPLv3 but no
> source" isn't necessarily so.
No, so I dropped it :)

> Is this firmware useful?  You seemed pretty quick to remove it.  I'm not
> sure if it's worth bothering about...
Cura allows user to upload firmware to the printer. The user can choose the firmware form the filesystem, therefor the firmwere doesn't need to be distributed together with the app and user can download it and use Cura to upload downloaded firmware. I've suggested to upstream to include a tool to download the firmware directly form the app.
Comment 5 Miro Hrončok 2013-01-19 09:21:20 EST
Bundling Skeinforge: Question for Fedora Packaging Committee
https://github.com/daid/Cura/issues/335
Comment 6 Miro Hrončok 2013-01-19 10:59:22 EST
Bundling exception request for Cura
https://fedorahosted.org/fpc/ticket/244
Comment 7 T.C. Hollingsworth 2013-01-20 01:25:22 EST
Wouldn't RepetierHost benefit from some of the work Cura upstream is doing to Skeinforge?  Have the two of them considered working together on a fork that better meets both projects' needs?

It seems that they both need something more libraryish than applicationish, as skeinforge was originally intended, and Cura upstream seems to already have made some improvements in that direction, in addition to other general improvements.  It seems to me that both projects could benefit from some collaboration here.

As for the firmware, it doesn't belong in this package, of course, but IMHO I think it's fine to ship in Fedora in its own package, if that would be desirable. To me it's no different than the firmware we ship to make wireless cards work; it's just some firmware loaded onto some hardware to make it work with Fedora.  I'd be happy to seek clarification from Fedora Legal on this; I think they would find this okay.
Comment 8 T.C. Hollingsworth 2013-01-20 01:56:24 EST
The runner script in Source1 has a couple issues:

Is it really necessary to cd to %{python_sitelib}?  I suspect upstream's runner script only does so because it's designed to be able to import when the files aren't in python's sitelib directory.

Also, please `exec python -m Cura.cura $@` so as not to confuse applications that might think /usr/bin/cura is the real thing.  Or better yet, just launch it in Python, like so:
---------------------
#!/usr/bin/python

import Cura.cura

cura.main()

---------------------
In fact, upstream's launcher would be a lot more simple and portable if it were written in Python too, instead of shelling out to it a bunch of times.  I might have to send them a pull request.  ;-)
Comment 9 Miro Hrončok 2013-01-20 07:07:32 EST
(In reply to comment #7)
> Wouldn't RepetierHost benefit from some of the work Cura upstream is doing
> to Skeinforge?  Have the two of them considered working together on a fork
> that better meets both projects' needs?
> 
> It seems that they both need something more libraryish than applicationish,
> as skeinforge was originally intended, and Cura upstream seems to already
> have made some improvements in that direction, in addition to other general
> improvements.  It seems to me that both projects could benefit from some
> collaboration here.
From my point of view, to RepetierHost is much easier to call an external application then integrate Python library in C# code. As far as I know, they bundle Slic3r, but they don't bundle Skeinforge, as the option in the application is more likely there just for users, who wants to use Skeinforge anyway.

For example in Printrun, you can use any slice command. Using Skeinforge (or any fork of it) as a library wouldn't do the job.
 
> As for the firmware, it doesn't belong in this package, of course, but IMHO
> I think it's fine to ship in Fedora in its own package, if that would be
> desirable. To me it's no different than the firmware we ship to make
> wireless cards work; it's just some firmware loaded onto some hardware to
> make it work with Fedora.  I'd be happy to seek clarification from Fedora
> Legal on this; I think they would find this okay.
You are suggesting to create another package, let's say ultimaker-firmware, and add is as a dep to Cura?

(In reply to comment #8)
> Or better yet, just
> launch it in Python, like so:
> ---------------------
> #!/usr/bin/python
> 
import Cura.cura as cura
> 
> cura.main()
> 
> ---------------------
Done, is's cool :)

> In fact, upstream's launcher would be a lot more simple and portable if it
> were written in Python too, instead of shelling out to it a bunch of times. 
> I might have to send them a pull request.  ;-)
You mean something like this?

try:
    import OpenGL
except:
    print "Error..."
    exit
...

Feel free to send the request, or I might to do it as well.

Spec URL: https://raw.github.com/hroncok/SPECS/master/cura.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/cura-12.12-2.fc18.src.rpm
Comment 10 Miro Hrončok 2013-01-20 11:56:39 EST
Currently, the first run wizzard will fail when there is no firmware.

So I was mistaken, the firmware should be present. I'll try to make the firmware package.
Comment 11 T.C. Hollingsworth 2013-01-20 19:29:57 EST
I'll let you do the pull request, since you can probably test it better and more easily than me.

I was thinking something like:
----------------------------------------------------
#!/usr/bin/python

import os, sys

try:
    import OpenGL
    import wx
    import serial
    import numpy
    import power
except ImportError as e:
    module = e.message.lstrip('No module named ')
    print 'Requires ' + module

    if module == 'power':
        print "Install from: https://github.com/GreatFruitOmsk/Power"
    else:
        print "Try sudo easy_install " + module

sys.path.insert(os.path.dirname(__file__))

import Cura.cura as cura

cura.main()

----------------------------------------------------

Seems a little simpler than five sets of try/except or ifs.  But that might not work out so well with the differing instructions to install the various modules.
Comment 12 T.C. Hollingsworth 2013-01-21 00:41:02 EST
Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed

Resolution: NEEDS WORK

===== Issues =====

[!]: This package contains a bundled copy of skeinforge.

     The Fedora Packaging Committee is reviewing a bundling request, and will
     decide if this is acceptable.

[!]: A Fedora-specific wrapper script is provided that does not use `exec`.

     I guess you fixed this one already.  ;-)

[!]: There are strange permissions in the source RPM.

     See the rpmlint section for details.

[!]: Please explain the Requires on PyPy.

     See Requires section for details.

==== Things to Consider ====

[ ]: No %check section is provided.

     Please make sure upstream provides no test cases, even if they are
     independent of the main codebase.

[ ]: Make sure `#!/usr/bin/env python` is not used.

     See Requires section for details.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[-]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[!]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 1 files have unknown license. Detailed output of
     licensecheck in /home/fedora/patches/901659-cura/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[-]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[-]: Large documentation must go in a -doc subpackage.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep
[-]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[!]: SourceX / PatchY prefixed with %{name}.
     Note: Source0 (Cura-12.12-linux-fedora.tar.gz)
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
[-]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------
Checking: cura-12.12-1.fc19.noarch.rpm
          cura-12.12-1.fc19.src.rpm
cura.noarch: W: no-manual-page-for-binary cura

GUI app; okay.

cura.src: W: strange-permission cura-stripper.sh 0775L

Please `chmod 0755` or explain the need for strange permissions.

cura.src: W: strange-permission cura 0755L

This is fine, the script should be executable.

cura.src: W: invalid-url Source0: Cura-12.12-linux-fedora.tar.gz

This is fine, tarball generated to remove fordbidden CC-BY-NC content.

2 packages and 0 specfiles checked; 0 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint cura
cura.noarch: W: no-manual-page-for-binary cura
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'

All taken care of above.

Requires
--------
cura-12.12-1.fc19.noarch.rpm (rpmlib, GLIBC filtered):
    
    /usr/bin/env

I assume this is the `#!/usr/bin/env bash` in the old launcher script, but
please do make sure `#!/usr/bin/env python` is never used.  RPM installed Python
software should always use the system interpreter located at /usr/bin/python,
not just any Python interpreter that happens to be in $PATH.
    
    PyOpenGL
    numpy
    pypy

What exactly does cura use pypy for?  Both upstream and your launcher scripts
use CPython, and all the libraries installed only work with CPython.

    pyserial
    python(abi) = 2.7
    python-power
    wxPython

    
Provides
--------
cura-12.12-1.fc19.noarch.rpm:
    
    cura = 12.12-1.fc19



MD5-sum check
-------------

Not possible; tarball is generated to remove prohibited content.

Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -b901659
Comment 13 Miro Hrončok 2013-01-21 08:43:43 EST
> I'll let you do the pull request, since you can probably test it better and
> more easily than me.
OK

> sys.path.insert(os.path.dirname(__file__))
What exactly is this part for?

> [!]: A Fedora-specific wrapper script is provided that does not use `exec`.
>      I guess you fixed this one already.  ;-)
Yes.

> [!]: There are strange permissions in the source RPM.
> cura.src: W: strange-permission cura-stripper.sh 0775L
> 
> Please `chmod 0755` or explain the need for strange permissions.
OK.

> [!]: Please explain the Requires on PyPy.
> What exactly does cura use pypy for?  Both upstream and your launcher scripts
> use CPython, and all the libraries installed only work with CPython.
PyPy is used for slicing.

> [ ]: Make sure `#!/usr/bin/env python` is not used.
$ grep -r '/usr/bin/env' . || echo "Tadaa"
Tadaa
Comment 14 Miro Hrončok 2013-01-21 09:29:13 EST
(In reply to comment #13)
> > sys.path.insert(os.path.dirname(__file__))
> What exactly is this part for?

OK, I got it, changed to:
sys.path.insert(1,os.path.dirname(__file__))

https://github.com/daid/Cura/pull/337

T.C., are you willing to help me with the firmware package? If so, please ping me on IRC (mhroncok), thanks.
Comment 15 T.C. Hollingsworth 2013-01-21 21:48:11 EST
(In reply to comment #14)
> T.C., are you willing to help me with the firmware package? If so, please
> ping me on IRC (mhroncok), thanks.

Sure, but it'll have to be later this week.  When is a good time for you?  I think my weird schedule in my time zone works great with a normal schedule in your time zone, but I want to make sure.  ;-)
Comment 16 Miro Hrončok 2013-02-19 16:56:10 EST
Spec URL: https://raw.github.com/hroncok/SPECS/master/cura.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/cura-12.12-3.fc18.src.rpm

- chmod 755 cura-stripper.sh
- Use firmware from ultimaker-marlin-firmware package
- removed bundling note (still waiting for FPC approval)
Comment 17 Ryan Rix 2013-03-20 17:56:00 EDT
FYI there's a new release out:

http://blog.ultimaker.com/2013/03/19/poduct-innovation-cura-13-03/
Comment 18 Miro Hrončok 2013-03-21 10:11:32 EDT
Thanks. Still waiting for FPC approval anyway.
Comment 20 T.C. Hollingsworth 2013-04-24 13:24:45 EDT
FPC has determined that a bundling exception is not needed in this instance:
https://fedorahosted.org/fpc/ticket/244#comment:2

All issues from the prior review have been fixed, and no new issues have cropped up, therefore this package is APPROVED.
Comment 21 Miro Hrončok 2013-04-24 13:47:36 EDT
Thanks for quick reaction (and the review), I was just going to inform you about that :)
Comment 22 Miro Hrončok 2013-04-24 13:48:34 EDT
New Package SCM Request
=======================
Package Name: cura
Short Description: 3D printer control software
Owners: churchyard
Branches: f17 f18 f19
Comment 23 Jon Ciesla 2013-04-24 13:55:54 EDT
Git done (by process-git-requests).
Comment 24 Fedora Update System 2013-04-24 14:40:00 EDT
cura-13.03-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/cura-13.03-1.fc19
Comment 25 Fedora Update System 2013-04-24 14:42:28 EDT
cura-13.03-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/cura-13.03-1.fc18
Comment 26 Fedora Update System 2013-04-24 14:43:18 EDT
cura-13.03-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/cura-13.03-1.fc17
Comment 27 Fedora Update System 2013-04-24 23:42:44 EDT
cura-13.03-1.fc19 has been pushed to the Fedora 19 testing repository.
Comment 28 Fedora Update System 2013-04-28 23:24:58 EDT
cura-13.03-1.fc19 has been pushed to the Fedora 19 stable repository.
Comment 29 Fedora Update System 2013-05-04 11:28:28 EDT
cura-13.04-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/cura-13.04-1.fc18
Comment 30 Fedora Update System 2013-05-04 11:29:53 EDT
cura-13.04-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/cura-13.04-1.fc17
Comment 31 Fedora Update System 2013-05-07 10:04:15 EDT
cura-13.04-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/cura-13.04-2.fc18
Comment 32 Fedora Update System 2013-05-07 10:11:59 EDT
cura-13.04-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/cura-13.04-2.fc17
Comment 33 Fedora Update System 2013-05-18 22:31:58 EDT
cura-13.04-2.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 34 Fedora Update System 2013-05-18 22:40:49 EDT
cura-13.04-2.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.