Bug 1432983 - Review Request: camotics - Open-Source Simulation & Computer Aided Machining - A 3-axis CNC GCode simulator
Summary: Review Request: camotics - Open-Source Simulation & Computer Aided Machining ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1457949
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2017-03-16 13:59 UTC by srakitnican
Modified: 2017-07-25 05:48 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-07-15 18:49:48 UTC
Type: ---
Embargoed:
lkundrak: fedora-review+


Attachments (Terms of Use)

Description srakitnican 2017-03-16 13:59:20 UTC
Spec URL: https://transfer.sh/yvaxy/camotics.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-25-x86_64/00525563-camotics/camotics-1.1.1-1.fc25.src.rpm

Description: 
CAMotics is an Open-Source software which can simulate
3-axis NC machining. It is a fast, flexible and user friendly simulation
software for the DIY and Open-Source community.

At home manufacturing is one of the next big technology revolutions. Much like
the PC was 30 years ago. There have been major advances in desktop 3D printing
(e.g.  Maker Bot) yet uptake of desktop CNCs has lagged despite the
availability of cheap CNC machines. One of the major reasons for this is a
lack of Open-Source simulation and CAM (model to tool path conversion)
software. CAM and NC machine simulation present some very difficult
programming problems as evidenced by 30+ years of academic papers on these
topics. Whereas 3D printing simulation and tool path generation is much
easier. However, such software is essential to using a CNC.

Being able to simulate is a critical part of creating usable CNC tool paths.
Programming a CNC with out a simulator is cutting with out measuring; it's
both dangerous and expensive. With CAMotics you can preview the results of
your cutting operations before you fire up your machine. This will
save you time and money and open up a world of creative possibilities by
allowing you to rapidly visualize and improve upon designs without wasting
material or breaking tools.


Fedora Account System Username: srakitnican

Comment 1 Lubomir Rintel 2017-04-30 14:01:29 UTC
This looks rather well done. Just a couple of comments:

1.) Please unbundle cbang

> %global commit1 9f80bae739a36727a5f66e5f1b0c55cd9ee1c01a
> %global name1 cbang
> %global shortcommit1 %(c=%{commit1}; echo ${c:0:7})
...
> Source1:        https://github.com/CauldronDevelopmentLLC/cbang/archive/%{commit1}.tar.gz#/%{name1}-%{shortcommit1}.tar.gz
...
Looks like this should go into a separate package.
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

2.) Don't disable debuginfo

