This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 787293 - Review Request: sparkleshare - Easy file sharing based on git repositories
Review Request: sparkleshare - Easy file sharing based on git repositories
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Pavel Alexeev
Fedora Extras Quality Assurance
:
: 629744 (view as bug list)
Depends On: 753364
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 15:34 EST by Nikos Roussos
Modified: 2012-03-01 20:16 EST (History)
8 users (show)

See Also:
Fixed In Version: sparkleshare-0.8.0-2.fc17
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-03-01 04:19:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pahan: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nikos Roussos 2012-02-03 15:34:57 EST
Spec URL: http://comzeradd.fedorapeople.org/specs/sparkleshare.spec
SRPM URL: http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/sparkleshare-0.8.0-1.fc16.src.rpm
Description: Easy file sharing based on git repositories. A special folder is setup and directories/files placed within are placed in a git-based version control system and synchronized elsewhere.

rpmlint sparkleshare.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint ../SRPMS/sparkleshare-0.8.0-1.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

These is my first mono package, so any help is much appreciated
Comment 1 Nikos Roussos 2012-02-03 15:35:47 EST
*** Bug 629744 has been marked as a duplicate of this bug. ***
Comment 2 Michel Alexandre Salim 2012-02-06 05:59:59 EST
Pavel's already volunteered to review, but I'm Cc:ing myself as I used to be quite active in Mono packaging, and now that the crazy libdir mess (we used to put Mono files in %{_libdir} rather than %{_prefix}/lib as upstream does and that breaks a lot) is sorted, it's starting to be an appealing platform again.
Comment 3 Ofer Schreiber 2012-02-06 12:20:17 EST
I'm not an official packager yet, but here's my review:

MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1] 

[oschreib@jerusalem spark]$ rpmlint sparkleshare-0.8.0-1.fc16.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[oschreib@jerusalem spark]$ rpmlint sparkleshare-0.8.0-1.fc16.x86_64.rpm 
sparkleshare.x86_64: E: no-binary
sparkleshare.x86_64: W: only-non-binary-in-usr-lib
sparkleshare.x86_64: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/NEWS
sparkleshare.x86_64: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/LICENSE
sparkleshare.x86_64: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/AUTHORS
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

1. Any reason why NEWS, LICENSE and AUTHORS are executable?

2. About the binary issues - sounds like rpmlint doesn't think .exe or .dll are binaries. 

MUST: The package must be named according to the Package Naming Guidelines .

PASS

MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2]

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

PASS

MUST: The License field in the package spec file must match the actual license. [3]

PASS

MUST: 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 must be included in %doc.[4]

PASS

MUST: The spec file must be written in American English. [5]

PASS

MUST: The spec file for the package MUST be legible. [6]

PASS

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

[oschreib@jerusalem spark]$ md5sum *tar.gz
e529adb83ae9ddba68c4b13c3823e2fc  sparkleshare-0.8.0.tar.gz

Verified.
PASS

MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]

PASS

MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]

N/A

MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.

PASS

MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]

N/A

MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]

Not sure about this one, you should consult with someone familar with mono packaging.


MUST: Packages must NOT bundle copies of system libraries.[11]

N/A

MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]

N/A

MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]

PASS

MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)[14]

PASS

MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. [15]

FAIL
-rwxr-xr-x. 1 root root  17733 Feb  6 18:05 /usr/lib64/sparkleshare/SparkleLib.dll.mdb
-rwxr-xr-x. 1 root root  46565 Feb  6 18:05 /usr/lib64/sparkleshare/SparkleShare.exe.mdb

.mdb shouldn't be executables.

MUST: Each package must consistently use macros. [16]

PASS

MUST: The package must contain code, or permissable content. [17]

PASS

MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]

N/A

MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]

N/A

MUST: Static libraries must be in a -static package. [19]

N/A

MUST: Development files must be in a -devel package. [20]

FAIL
Sounds to me like .mdb files (mentioned above) should be in a seperate package

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} [21]

N/A

MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[19]

N/A

MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [22]

PASS
desktop-file-install is not used here, but according to https://fedoraproject.org/wiki/Packaging/Guidelines#desktop desktop-file-validate is fine as well.


MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23]

PASS

MUST: All filenames in rpm packages must be valid UTF-8. [24]

