Bug 691597

Summary: Review Request: libopkele - A C++ implementation of the OpenID decentralized identity system
Product: [Fedora] Fedora Reporter: Bryan O'Sullivan <bos>
Component: Package ReviewAssignee: Michel Alexandre Salim <michel>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, hjia, michel, notting
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: 2012-05-04 11:44:01 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449, 691602    

Description Bryan O'Sullivan 2011-03-28 18:53:09 EDT
Spec URL: http://www.serpentine.com/bos/files/libopkele.spec
SRPM URL: http://www.serpentine.com/bos/files/libopkele-2.0.4-1.src.rpm
Description:

libopkele is a C++ implementation of the OpenID decentralized identity
system. It provides OpenID protocol handling, leaving authentication
and user interaction to the implementor.

This package is needed by mod_auth_openid, which I'll be packaging separately.
Comment 1 Hushan Jia 2011-03-29 01:13:18 EDT
Hi Bryan,

The requested URL /bos/files/libopkele-2.0.4-1.src.rpm was not found on this server.
Comment 2 Hushan Jia 2011-03-29 01:16:03 EDT
$ rpmlint libopkele.spec 
libopkele.spec:2: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 2)
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Please change spaces in line 1 to tab as well?
Comment 3 Bryan O'Sullivan 2011-03-29 13:15:13 EDT
Hi, Hushan -

Sorry about that, I got the file name wrong for the SRPM. Here are the corrected links to the spec file with no tabs, and the SRPM built from it:

Spec URL: http://www.serpentine.com/bos/files/libopkele.spec
SRPM URL: http://www.serpentine.com/bos/files/libopkele-2.0.4-1.fc14.src.rpm
Comment 4 Hushan Jia 2011-03-30 05:11:27 EDT
I will do an informal review. thanks.
Comment 5 Hushan Jia 2011-03-31 02:09:33 EDT
Informal review :)

[ok] MUST: rpmlint must be run on the source rpm and all binary rpms the build produces.

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

[ok] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec

[ok] MUST: The package must meet the Packaging Guidelines

[?] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[?] MUST: The License field in the package spec file must match the actual license.
The license of aux.d/ltmain.sh is GPLv2+, COPYING is MIT license, so the license would be MIT and GPLv2+?

[ok] 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]

[?] MUST: The spec file must be written in American English.
$ rpmlint libopkele.spec libopkele-2.0.4-1.fc14.src.rpm
libopkele.src: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
1 packages and 1 specfiles checked; 0 errors, 1 warnings.

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

[ok] 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.

[ok] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2962667

[ok] 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.

[ok] 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.

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

[ok] 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.

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

[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.

[ok] 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.

[ok] 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)

[ok] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.

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

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

[N/A] 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).

[ok] 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.

[ok] MUST: Header files must be in a -devel package.

[N/A] MUST: Static libraries must be in a -static package.

[ok] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.

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

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

