Bug 825333 - Review Request: libopkele - A C++ implementation of the OpenID decentralized identity system
Review Request: libopkele - A C++ implementation of the OpenID decentralized ...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrick Uiterwijk
Fedora Extras Quality Assurance
: Reopened
: 691597 (view as bug list)
Depends On:
Blocks: 965443
  Show dependency treegraph
 
Reported: 2012-05-25 13:46 EDT by Kevin Fenzi
Modified: 2013-06-04 20:48 EDT (History)
5 users (show)

See Also:
Fixed In Version: libopkele-2.0.4-4.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-06-04 20:48:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
puiterwijk: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kevin Fenzi 2012-05-25 13:46:57 EDT
Spec URL: http://www.scrye.com/~kevin/fedora/review/libopkele.spec
SRPM URL: http://www.scrye.com/~kevin/fedora/review/libopkele-2.0.4-2.fc18.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.

Fedora Account System Username: kevin

Scratch build at: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=4101112

rpmlint says: 


libopkele.i686: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
libopkele.src: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
libopkele.x86_64: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
libopkele-devel.i686: W: no-documentation
libopkele-devel.x86_64: W: no-documentation

which can all be ignored.
Comment 1 Kevin Fenzi 2012-05-25 13:47:49 EDT
*** Bug 691597 has been marked as a duplicate of this bug. ***
Comment 2 Michael Schwendt 2012-05-30 05:55:03 EDT
> Requires:       %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> Summary:        A C++ implementation of the OpenID decentralized identity system

Some day we will have eliminated all remaining leading articles from package summaries. ;)
Sure, it's a matter of taste, but summaries are even more concise, if they don't start with superfluous "A", "An, "The".


> http://koji.fedoraproject.org/koji/getfile?taskID=4101114&name=build.log
> ...
> checking for UUID... no

Smells like "BuildRequires: libuuid-devel" is missing.


> $ cat libopkele.pc 
> ...
> Requires: openssl
> Libs: -L${libdir} -lopkele -lcurl   -lexpat -ltidy

These explicitly linked libs deserve a closer look. First of all, they mean that libopkele-devel is missing "Requires: libcurl-devel libdidy-devel expat-devel" because no automatic pkg-config dep is present. Secondly, only "curl" headers are included from within libopkele headers, so there is no need to (re)link with the other two libs.


> libopkele-2.0.4/test/

The "test" executable that is built by default looks like it could be executed in a %check section.
Comment 3 Michael Schwendt 2012-05-30 06:27:47 EDT
> libuuid-devel

This looks trouble-some with regard to reproducible builds. It happily detects and uses libuuid-devel, if installed, for some of the test files, but it also includes a "uuid" inter-package dependency in the .pc file.
Comment 4 Kevin Fenzi 2012-06-03 18:20:53 EDT
Updated to address (most) issues noted. 

Also filed: https://github.com/hacker/libopkele/issues/2 about the pc file issues. 

Spec URL: http://www.scrye.com/~kevin/fedora/review/libopkele.spec
SRPM URL: http://www.scrye.com/~kevin/fedora/review/libopkele-2.0.4-3.fc18.src.rpm
Comment 5 Kevin Fenzi 2012-06-08 14:56:50 EDT
Hum, I'm a bit confused here now about: 

"Secondly, only "curl" headers are included from within libopkele headers, so there is no need to (re)link with the other two libs."

I clearly see expat and tidy includes... in include/opkele/tidy.h and include/opkele/expat.h 

Am I missing something here?
Comment 6 Michael Schwendt 2012-06-23 16:11:43 EDT
$ rpmdev-extract libopkele-devel-2.0.4-2.fc17.x86_64.rpm 
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/acconfig.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/association.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/ax.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/basic_op.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/basic_rp.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/exception.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/extension.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/extension_chain.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/iterator.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/oauth_ext.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/opkele-config.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/prequeue_rp.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/sreg.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/tr1-mem.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/types.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/uris.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/util.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele/verify_op.h
libopkele-devel-2.0.4-2.fc17.x86_64/usr/lib64/libopkele.so
libopkele-devel-2.0.4-2.fc17.x86_64/usr/lib64/pkgconfig/libopkele.pc

No such header files as you mention. Closer look:

