Bug 557776

Summary: Review Request: libyubikey - Library for parsing / decrypting Yubikey passwords
Product: [Fedora] Fedora Reporter: Maxim Burgerhout <maxim>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, mads, mtasaka, notting, zbe64533
Target Milestone: ---Flags: mtasaka: fedora-review+
tcallawa: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.5-4.fc12 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-01-27 20:03:57 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 557794    

Description Maxim Burgerhout 2010-01-22 15:09:08 UTC
Spec URL: http://dl.dropbox.com/u/284093/libyubikey.spec
SRPM URL: http://dl.dropbox.com/u/284093/libyubikey-1.5-1.fc12.src.rpm
Description: library for the Yubico authentication device, the Yubikey

A Yubikey is a little gadget that generates one-time passwords. I am working on packaging ykpers too, which allows for customization of AES key inside the Yubikey. ykpers requires libyubikey.

This is not the first Yubikey software in Fedora. We already have pam_yubico (a PAM module for the device) and ykclient (a general CLI) in the repositories.

This is my first package for Fedora, so I will need a sponsor. I'm happy to answer any questions.

Comment 1 Akio Idehara 2010-01-24 03:44:07 UTC
Hello. Here are some note:

(1) Documentation
  - "rpmlint" says "libyubikey-devel.i686: W: no-documentation".
  - Pay attention about which subpackage you include documentation in,
    for example API documentation belongs in the -devel subpackage,
    not the main one.
    * http://fedoraproject.org/wiki/Packaging/Guidelines#Documentation

(2) Compiler flags
  - Compilers used to build packages should honor the applicable
    compiler flags set in the system rpm configuration.
    * http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
  - $RPM_OPT_FLAGS or %{optflags} for C, C++, and Fortran compilers.

(3) Timestamps
  - Please keep timestamps on installed files as much as possible:
    * http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
  - When using "cp" or "install" commands add "-p" option
  - Also please consider to add 'INSTALLFLAGS="-c -p"' to "make
    install"

(4) URL
  - The URL and the Source0 is old (redirected), please keep the
    latest URL.
     * "http://code.google.com/p/yubico-c/"

(5) binary files
  - If the software being packaged contains files intended solely for
    development, those files should be put in a -devel subpackage.
    * http://fedoraproject.org/wiki/Packaging/Guidelines#Devel_Packages
      o %{_bindir}/modhex
      o %{_bindir}/ykdebug

(6) pkgconfig file
  - This library does not have pkgconfig file(.pc), so "BuildRequires:
    pkgconfig" is needless.

Comment 2 Maxim Burgerhout 2010-01-24 14:08:41 UTC
Thanks for looking at this, Akio. I went over your notes:

1. Correct, but as modhex and ykdebug went to -devel, so did README (which is about those two files for the largest part). So -devel now has a %doc section.
2. Fixed.
3. Looked into this. I redid the download so it's timestamp matches upstream. However, the Makefile doesn't seem to handle INSTALLFLAGS or pass it to install-sh, but the timestamps of installed files are intact, afaics. I don't think the install part needs fixing. Please correct me if I'm wrong; I really want to learn this. 
4. Fixed.
5. Moved them to -devel. I agree that is a better place for those binaries.
6. Fixed. Missed that.

New versions at:
http://dl.dropbox.com/u/284093/update-1/libyubikey.spec
http://dl.dropbox.com/u/284093/update-1/libyubikey-1.5-3.fc12.src.rpm

Comment 3 Mamoru TASAKA 2010-01-24 19:15:18 UTC
Some notes for 1.5-3

* Compilar flags
  - "CFLAGS="$RPM_OPT_FLAGS"" in "make %{?_smp_mflags}" is not needed.

    configure generated by recent autotools acknowledges CFLAGS
    environment and %configure sets this.
    Please check what %configure actually does by
    $ rpm --eval %configure .

* Timestamp
  - Please consider to use
--------------------------------------------------------------------
%{__make} install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
--------------------------------------------------------------------
    to keep timestamps on installed files. This method usually
    works for Makefiles generated by recent autotools.

* %defattr
  - Now we prefer to use %defattr(-,root,root,-)

* File placement
  - As the Summary of the main package already says "low-level C software 
    *development* kit", I don't see strict necessity for putting
    two binaries into -devel package.

  - Also "README" should usually be packaged in main package.