[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.

[ok] 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.

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


Summary:
-----
1. rpmlint complains about implementor, which is alternative of implementer.
2. There is AUTHOR file, I think this should be packaged too, with %doc directive.
3. The license of this project looks like a mixture of MIT and GPLv2+, the license tag of spec is MIT.
Comment 6 Michel Alexandre Salim 2011-04-01 12:01:08 EDT
Taking the review
Comment 7 Michel Alexandre Salim 2011-04-01 12:29:07 EDT
Hushan -- FYI, the GPLv2+ parts are just GNU autotools, they are only used during the build process. But it's a good habit to be careful with licenses when reviewing packages!

Brian --
  apart from AUTHORS not being packaged, everything else looks fine. There are
  some declarations that are redundant unless you want to target EL5 or EL6, see
  complete reviews below.

  Feel free to include AUTHORS when updating the package before you build it,
  I wouldn't hold the review up for it. APPROVED.

* TODO Review [90%]
  - [X] Names [2/2]
    - [X] Package name
    - [X] Spec name
  - [X] Package version [2/2]
	http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Versioning
    - [X] Version number
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Version_Tag
    - [X] Release tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Release_Tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
  - [X] Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
  - [X] Source files match upstream
	$ sha1sum libopkele-2.0.4.tar.bz2 ../SOURCES/libopkele-2.0.4.tar.bz2 
        0c403d118efe6b4ee4830914448078c0ee967757  libopkele-2.0.4.tar.bz2
        0c403d118efe6b4ee4830914448078c0ee967757  ../SOURCES/libopkele-2.0.4.tar.bz2
  - [X] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No bundled libraries]]
  - [X] License [4/4]
    - [X] License is Fedora-approved
    - [X] No licensing conflict
    - [X] License field accurate
    - [X] License included iff packaged by upstream
  - [X] rpmlint [2/2]
    - [X] on src.rpm
      libopkele.src: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
      => rpmlint needs a better dictionary
      1 packages and 0 specfiles checked; 0 errors, 1 warnings.
    - [X] on x86_64.rpm
          libopkele.x86_64: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
          libopkele-devel.x86_64: W: no-documentation
          3 packages and 0 specfiles checked; 0 errors, 2 warnings.

          additional warnings on on installed packages; should be harmless.
            the sources do include the relevant headers, so I'm not sure if
            the warnings are accurate.
          libopkele.x86_64: W: unused-direct-shlib-dependency
            /usr/lib64/libopkele.so.3.0.0 /usr/lib64/libssl.so.10
          libopkele.x86_64: W: unused-direct-shlib-dependency
            /usr/lib64/libopkele.so.3.0.0 /lib64/libdl.so.2
          libopkele.x86_64: W: unused-direct-shlib-dependency
            /usr/lib64/libopkele.so.3.0.0 /lib64/libz.so.1
          libopkele.x86_64: W: unused-direct-shlib-dependency
            /usr/lib64/libopkele.so.3.0.0 /lib64/libm.so.6
          
  - [X] Language & locale [3/3]
    - [X] Spec in US English
    - [X] Spec legible
    - [X] Use %find_lang to handle locale files
	  N/A
  - [X] Build [3/3]
    - [X] Koji results
          http://koji.fedoraproject.org/koji/taskinfo?taskID=2962668
    - [X] BRs complete
	  There's an optional dependency on Konforka, developed by the
	  same folks, but we don't have it yet. Might be worth packaging
	  next?
    - [X] Directory ownership
  - [-] Spec inspection [9/10]
    - [X] ldconfig for libraries
    - [X] No duplicate files
    - [X] File permissions
    - [X] Filenames must be UTF-8
    - [X] no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting RHEL5]])
	  You don't need BuildRoot anymore, but it's fine to have it.
    - [X] [RHEL]  %clean section
          https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)
	  Likewise; unnecessary unless targeting EL5 and EL6
    - [X] [RHEL 5] %buildroot cleaned on %install
	  ... and this
    - [X] Macro usage consistent
    - [-] Documentation [2/3]
      - [X] If large docs, separate -doc
	    N/A
      - [X] %doc files are non-essential
      - [-] Relevant documentations included
	    AUTHORS file should probably be packaged
    - [X] Development [5/5]
      - [X] Headers in -devel
      - [X] If versioned .so's, unversioned in -devel
      - [X] Static only if necessary, put in -static
	    N/A
      - [X] -devel, -static requires main
      - [X] No .la
Comment 8 Michel Alexandre Salim 2012-02-02 09:35:13 EST
Er, just noticed that this bug has been sitting here for 9 months -- Hushan, are you still interested in maintaining the package? You forgot to ask for the package to be added to our Git repositories (by setting fedora-cvs to ? -- see the procedures)
Comment 9 Michel Alexandre Salim 2012-05-04 11:44:01 EDT
Closing this due to lack of response. If Hushan or someone else want to continue the review, please open a new ticket and mark this as a duplicate; the package will need to be re-reviewed (but the review can be based on this).
Comment 10 Kevin Fenzi 2012-05-25 13:47:49 EDT

*** This bug has been marked as a duplicate of bug 825333 ***