Bug 3095

Summary: Review request: netfabb-basic - Freeware suite for STL editing
Product: Package Reviews Reporter: Miro Hrončok <mhroncok>
Component: Review RequestAssignee: Nicolas Chauvet <kwizart>
Status: RESOLVED FIXED    
Severity: normal CC: hobbes1069, leamas.alec, mike, oget.fedora, rpmfusion-package-review
Priority: P5    
Version: Current   
Hardware: All   
OS: GNU/Linux   
namespace:
Bug Depends on:    
Bug Blocks: 4    

Description Miro Hrončok 2014-01-01 16:56:32 CET
SPEC: https://raw.github.com/hroncok/SPECS/master/netfabb-basic.spec
SRPM: http://churchyard.fedorapeople.org/SRPMS/netfabb-basic-5.0.1-1.fc20.src.rpm

netfabb Basic is a free (as in free beer) software for 3D Printing
and the STL file format. Numerous tools allow all steps of the fabrication
process: editing, repairing, positioning, slicing and exporting triangulated
CAD data. For professional use, the author offers commercial support and
additional modules.

This does obviously not go into Fedora, as it is nonfree. It goes to nonfree section.



$ rpmlint ../SRPMS/netfabb-basic-5.0.1-1.fc20.src.rpm 
netfabb-basic.src: W: invalid-license Redistributable

Not quite sure what license to use here, so if this is not OK, just give me the hint.

netfabb-basic.src: W: invalid-url Source1: netfabb-basic_5.0.1_linux64.tar.gz
netfabb-basic.src: W: invalid-url Source0: netfabb-basic_5.0.1_linux32.tar.gz

Sources has no public URL, commented in the spec.

1 packages and 0 specfiles checked; 0 errors, 3 warnings.



$ rpmlint ../RPMS/x86_64/netfabb-basic-5.0.1-1.fc20.x86_64.rpm 
netfabb-basic.x86_64: W: invalid-license Redistributable

Already addressed.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 1 Miro Hrončok 2014-01-08 14:08:55 CET
bumping this (for the e-mail issue, so it it populated on the mailinglist)
Comment 2 Richard 2014-01-08 15:14:28 CET
Redistributable seems to be correct based on the provided LICENSE file. It looks like it bundles some sort of unzip library. I'm not sure how to check to see if this is the same as the fedora library, if so, it would require a bundling exception (to my knowledge the non-free repository is not exempt from this).

Quick spec review:

1. All the echos for the desktop file is messy. Use this method instead:
cat >%{buildroot}%{_datadir}/applications/%{name}.desktop <<EOL
line 1
line 2 
line 3
line 4
... 
EOL

2. Use install instead of cp to install the binary and libraries so you don't have to fix it in %files:
install -pm 0755 netfabb %{buildroot}%{_bindir}/%{name}
install -pm 0755 *.so.* %{buildroot}%{_libdir}

I'm not sure if install works with wildcards but worth a shot. Worst case you do them individually.

3. %check is for any unit tests that the project can run against itself to make sure it functions, desktop-file-validate belongs in %install.
Comment 3 Alec Leamas 2014-01-08 15:22:44 CET
You need to fix the iconcache snippets:

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

HTH --alec
Comment 4 Miro Hrončok 2014-01-08 16:10:17 CET
0. (unzip) This might be something completely different as far as I know and if it would have been staticly compiled inside the binary, we wouldn't even know about it. So I guess this cannot be considered as bundling.

$ readelf -Ws libunzip-netfabb.so.0 | awk '{print $8}';


Name


__gmon_start__
_Jv_RegisterClasses
malloc@GLIBC_2.2.5
free@GLIBC_2.2.5
strlen@GLIBC_2.2.5
__cxa_finalize@GLIBC_2.2.5
memcpy@GLIBC_2.2.5
strcmp@GLIBC_2.2.5
memstreambuffer_getfirst
memstreambuffer_addstream
memstream_getdata
_end
_edata
memstreambuffer_unzip
memstream_getnext
memstream_getname
memstream_getsize
__bss_start
_init
_fini
memstreambuffer_init
memstreambuffer_free


We have no unizp library I'd know about and libzip and libz symbols are completely different.

1. This is copied from their install script and I would like to keep it in this way, for easier update later.

2. Will check.

3. https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

4. (icon cache) Will do.
Comment 5 Miro Hrončok 2014-01-08 22:05:02 CET
* Wed Jan 08 2014 Miro Hrončok <mhroncok@redhat.com> - 5.0.1-2
- Use install to copy files that needs different rights
- Rework icon cache scriptlets

SPEC: https://raw.github.com/hroncok/SPECS/master/netfabb-basic.spec
SRPM:
http://churchyard.fedorapeople.org/SRPMS/netfabb-basic-5.0.1-2.fc20.src.rpm
Comment 6 Miro Hrončok 2014-01-18 15:24:23 CET
Anything?
Comment 7 Miro Hrončok 2014-03-19 13:02:28 CET
SPEC: https://raw.github.com/hroncok/SPECS/master/netfabb-basic.spec
SRPM:
http://churchyard.fedorapeople.org/SRPMS/netfabb-basic-5.1.0-1.fc20.src.rpm

* Wed Mar 19 2014 Miro Hrončok <mhroncok@redhat.com> - 5.1.0-1
- Updated to 5.1.0
Comment 9 Orcan Ogetbil 2014-04-09 02:47:55 CEST
In the world of programming STL has a very well known meaning. I think any non-standard use of STL must be defined in %description. My 2 cents...
Comment 10 Miro Hrončok 2014-04-09 08:07:19 CEST
Well, "STL file format" is clear enough.

