Bug 554647

Summary: Review Request: wbfs-manager - Manager for Nintendo RAW File System
Product: [Fedora] Fedora Reporter: Marco Mornati <ilmorna>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, notting, susi.lehtola, tcallawa
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-06-22 11:27:02 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 182235, 201449    

Description Marco Mornati 2010-01-12 04:14:31 EST
Spec URL: http://mmornati.fedorapeople.org/wbfs-manager/wbfs-manager.spec
SRPM URL: http://mmornati.fedorapeople.org/wbfs-manager/wbfs-manager-0.1.11-1.fc12.src.rpm
Description: WBFS management is a GTK manager to copy file into Nintendo File System. The official project page is locate to: http://code.google.com/p/linux-wbfs-manager/
Comment 1 Jason Tibbitts 2010-01-12 12:59:47 EST
*** Bug 554659 has been marked as a duplicate of this bug. ***
Comment 2 Fabian Affolter 2010-01-13 05:32:08 EST
Just some comments after a quick look at your spec file.

- The summary needs some love
  https://fedoraproject.org/wiki/Examples_of_good_package_summaries
- Lines in the %description seems to be to long
- Remove %pre, %post, and %postun they are useless
- gcc is not needed
  https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
- All GUI applications need a .desktop file
  https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
- Please preserve the time stamps
  https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
- Isn't parallel building working?
  https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make
Comment 3 Marco Mornati 2010-01-13 07:19:03 EST
Thanks a lot for your suggestions Fabian.

I've uploaded the new spec file (http://mmornati.fedorapeople.org/wbfs-manager/wbfs-manager.spec) following your comments.

I'm not sure where can I add the png icon to use with .desktop file. Is it correct to add a resources as a "SourceX" in the spec file? (The .destop is created directly inside spec)

Thanks a lot

Marco
Comment 4 Eric Smith 2010-01-27 20:17:50 EST
I've just been through learning where to put icons for one of my own packages.  Apparently the preferred location is /usr/share/pixmaps.

I can only offer an informal review, someone else will have to do an official one.

I'm not sure whether removing the "linux-" prefix from the upstream naming is a good idea.  I can see why it would be desirable, and I'd personally prefer it, but I can't find anything in the Fedora naming guidelines to support it:  

    "When naming a package, the name should match the upstream tarball or
    project name from which this software came."
    http://fedoraproject.org/wiki/Packaging:NamingGuidelines#General_Naming

Becuase of this, the Source0 tag and setup macro aren't working as written.  The Source0 tag should apparently use "linux-%{name}-%{version}.tar.gz", and the corresponding setup macro should be "%setup -q -n linux-%{name}" (no -%{version} because the tarball doesn't use that in the path).

rpmlint reports:
rpmlint wbfs-manager.spec ../RPMS/x86_64/wbfs-manager-*
wbfs-manager.spec:65: W: macro-in-%changelog %pre
wbfs-manager.x86_64: W: no-documentation
wbfs-manager.x86_64: E: script-without-shebang /usr/share/applications/wbfsmanager.desktop
wbfs-manager-debuginfo.x86_64: E: empty-debuginfo-package

defattr should be "%defattr(-,root,root,-)"; if you need 0755 permissions on some files, you should arrange for that in the build or install sections.  That should fix the "script-without-shebang" error.  Should probably use install rather than cp to install the executable, e.g.
    install -D -p -m 755 lmdemo %{buildroot}%{_bindir}/lmdemo

The make invocation should include CFLAGS="%{optflags}".  I'd thought that might fix the empty-debuginfo-package warning, but it doesn't, possibly because the Makefile isn't using CFLAGS for the link.