* %changelog
  - It is recommended to put one line between each %changelog entry
    like below because it is useful on Fedora CVS
---------------------------------------------------------------------
* Sun Jan 24 2010 - Maxim Burgerhout <maxim> - 1.5-3
- Took out the dep on libusb1-devel
- Moved README doc to -devel: it's mostly about ykdebug and modhex

* Sun Jan 24 2010 - Maxim Burgerhout <maxim> - 1.5-2
- Used macros in Source0
- URL no longer point to redirect
- Removed INSTALL from documentation
- Moved modhex and ykdebug to -devel
- Inserted compilerflags 
- Inserted INSTALLFLAGS to keep timestamps
- Some more macros for make and sed

* Wed Jan 20 2010 - Maxim Burgerhout <maxim> - 1.5-1
- First packaged release
---------------------------------------------------------------------

Comment 4 Maxim Burgerhout 2010-01-24 20:51:05 UTC
Mamoru, thank you for looking into this package.

Timestamps: done.
Compilerflags: reverted.
Defattr: done.
Changelog: done.
File placement: reverted.

Updated files:
- http://dl.dropbox.com/u/284093/update-2/libyubikey.spec
- http://dl.dropbox.com/u/284093/update-2/libyubikey-1.5-4.fc12.src.rpm

Comment 5 Akio Idehara 2010-01-25 13:39:12 UTC
Sorry. The above review comment was my FIRST one and it introduced the confusion....

Mamoru-san:
  I have one question about rpmlint.
  It still says "libyubikey-devel.i686: W: no-documentation".
  Could we simply ignore this message?

Comment 6 Mamoru TASAKA 2010-01-25 18:07:30 UTC
(In reply to comment #5)
> Mamoru-san:
>   I have one question about rpmlint.
>   It still says "libyubikey-devel.i686: W: no-documentation".
>   Could we simply ignore this message?    

If the needed documents are already installed in one of the
rpms the rpm depens on, we ignore this warning.

Now
- This package can be approved.
- You have another review requests, which is in good shape to some
  extent.

------------------------------------------------------
   This package (libyubikey) is APPROVED by mtasaka
------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 11/12, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 7 Maxim Burgerhout 2010-01-26 07:13:29 UTC
Mamoru, I have requested membership. My FAS name is wzzrd.

Comment 8 Mamoru TASAKA 2010-01-26 07:40:32 UTC
Now I am sponsoring you. Please follow "Join" wiki again.

Comment 9 Maxim Burgerhout 2010-01-26 09:00:40 UTC
Thanks! Here's the CVS request:

New Package CVS Request
=======================
Package Name: libyubikey
Short Description: C library for decrypting and parsing Yubikey One-time passwords
Owners: wzzrd
Branches: F-12
InitialCC:

Comment 10 Jason Tibbitts 2010-01-27 05:25:06 UTC
CVS done (by process-cvs-requests.py).

Comment 11 Mamoru TASAKA 2010-01-27 14:23:52 UTC
Please actually build this package on koji (not by scratch build)
and for F-12 please submit push requests on bodhi.

Comment 12 Maxim Burgerhout 2010-01-27 14:33:15 UTC
Will do. CVS import gave me a hard time this morning, so as soon as I have that working, I'm building. The scratch builds were just me checking out koji.

Comment 13 Fedora Update System 2010-01-27 19:43:40 UTC
libyubikey-1.5-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/libyubikey-1.5-4.fc12

Comment 14 Maxim Burgerhout 2010-01-27 19:45:23 UTC
Submitted to Bodhi. Thanks, Mamoru!

Comment 15 Mamoru TASAKA 2010-01-27 20:03:57 UTC
Closing.

Comment 16 Fedora Update System 2010-02-01 01:20:34 UTC
libyubikey-1.5-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Maxim Burgerhout 2010-10-13 06:51:22 UTC
Package Change Request
======================
Package Name: libyubikey
New Branches: el5 el6
Owners: wzzrd

Recent builds of fedora-packager require ykpers (and therefore libyubikey); fedora-packager availability in EPEL requires availability of ykpers and libyubikey in EPEL, too.

Comment 18 Tom "spot" Callaway 2010-10-13 15:09:38 UTC
Git done (by process-git-requests).