http://en.wikipedia.org/wiki/STL_(file_format)

But I can reword it to "STL (STereoLithography) file format" if you think that's more clear.

Could you please do the review?
Comment 11 Miro Hrončok 2014-07-10 16:13:40 CEST
bump? Please?
Comment 12 Michael Cronenworth 2014-07-10 17:35:09 CEST
I'll take review.

Regarding the bundled libs: The included libunzip is not the same as Info-ZIP libunzip and I do not know what libxiot is so I think they are OK. The problem would be with lib3ds. It should be able to work with the one in Fedora so could you try unbundling that one?
Comment 13 Michael Cronenworth 2014-07-10 17:48:21 CEST
Also, since MimeType is being used in the Desktop file you need to add a update-desktop-database call in %post and %postun.

%post
update-desktop-database &>/dev/null || :

%postun
update-desktop-database &>/dev/null || :
Comment 15 Michael Cronenworth 2014-07-10 19:20:21 CEST
Package APPROVED. Good work!


Package Review
==============

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



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

C/C++:
[-]: Package does not contain kernel modules.
[-]: Package contains no static executables.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[-]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[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". 3 files have unknown license. Detailed output of
     licensecheck in /home/mcronenworth/Downloads/netfabb-
     basic/licensecheck.txt
[-]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[-]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[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]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: update-desktop-database is invoked in %post and %postun if package
     contains desktop file(s) with a MimeType: entry.
     Note: desktop file(s) with MimeType entry in netfabb-basic
[x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in netfabb-basic
[-]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 563200 bytes in 9 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[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]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[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 or desktop-
     file-validate if there is such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

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

Generic:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: 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]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[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)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------
Checking: netfabb-basic-5.1.1-2.fc20.x86_64.rpm
          netfabb-basic-5.1.1-2.fc20.src.rpm
netfabb-basic.x86_64: W: invalid-license Redistributable
netfabb-basic.x86_64: W: dangling-relative-symlink /usr/lib64/lib3ds-netfabb-1.so.3 lib3ds-1.so.3
netfabb-basic.src: W: invalid-license Redistributable
netfabb-basic.src: W: invalid-url Source1: netfabb-basic_5.1.1_linux64.tar.gz
netfabb-basic.src: W: invalid-url Source0: netfabb-basic_5.1.1_linux32.tar.gz
2 packages and 0 specfiles checked; 0 errors, 5 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint netfabb-basic
netfabb-basic.x86_64: W: invalid-license Redistributable
netfabb-basic.x86_64: W: dangling-relative-symlink /usr/lib64/lib3ds-netfabb-1.so.3 lib3ds-1.so.3
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
# echo 'rpmlint-done:'



Requires
--------
netfabb-basic (rpmlib, GLIBC filtered):
    /bin/sh
    lib3ds(x86-64)
    libGL.so.1()(64bit)
    libX11.so.6()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libdl.so.2()(64bit)
    libexpat.so.1()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgdk-x11-2.0.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgmodule-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgthread-2.0.so.0()(64bit)
    libgtk-x11-2.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libz.so.1()(64bit)



Provides
--------
netfabb-basic:
    application()
    application(netfabb-basic.desktop)
    libunzip-netfabb.so.0()(64bit)
    libxiot-netfabb.so.0()(64bit)
    mimehandler(application/netfabb)
    mimehandler(application/sla)
    mimehandler(application/x-3ds)
    mimehandler(image/x-3ds)
    mimehandler(model/mesh)
    mimehandler(model/x3d+binary)
    mimehandler(model/x3d+xml)
    netfabb-basic
    netfabb-basic(x86-64)
Comment 16 Miro Hrončok 2014-07-10 21:23:34 CEST
Package CVS request
======================
Package Name: netfabb-basic
Short Description: Freeware suite for STL editing
Owners: churchyard
Branches: f19 f20
----------------------
License tag: nonfree
Comment 17 Miro Hrončok 2014-07-10 21:25:09 CEST
And of course, thank you Michael.
Comment 18 Miro Hrončok 2014-07-11 00:14:35 CEST
Checking out module: 'netfabb-basic'
cvs server: cannot find module `netfabb-basic' - ignored
cvs [checkout aborted]: cannot expand modules
ERROR: "netfabb-basic" module does not exist in cvs.
Comment 19 Miro Hrončok 2014-07-11 17:57:28 CEST
Nicolas?
Comment 20 Nicolas Chauvet 2014-07-11 19:08:45 CEST
works for me.
Which command have you used to do the checkout ?
try with -d
Comment 21 Miro Hrončok 2014-07-11 19:43:16 CEST
[churchyard fedora-scm]$ cvs co common
cvs checkout: Updating common
U common/Makefile.common
U common/branches
U common/cvs-import.sh
[churchyard fedora-scm]$ cd common
[churchyard common]$ ./cvs-import.sh -b devel ../../SRPMS/netfabb-basic-5.1.1-2.fc20.src.rpm 
Checking out module: 'netfabb-basic'
cvs server: cannot find module `netfabb-basic' - ignored
cvs [checkout aborted]: cannot expand modules
ERROR: "netfabb-basic" module does not exist in cvs.


Where do I put -d?
Comment 22 Nicolas Chauvet 2014-07-11 20:13:41 CEST
(In reply to comment #21)
> [churchyard fedora-scm]$ cvs co common
Are you sure to have the nonfree version ?
Comment 23 Miro Hrončok 2014-07-11 20:55:34 CEST
Oh, that was it, sorry :(

Thanks