$ cd libopkele-devel-2.0.4-2.fc17.x86_64/usr/include/opkele
$ grep expat * -R
$ grep tidy * -R
exception.h:     * htmltidy related error occured
exception.h:    class exception_tidy : public exception {
exception.h:	    exception_tidy(OPKELE_E_PARS);
exception.h:	    exception_tidy(OPKELE_E_PARS,int r);
exception.h:	    ~exception_tidy() throw() { }
$ grep buffio * -R
$


$ cd libopkele-2.0.4
$ find|grep tidy
./libopkele-2.0.4/include/opkele/tidy.h
$ find|grep expat
./libopkele-2.0.4/lib/expat.cc
./libopkele-2.0.4/include/opkele/expat.h
$

They are conditional. Will be back after a resubmitted build job to get a chance to check the build log as why these have not been installed in my test-build on May 30th.
Comment 7 Michael Schwendt 2012-06-23 16:14:11 EDT
Or did you peek at the source tarball instead of the libopkele-devel rpm?
Comment 8 Michael Schwendt 2012-06-23 16:21:40 EDT
For both Rawhide and F17, the two mentioned headers are not installed, so bottom of comment 2 is true:


checking expat.h usability... yes
checking expat.h presence... yes
checking for expat.h... yes
checking for XML_ParserCreate in -lexpat... yes
checking tidy.h usability... yes
checking tidy.h presence... yes
checking for tidy.h... yes
checking for tidyParseBuffer in -ltidy... yes
checking tidy/tidy.h usability... no
checking tidy/tidy.h presence... no
checking for tidy/tidy.h... no

$ rpm -qpl libopkele-devel-2.0.4-2.fc18.x86_64.rpm 
/usr/include/opkele
/usr/include/opkele/acconfig.h
/usr/include/opkele/association.h
/usr/include/opkele/ax.h
/usr/include/opkele/basic_op.h
/usr/include/opkele/basic_rp.h
/usr/include/opkele/exception.h
/usr/include/opkele/extension.h
/usr/include/opkele/extension_chain.h
/usr/include/opkele/iterator.h
/usr/include/opkele/oauth_ext.h
/usr/include/opkele/opkele-config.h
/usr/include/opkele/prequeue_rp.h
/usr/include/opkele/sreg.h
/usr/include/opkele/tr1-mem.h
/usr/include/opkele/types.h
/usr/include/opkele/uris.h
/usr/include/opkele/util.h
/usr/include/opkele/verify_op.h
/usr/lib64/libopkele.so
/usr/lib64/pkgconfig/libopkele.pc
Comment 9 Kevin Fenzi 2012-08-12 18:00:25 EDT
The reason I wanted this in was for mod_auth_openid... but now I no longer have a need for that, so I think I will just close this request out. 

If someone would like to work on it, feel free to use my spec/work on the issues raised here. 

Thanks,
Comment 10 Kevin Fenzi 2013-05-20 18:40:41 EDT
ok. Seems I have a need for this again, so re-opening. 

Any new comments on this welcome. 

srpm and spec still at the same url.
Comment 11 Patrick Uiterwijk 2013-05-20 18:46:11 EDT
I will take this review.
Comment 12 Patrick Uiterwijk 2013-05-20 19:05:36 EDT
Regarding those two headers: that appears to be exactly what upsteam wants, as in the include/Makefile.am they are both in noinst_HEADERS.
Comment 13 Patrick Uiterwijk 2013-05-20 19:13:49 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License MIT
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
Provided: 47a7efbdd2c9caaaa8e4360eb2beea21
Upstream: 47a7efbdd2c9caaaa8e4360eb2beea21

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package compiles and builds on at least one arch. 
OK, See below - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
OK, See below - No rpmlint output. 
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have dist tag 
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 
1. Why both 
- %dir %{_includedir}/opkele
and
- %{_includedir}/opkele/*.h
and not just
%{_includedir}/opkele ?


2. Rpmlint says:
libopkele.x86_64: W: spelling-error %description -l en_US implementor -> implementer, implement or, implement-or
libopkele-devel.x86_64: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

The spelling error can be ignored, no-doc makes sense as the documentation is in the main package.



Please look at my first issue, but this package is

APPROVED
Comment 14 Kevin Fenzi 2013-05-20 19:21:17 EDT
Thanks!

happy to change that item in issue 1, I don't think it matters much either way. 

New Package SCM Request
=======================
Package Name: libopkele
Short Description: C++ implementation of the OpenID decentralized identity system
Owners: kevin puiterwijk
Branches: devel el6
InitialCC:
Comment 15 Jon Ciesla 2013-05-20 19:27:33 EDT
Git done (by process-git-requests).
Comment 16 Fedora Update System 2013-05-20 19:56:06 EDT
libopkele-2.0.4-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/libopkele-2.0.4-4.el6
Comment 17 Fedora Update System 2013-05-20 21:45:34 EDT
libopkele-2.0.4-4.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 18 Fedora Update System 2013-06-04 20:48:00 EDT
libopkele-2.0.4-4.el6 has been pushed to the Fedora EPEL 6 stable repository.

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