PASS
Comment 4 Kyle J. Harms 2012-02-06 19:52:23 EST
This spec does not build the nautilus extensions for gnome 3, since it is the default desktop in f16. Which you'll need nautilus-python >= 1.1.

However, this will depend on this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=753364

Further, since this is building the nautilus 2 extensions you do need a Requires: nautilus-python
Comment 5 Nikos Roussos 2012-02-07 07:18:57 EST
Thanx for the tips.
I'll fix the NEWS, LICENSE and AUTHORS permission error. They seem to have executable permissions inside the tarball and I didn't realized it.

I'm not sure if *.mdb files should be in a seperate -devel package. But as I said I'm not a mono packaging expert, so if any has more experience could help would be nice.

It seems that nautilous-python should be upgraded to 1.1 for sparkleshare run properly.
Comment 6 Pavel Alexeev 2012-02-12 09:07:14 EST
Sorry for such delay.

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

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated


==== Generic ====
[!]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.

$ licensecheck -r * | fgrep -v 'GPL (v3 or later)'
ltmain.sh: GPL (v2 or later)
SparkleLib/AssemblyInfo.cs: *No copyright* UNKNOWN
SparkleShare/SparkleOptions.cs: MIT/X11 (BSD like)
SparkleShare/Mac/Growl.framework/Headers/GrowlDefines.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Headers/GrowlApplicationBridge.h: UNKNOWN
SparkleShare/Mac/Growl.framework/Headers/Growl.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Headers/GrowlApplicationBridge-Carbon.h: *No copyright* Public domain
SparkleShare/Mac/Growl.framework/Versions/A/Headers/GrowlDefines.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/A/Headers/GrowlApplicationBridge.h: UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/A/Headers/Growl.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/A/Headers/GrowlApplicationBridge-Carbon.h: *No copyright* Public domain
SparkleShare/Mac/Growl.framework/Versions/Current/Headers/GrowlDefines.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/Current/Headers/GrowlApplicationBridge.h: UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/Current/Headers/Growl.h: *No copyright* UNKNOWN
SparkleShare/Mac/Growl.framework/Versions/Current/Headers/GrowlApplicationBridge-Carbon.h: *No copyright* Public domain
SparkleShare/Mac/MainMenu.xib.designer.cs: *No copyright* GENERATED FILE
SparkleShare/SparkleSetup.cs: UNKNOWN
SparkleShare/SparkleEntry.cs: UNKNOWN

ltmain.sh is part of libtool and bundling does not allowed.

Do you known why these files have different license or does not licensed on anything manner?