> # With debug flags camotics executable does not run
> %global debug_package %{nil}
...
> # Trim ~2MB more, manually since not a debug build
> %{__strip} %{buildroot}%{_bindir}/*

Eeek! Don't do this.

What didn't work here? It certainly works all right on my system.

3.) Submit the MIME info upstream

> Source2:        camotics.xml

It's okay to add it in the package, but please ask upstream to include it and add a comment with a link to the upstream ticket.

4.) Why is this conditional on Fedora?

> cd %{_builddir}/%{name1}-%{commit1}
> scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}
> 
> cd %{_builddir}/CAMotics-%{version}
> scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}

5.) Please add BuildRequires: gcc-c++

The guindelines seem to mandate it now:
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B

The rest of the review will follow.

I'm a packager sponsor, so I can eventually sponsor you when we sort this review out.

Comment 2 Lubomir Rintel 2017-04-30 14:10:27 UTC
Also, please ask upstream to consider shipping AppStream metadata. That way the package would be installable via app stores such as GNOME Software.
https://fedoraproject.org/wiki/Packaging:AppData

Comment 3 srakitnican 2017-05-01 09:13:39 UTC
(In reply to Lubomir Rintel from comment #1)
> This looks rather well done. Just a couple of comments:
> 
> 1.) Please unbundle cbang
> 
> > %global commit1 9f80bae739a36727a5f66e5f1b0c55cd9ee1c01a
> > %global name1 cbang
> > %global shortcommit1 %(c=%{commit1}; echo ${c:0:7})
> ...
> > Source1:        https://github.com/CauldronDevelopmentLLC/cbang/archive/%{commit1}.tar.gz#/%{name1}-%{shortcommit1}.tar.gz
> ...
> Looks like this should go into a separate package.
> See:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Bundling_and_Duplication_of_system_libraries

Well, initially I had it as a separate package, but I've ran into all sort of problems because it is not really intended to be packaged separately, as the author itself told me. For example CAMotics is using configuration files from cbang for scons and build dependencies checking is done as a one unit. Also I have found cbang compiles mostly into static library files. If you still think that should be packaged separately, I can do that.


> 2.) Don't disable debuginfo
> 
> > # With debug flags camotics executable does not run
> > %global debug_package %{nil}
> ...
> > # Trim ~2MB more, manually since not a debug build
> > %{__strip} %{buildroot}%{_bindir}/*
> 
> Eeek! Don't do this.
> 
> What didn't work here? It certainly works all right on my system.

With debug flag I ran into an issue where compiled binary immediately crashed without starting up. Also compiled binaries file sizes did not look right, e.g. smaller then upstream. So debug_package is here in order for rpmbuild to not error out on missing debuginfo.


> 3.) Submit the MIME info upstream
> 
> > Source2:        camotics.xml
> 
> It's okay to add it in the package, but please ask upstream to include it
> and add a comment with a link to the upstream ticket.
> 

Submited and included, will make a note in spec file.
https://github.com/CauldronDevelopmentLLC/CAMotics/issues/211

> 4.) Why is this conditional on Fedora?
> 
> > cd %{_builddir}/%{name1}-%{commit1}
> > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}
> > 
> > cd %{_builddir}/CAMotics-%{version}
> > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}

cbang does not work with newer version of v8 which Fedora defaults to. I guess conditional is not strictly required, but I though it would be more straightforward to mark what exactly needs it because RHEL still uses 3.14 as default.


> 5.) Please add BuildRequires: gcc-c++
> 
> The guindelines seem to mandate it now:
> https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B
> 
> The rest of the review will follow.
> 
> I'm a packager sponsor, so I can eventually sponsor you when we sort this
> review out.

Thanks!

Comment 4 Lubomir Rintel 2017-05-11 11:22:19 UTC
(In reply to srakitnican from comment #3)
> (In reply to Lubomir Rintel from comment #1)
> > This looks rather well done. Just a couple of comments:
> > 
> > 1.) Please unbundle cbang
> > 
> > > %global commit1 9f80bae739a36727a5f66e5f1b0c55cd9ee1c01a
> > > %global name1 cbang
> > > %global shortcommit1 %(c=%{commit1}; echo ${c:0:7})
> > ...
> > > Source1:        https://github.com/CauldronDevelopmentLLC/cbang/archive/%{commit1}.tar.gz#/%{name1}-%{shortcommit1}.tar.gz
> > ...
> > Looks like this should go into a separate package.
> > See:
> > https://fedoraproject.org/wiki/Packaging:
> > Guidelines#Bundling_and_Duplication_of_system_libraries
> 
> Well, initially I had it as a separate package, but I've ran into all sort
> of problems because it is not really intended to be packaged separately, as
> the author itself told me. For example CAMotics is using configuration files
> from cbang for scons and build dependencies checking is done as a one unit.
> Also I have found cbang compiles mostly into static library files. If you
> still think that should be packaged separately, I can do that.

That is fair enough. However, please add a comment explaining why do you bundle cbang and add a Provides: bundled(cbang) as described in the wiki paragraph linked to above.

> > 2.) Don't disable debuginfo
> > 
> > > # With debug flags camotics executable does not run
> > > %global debug_package %{nil}
> > ...
> > > # Trim ~2MB more, manually since not a debug build
> > > %{__strip} %{buildroot}%{_bindir}/*
> > 
> > Eeek! Don't do this.
> > 
> > What didn't work here? It certainly works all right on my system.
> 
> With debug flag I ran into an issue where compiled binary immediately
> crashed without starting up. Also compiled binaries file sizes did not look
> right, e.g. smaller then upstream. So debug_package is here in order for
> rpmbuild to not error out on missing debuginfo.

Well, this needs to be fixed first.

I could help with that, but am not able to reproduce the issue. Please share as much information as you can -- what version and architecture are you building on?  Can you reproduce the bug with a mock build?

> > 3.) Submit the MIME info upstream
> > 
> > > Source2:        camotics.xml
> > 
> > It's okay to add it in the package, but please ask upstream to include it
> > and add a comment with a link to the upstream ticket.
> > 
> 
> Submited and included, will make a note in spec file.
> https://github.com/CauldronDevelopmentLLC/CAMotics/issues/211

Thank you.

> > 4.) Why is this conditional on Fedora?
> > 
> > > cd %{_builddir}/%{name1}-%{commit1}
> > > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}
> > > 
> > > cd %{_builddir}/CAMotics-%{version}
> > > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags}
> 
> cbang does not work with newer version of v8 which Fedora defaults to. I
> guess conditional is not strictly required, but I though it would be more
> straightforward to mark what exactly needs it because RHEL still uses 3.14
> as default.

That is okay. But please add a comment explaining why are you doing that.

> > 5.) Please add BuildRequires: gcc-c++
> > 
> > The guindelines seem to mandate it now:
> > https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B
> > 
> > The rest of the review will follow.
> > 
> > I'm a packager sponsor, so I can eventually sponsor you when we sort this
> > review out.
> 
> Thanks!

You're welcome.

Comment 5 Lubomir Rintel 2017-05-11 12:12:59 UTC
Rest of the review:

* Package named correctly
* Packaging the latest version
* License good for Fedora (see below for remarks about the license tag)
* License text included
* Filelist sane
* Requires/Provides sane
* Scriptlets look good

6.) Please unbundle cairo, GLEW and perhaps dxflib (ideally ask upstream to allow building against system copies). See the wiki reference above for explanation why bundling is bad.

7.) Use the correct compiler flags. E.g. change

  scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" 

to

  scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/} %{optflags}"

wherever appropriate.

8.) The license tag is incorrect:

  License:        LGPLv2

It looks like the camotics source is GPLv2+. (Some of the bundled files use different licenses -- if it's not possible to unbundle them then they should be included in the License tag).

9.) rpmlint is unhappy

camotics.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/camotics/examples/tiger/tiger.nc
camotics.x86_64: W: spurious-executable-perm /usr/share/doc/camotics/examples/camotics/camotics.svg
camotics.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/camotics/examples/cameo/cameo.nc
camotics.x86_64: E: script-without-shebang /usr/share/camotics/tpl_lib/clipper/clipper.js
camotics.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/camotics/examples/stl-test/V1.stl

Please at least remove the executable bits; ideally also recode the DOS line endings in examples.

Comment 6 srakitnican 2017-05-25 15:54:12 UTC
Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/srakitnican/default/camotics.git/plain/camotics.spec?id=0166e50ff661e785a5ed7e23c276ee7875b7e81a
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-25-x86_64/00556754-camotics/camotics-1.1.1-2.fc25.src.rpm

Changes from the last version:

* mariadb-devel seems to not be required so I've dropped it
* qt-devel and openssl-devel as pkgconfig modules
* Removed strip step
* Debug build
* Patch to fix misleading-indentation error that shows up with particular flags
* Updated cbang, fixes compilation error with optflags
* Commented unusual procedures
* Added Provides bundle(cbang)
* Added gcc-c++ build requirement
* Corrected license tag partially
* Fixed line-endings and removed executable bit from datadir files

Still left to do:

* Debug build possibly does not work (compiled executable aborts with a coredump)
* AppStream metadata
* Possibly unbundle Cairo, GLEW and dxflib
* License tag according to bundled libraries


(In reply to Lubomir Rintel from comment #4)
> (In reply to srakitnican from comment #3)
> > With debug flag I ran into an issue where compiled binary immediately
> > crashed without starting up. Also compiled binaries file sizes did not look
> > right, e.g. smaller then upstream. So debug_package is here in order for
> > rpmbuild to not error out on missing debuginfo.
> 
> Well, this needs to be fixed first.
> 
> I could help with that, but am not able to reproduce the issue. Please share
> as much information as you can -- what version and architecture are you
> building on?  Can you reproduce the bug with a mock build?

Yes, I am using only mock.

In this version I am trying to make a debug build, so I've removed everything that would otherwise prevent this from happening. Binaries size looks correct, but produced binaries (stripped or unstripped) does not work for me. In order to reproduce failed build I can simply add debug=1 option to scons in all stages.

Without scons debug=1 flag, binaries seems to get "stripped" by build system before rpm gets a chance to do anything.

Comment 7 srakitnican 2017-05-26 16:11:02 UTC
camotics-1.1.1-3

SPEC URL: http://copr-dist-git.fedorainfracloud.org/cgit/srakitnican/default/camotics.git/plain/camotics.spec?id=29ed27f71d3772f02ae02245c978d23a6d54bcdc
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-25-x86_64/00557191-camotics/camotics-1.1.1-3.fc25.x86_64.rpm

Changes from 1.1.1-2:

* Use Cairo and GLEW from system
* Updated cbang that includes additional fixes 


Joseph Coffland implemented possibility to use Cairo, GLEW and dxflib as system libraries if present. I've backported Cairo and GLEW patches to version 1.1.1. Although CAMotics can use dxflib from system, Joseph pointed out that there is a feature missing in dxflib that CAMotics require. Joseph also submited pull request upstream [1].

Until this is sorted out we can't make use of dxflib as a system library.


Interestingly unbundling Cairo and GLEW seems to fix coredump issue, so now CAMotics works as a debug build.


[1] https://github.com/qcad/qcad/pull/15

Comment 8 srakitnican 2017-06-01 15:45:05 UTC
The requested change in dxflib was partially accepted upstream, so I went ahead and built a package with required changes: bug 1457949

I have a working camotics package with such a change already.

Comment 9 srakitnican 2017-06-01 23:05:41 UTC
I believe I've got everything now and some more! (or less)
SPEC: http://copr-dist-git.fedorainfracloud.org/cgit/srakitnican/default/camotics.git/plain/camotics.spec?id=d0a5a1e8a194d3188887b5687b91d8ead7f10b84
SRPM: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/epel-7-x86_64/00560685-camotics/camotics-1.1.1-5.el7.centos.src.rpm

camotics-1.1.1-5


Changes from 1.1.1-3:

* Use system dxflib (depends on bug 1457949)
* Boost is bundled (change in upstream)
* Unbundled re2
* Build using c++11 compiler standard for re2
* Pass the option to build system to exclude bzip2, expat, re2, sqlite3 and zlib
* Corrected License tag according to bundled libraries
* Reused common compiler flags and build options from global variables
* Included AppStream Metadata
* Added missing bzip2-devel build requirement

Comment 10 Lubomir Rintel 2017-07-04 06:54:45 UTC
Thanks. This looks cool now!

APPROVED

I've also sponsored you into the packager group. You should have gotten the welcome e-mail by now. You can go ahead and request a repository for the package in pkgdb, import and build it. Thank you!

Comment 11 Gwyn Ciesla 2017-07-05 11:03:06 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/camotics

Comment 12 Fedora Update System 2017-07-06 18:25:44 UTC
camotics-1.1.1-7.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-045ae0bbc9

Comment 13 Fedora Update System 2017-07-06 18:28:53 UTC
camotics-1.1.1-6.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-8e5487b2e9

Comment 14 Fedora Update System 2017-07-07 07:49:22 UTC
camotics-1.1.1-6.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-8e5487b2e9

Comment 15 Fedora Update System 2017-07-07 09:05:45 UTC
camotics-1.1.1-7.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-b10e857c05

Comment 16 Fedora Update System 2017-07-07 09:05:52 UTC
camotics-1.1.1-7.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-25c9eb1f73

Comment 17 Fedora Update System 2017-07-09 02:52:38 UTC
camotics-1.1.1-7.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-045ae0bbc9

Comment 18 Fedora Update System 2017-07-15 18:49:48 UTC
camotics-1.1.1-7.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 19 Fedora Update System 2017-07-15 19:53:06 UTC
camotics-1.1.1-7.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2017-07-17 04:51:19 UTC
camotics-1.1.1-7.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 21 Fedora Update System 2017-07-25 05:48:42 UTC
camotics-1.1.1-6.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.