MUST: The package does not yet exist in Fedora. The Review Request is not a
duplicate. OK
MUST: The spec file for the package is legible and macros are used
consistently. NEEDSWORK -- spec uses both $RPM_BUILD_ROOT and %{buildroot}
MUST: The package must be named according to the  Package Naming Guidelines.  probably OK? see what an experienced reviewer says
MUST: The package must be licensed with a Fedora approved license and meet the 
Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license.
OK
MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. NEEDSWORK - just fix the Source0 tag as described above
MUST: The package MUST successfully compile and build into binary rpms. NEEDSWORK - just fix the setup macro as described above
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. NEEDSWORK
MUST: Packages containing shared library files must call ldconfig. N/A

MUST: A package must own all directories that it creates or require the package
that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. NEEDSWORK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect
runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files
ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. NEEDSWORK add icon as described above, and need to run desktop-file-install or desktop-file-validate
    http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from
upstream, the packager should query upstream to include it. NEEDSWORK
SHOULD: The package builds in mock. NEEDSWORK - if the Source0 tag and setup macro are fixes as described above, that will probably take care of mock.
Comment 5 Susi Lehtola 2010-01-28 03:21:57 EST
Assigning.
Comment 6 Susi Lehtola 2010-01-28 03:44:46 EST
MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- Create desktop file in %prep, not in %install.
- IMHO too much empty lines within sections. 
- Use of macros for standard commands is OK, although unnecessary.

MUST: The package must be named according to the Package Naming Guidelines. NEEDSWORK
- The naming guidelines clearly indicate that the package name should be linux-wbfs-manager.
 http://fedoraproject.org/wiki/Packaging/NamingGuidelines

MUST: The spec file name must match the base package %{name}. OK
- For now.

MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- Not all files are GPLv2:

/* Rijndael Block Cipher - rijndael.c

   Written by Mike Scott 21st April 1999
   mike@compapp.dcu.ie

   Permission for free direct or derivative use is granted subject 
   to compliance with any conditions that the originators of the 
   algorithm place on its exploitation.  

*/

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSWORK
- URL not functioning.

MUST: The package MUST successfully compile and build into binary rpms. NEEDSWORK
- Package does not compile.

MUST: The spec file MUST handle locales properly. N/A
 
MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Optimization flags are not used. Using
 make CFLAGS="%{optflags}" %{?_smp_mflags}
does the trick.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. N/A

MUST: Files only listed once in %files listings. ~OK
- Remove the empty %dir line.

MUST: Debuginfo package is complete. NEEDSWORK
- Debuginfo is empty.

MUST: Permissions on files must be set properly. NEEDSWORK
- Use %defattr(-,root,root,-).

MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Add README to %doc.

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. NEEDSWORK
- Use desktop-file-install to install the desktop file.

MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- License not included in tarball.

SHOULD: The package builds in mock. NEEDSWORK
- Does not build at all, when it is fixed it builds in mock.
Comment 7 Susi Lehtola 2010-01-28 03:47:12 EST
Oh, and as Eric pointed out you are also mixing macros, which is not allowed.
Comment 8 Susi Lehtola 2010-01-28 03:49:09 EST
As was quite clear from the quality of the spec file, Marco is not sponsored and is probably looking for one. Added FE-NEEDSPONSOR.
Comment 9 Susi Lehtola 2010-01-28 03:50:39 EST
More notes:

GenericName=WBFS Manager
GenericName[it]=WBFS Manager

is redundant, drop the Italian name.

**

Drop the .png suffix in

Icon=wbfs_gtk.png

the system will automatically determine the best file to use.
Comment 10 Tom "spot" Callaway 2010-01-28 11:03:47 EST
The rijndael.c code license is non-free because it does not give permission to freely redistribute. We either need to get that permission from the copyright holder, or we need to replace that code with something under a Free and GPLv2 compatible license. There is a list of known implementations here:

http://en.wikipedia.org/wiki/AES_implementations
Comment 11 Susi Lehtola 2010-06-22 11:27:02 EDT
No activity in 5 months, closing review.

Removing FE-LEGAL and FE-NEEDSPONSOR, adding FE-DEADREVIEW.