[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[!]: MUST Buildroot is not present
     Note: Buildroot is not needed unless packager plans to package for EPEL5

As It is not build on EPEL-5 due to the missing ndesk-dbus-devel dependency (http://koji.fedoraproject.org/koji/taskinfo?taskID=3783802) I assume it is not targeted for EPEL-5. Please remove such legacy things or ensure it is built on EPEL-5.

[!]: MUST Package contains no bundled libraries.
See comment above about libtool. It must be removed in %prep.

[x]: MUST Changelog in prescribed format.
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
[!]: MUST Sources contain only permissible code or content.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files -f %{name}.lang section. This is OK
     if packaging for EPEL5. Otherwise not needed
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package contains a properly installed %{name}.desktop using desktop-
     file-install file if it is a GUI application.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[x]: MUST 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]: MUST License field in the package spec file matches the actual license.
[x]: MUST The spec file handles locales properly.

Guidelines say http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files what BuildRequires: gettext needed. But I'm doubt it is really needed.

[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).

For what you include:
%global debug_package %{nil}
?
I think it should be removed. Or just add comment about purposes.

You free to use or not macroses in URL, but please do not mix and chose one of:
Source0:        https://github.com/downloads/hbons/SparkleShare/%{name}-%{version}.tar.gz                                                          
Source0:        https://github.com/downloads/hbons/SparkleShare/sparkleshare-0.8.0.tar.gz                                                          

[x]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generates any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[!]: MUST Rpmlint output is silent.

rpmlint sparkleshare-0.8.0-1.fc18.src.rpm

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


rpmlint sparkleshare-0.8.0-1.fc18.i686.rpm

sparkleshare.i686: E: no-binary
sparkleshare.i686: W: only-non-binary-in-usr-lib
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/LICENSE
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/NEWS
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/AUTHORS
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

Permissions must be fixed.

[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/pasha/SOFT/Review/sparkleshare/787293/sparkleshare-0.8.0.tar.gz :
  MD5SUM this package     : e529adb83ae9ddba68c4b13c3823e2fc
  MD5SUM upstream package : e529adb83ae9ddba68c4b13c3823e2fc

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD 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]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[-]: SHOULD Package functions as described.
[+]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[-]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Buildroot is not present
     Note: Buildroot is not needed unless packager plans to package for EPEL5
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files -f %{name}.lang section. This is OK
     if packaging for EPEL5. Otherwise not needed
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[!]: MUST Rpmlint output is silent.

rpmlint sparkleshare-0.8.0-1.fc18.src.rpm

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


rpmlint sparkleshare-0.8.0-1.fc18.i686.rpm

sparkleshare.i686: E: no-binary
sparkleshare.i686: W: only-non-binary-in-usr-lib
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/LICENSE
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/NEWS
sparkleshare.i686: W: spurious-executable-perm /usr/share/doc/sparkleshare-0.8.0/AUTHORS
1 packages and 0 specfiles checked; 1 errors, 4 warnings.



Generated by fedora-review 0.1.2
External plugins:
Comment 7 Nikos Roussos 2012-02-14 08:05:42 EST
Hi Pavel,

New source files:
http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/sparkleshare-0.8.0-2.fc16.src.rpm
http://comzeradd.fedorapeople.org/specs/sparkleshare.spec

Some comments:

1.
I fixed permission problems

2.
> ltmain.sh is part of libtool and bundling does not allowed.
True but it's just part of the upstream source file. It's not included on the installed files, so it's not actually bundled. I don't know why they do this, but it's seems like a common practice (even firefox does that).

3. 
> For what you include:
> %global debug_package %{nil}
> ?
> I think it should be removed. Or just add comment about purposes.
Sorry for not adding a comment there. This is needed because of the empty debuginfo package. https://fedoraproject.org/wiki/Packaging:Mono#Empty_debuginfo
Comment 8 Pavel Alexeev 2012-02-20 04:12:53 EST
Hello, Nikos.

(In reply to comment #7)
> 1.
> I fixed permission problems
There still some permission warnings:
rpmlint sparkleshare-0.8.0-2.fc18.i686.rpm

sparkleshare.i686: E: no-binary
sparkleshare.i686: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Isn't %{_bindir}/%{name} should have executable permissions?
Comment 9 Nikos Roussos 2012-02-20 09:15:38 EST
Yeap, this is a typical warning on Mono package. If you install it you'll see that /usr/bin/sparkleshare is executable bit not binary because it's a bash script. Despite that Mono packages should not be noarch, thus the rpmlint message. You can safely ignore this.

See more here:
https://fedoraproject.org/wiki/Packaging:Mono#rpmlint_and_mono_packages
Comment 10 Nikos Roussos 2012-02-20 12:01:30 EST
btw, i still need to have nautilus-python-devel >=1.1 for buildrequires. i'll do it asap since nautilus-python-1.1 is already on updates-testing
https://admin.fedoraproject.org/updates/nautilus-python
Comment 11 Pavel Alexeev 2012-02-20 12:04:08 EST
Ok, then package APPROVED.
Comment 12 Nikos Roussos 2012-02-21 03:57:48 EST
New Package SCM Request
=======================
Package Name: sparkleshare
Short Description: Easy file sharing based on git repositories
Owners: comzeradd
Branches: f16 f17
InitialCC:
Comment 13 Jon Ciesla 2012-02-21 08:39:56 EST
Git done (by process-git-requests).
Comment 14 Fedora Update System 2012-02-21 10:47:20 EST
sparkleshare-0.8.0-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/sparkleshare-0.8.0-2.fc16
Comment 15 Fedora Update System 2012-02-21 11:05:53 EST
sparkleshare-0.8.0-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/sparkleshare-0.8.0-2.fc17
Comment 16 Fedora Update System 2012-02-21 12:45:03 EST
sparkleshare-0.8.0-2.fc17 has been pushed to the Fedora 17 testing repository.
Comment 17 Fedora Update System 2012-03-01 04:19:51 EST
sparkleshare-0.8.0-2.fc16 has been pushed to the Fedora 16 stable repository.
Comment 18 Fedora Update System 2012-03-01 20:16:17 EST
sparkleshare-0.8.0-2.fc17 has been pushed to the Fedora 17 stable repository.

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