Bug 474992 - Review Request: libirman - Library for IRMAN hardware
Review Request: libirman - Library for IRMAN hardware
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-06 08:14 EST by Jan ONDREJ
Modified: 2011-10-06 08:26 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.4.5-3.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-29 12:51:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jan ONDREJ 2008-12-06 08:14:16 EST
Spec URL: http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
SRPM URL: http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.4-2.fc10.src.rpm
Description:
A library for accessing the IRMAN hardware from Linux and other Unix systems.


This package contains only a static library. It's not suggested to use static libraries in Fedora, but it's not forbidden.

If somebody can help me to patch this to build as shared (libirman.so), please send me or attach patch here.

This library is required to update lirc to use IRMAN hardware. I can make required changes in lirc.spec after this package will be approved.
Comment 1 Fabian Affolter 2008-12-06 08:54:19 EST
Just some quick comments on your spec file

- 'Release: 3%{?dist}' should be 'Release: 1%{?dist}' and your changelog entry doesn't match the release.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

 [fab@laptop024 i386]$ rpmlint libirman*
 libirman.i386: W: incoherent-version-in-changelog 0.4.4-2 ['0.4.4-3.fc9', '0.4.4-3']
 3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- URL should point to the website of the upstream project.  Why not http://www.lirc.org/ ?
Comment 2 Jan ONDREJ 2008-12-06 13:06:58 EST
(In reply to comment #1)
> Just some quick comments on your spec file
> 
> - 'Release: 3%{?dist}' should be 'Release: 1%{?dist}' and your changelog entry
> doesn't match the release.
>   https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

ChangeLog entry fixed. This release 3 is because there is libirman of this release in my repository and I need updates from this version.

>  [fab@laptop024 i386]$ rpmlint libirman*
>  libirman.i386: W: incoherent-version-in-changelog 0.4.4-2 ['0.4.4-3.fc9',
> '0.4.4-3']
>  3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Thanks. For info.

> - URL should point to the website of the upstream project.  Why not
> http://www.lirc.org/ ?

I think lirc is not upstream for this, it's just required by this package.
But if you stand that this URL is better, I can change it.
Comment 3 Fabian Affolter 2009-02-02 08:38:42 EST
Is there a new Source RPM somewhere?
Comment 4 Jan ONDREJ 2009-02-02 09:40:58 EST
Yes, I forgot to add them. Spec file URL is unchanged:

http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.4-3.fc10.src.rpm
Comment 5 Jan ONDREJ 2009-02-02 09:44:17 EST
Jarod, can you look if it helps to build IRMAN support in fedora's lirc package?
I also can create another bug for lirc and help to add IRMAN support.
Comment 6 Tom Hughes 2009-02-18 06:05:31 EST
I believe lirc is now the effective upstream for this. I have just reworked the build process to make shared library builds possible (with the intention of then submitting a Fedora package) and I submitted those back to the lirc folks who committed them to their CVS tree.

Getting lirc supporting libirman should be as simple as adding a libirman-devel BuildRequires to the lirc spec file - it was for me.
Comment 7 Jarod Wilson 2009-02-18 09:03:52 EST
Apologies, meant to look at this earlier... Yeah, once libirman is being built into Fedora, it should definitely be as straight-forward as BR: libirman-devel.
Comment 8 Jason Tibbitts 2009-03-13 16:04:25 EDT
Well, it's been a few weeks and nobody else has stepped in, so I'll review this even though I don't have the hardware.

For a multiple license scenario like this, you need to indicate (usually by a comment in the spec) which parts of the package are under which license.  I'm not sure which part of the main package might fall under the LGPL.

I don't see anywhere in the code that a version of the GPL or LGPL is specified, 
which makes the situation complex.  The LGPL parts end up as LGPLv2+ while GPL parts end up as GPL+, which when compiled together make the result GPLv2+.  Ugh.

I'm not sure why you call ldconfig; no dynamic libraries are installed by this package.  Actually, you get a static lib even though you pass --disable-static.  Any idea what's going on?

* source files match upstream.  sha256sum:
   b29d0858450c56fca97c03cb1032e3b469166d431bfa7327fa3183d31a9f64b2  
   libirman-0.4.4.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
? unsure whether the license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper (none).
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
  libirman-0.4.4-3.fc11.x86_64.rpm
   config(libirman) = 0.4.4-3.fc11
   libirman = 0.4.4-3.fc11
   libirman(x86-64) = 0.4.4-3.fc11
  =
   /sbin/ldconfig
   config(libirman) = 0.4.4-3.fc11

  libirman-devel-0.4.4-3.fc11.x86_64.rpm
   libirman-static = 0.4.4-3.fc11
   libirman-devel = 0.4.4-3.fc11
   libirman-devel(x86-64) = 0.4.4-3.fc11
  =
   libirman = 0.4.4-3.fc11

* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
X ldconfig scriptlets present, but I'm not sure why.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
* static libraries are present:
  No dynamic libs, so they can be in the -devel package.
  -static provide is there.
* no libtool .la files.
Comment 9 Tom Hughes 2009-03-13 17:31:30 EDT
The reason it is static despite the switch is that the 0.4.4 release of libirman doesn't support shared builds at all.

The current upstream CVS code does support shared builds and will respect the switch and produce shared objects.
Comment 10 Jason Tibbitts 2009-03-13 17:40:21 EDT
Would it be better to either wait for a newer release or to pull a snapshot from CVS?  I have no way of objectively determining whether we would benefit from having a dynamic library here.
Comment 11 Jan ONDREJ 2009-03-14 04:52:01 EDT
Thank you for review.

About license, there is 2 licenses mentioned in README. Added comment into spec file:
#The files which make up the library are covered under the GNU Library
#General Public License, which is in the file COPYING.lib.
#The files which make up the test programs and the documentation are covered
#under the GNU General Public License, which is in the file COPYING.
License:        GPLv2+ and LGPLv2+

My new rpms have included latest CVS patch. This patch removed Makefile.in and added Makefile.am, there is no configure script, so I have to include some buildrequires to create this with autogen.sh .

This patch adds dynamic library build for libirman and also fixes IRMAN restart, because it's required to stop powering IRMAN before reinit.
I see no other changes in this version.

%changelog
* Sat Mar 14 2009 Jan ONDREJ (SAL) <ondrejj(at)salstar.sk> - 0.4.4-4.20090314cvs
- applied cvs patch, which fixed dynamic library build and IRMAN restart
- added BuildRequires: autoconf, automake, libtool

rpmlint show just this warning, I think I can do nothing with this:
libirman.i386: W: shared-lib-calls-exit /usr/lib/libirman.so.0.0.0 exit@GLIBC_2.0

New packages:
http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.4-4.20090314cvs.fc10.src.rpm
Comment 12 Jason Tibbitts 2009-04-01 14:24:22 EDT
Sorry for taking so long; I had too many reviews in flight and somehow this one slipped through the cracks.  Please feel free to ping me if I've let something go idle for too long.

You seem to have clarified the license situation well enough, and the static stuff is gone, which is good.

However, it's kind of weird to patch up to a CVS snapshot by including a patch thats larger than the source tarball.  It's OK to pick patches out of the upstream SCM if that's what you want, but if you want to ship a snapshot, it's better to simply do a checkout and include that as your tarball.  See http://fedoraproject.org/wiki/Packaging/SourceURL for information on how we do this.  Basically, you make the tarball but include instructions for generating it so that someone else who comes along will know where it came from.  Currently you just have this big patch with no information on duplicating it.
Comment 13 Jan ONDREJ 2009-04-02 02:33:58 EDT
(In reply to comment #12)
> Sorry for taking so long;

No problem. We have time. I also forgot that I have an open review. :)

> However, it's kind of weird to patch up to a CVS snapshot by including a patch
> thats larger than the source tarball.  It's OK to pick patches out of the
> upstream SCM if that's what you want, but if you want to ship a snapshot, it's
> better to simply do a checkout and include that as your tarball.  See
> http://fedoraproject.org/wiki/Packaging/SourceURL for information on how we do
> this.  Basically, you make the tarball but include instructions for generating
> it so that someone else who comes along will know where it came from. 
> Currently you just have this big patch with no information on duplicating it.  

OK, repackaged as source and also sent an email to upstream mailinglist, which is asking to release this as stable (or at least official snapshot).

New package can be found here:
  http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
  http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.4-5.20090314cvs.fc10.src.rpm

It was also possible to strip patch by removing "configure" part, which is autoregenerated later from spec file.
Comment 14 Jan ONDREJ 2009-04-06 08:58:35 EDT
If somebody interested, here is my email to lirc mailinglist:

http://sourceforge.net/mailarchive/forum.php?thread_name=20090402060831.GO6207%40salstar.sk&forum_name=lirc-list

Without any reply yet. :-(
Comment 16 Jason Tibbitts 2009-04-16 15:54:21 EDT
This seems to be new:
  libirman.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/workmanir 
   ['/usr/lib64']

Any idea why that's just showing up now?
Comment 17 Jan ONDREJ 2009-04-18 02:46:33 EDT
(In reply to comment #16)
> Any idea why that's just showing up now?  

It has something with autoconf used to generate files in original package.
In CVS there was no configure scripts and they was regenerated from spec file.

Only --disable-rpath parameter does not help.

There are 2 solutions:
  1. use autoreconf in spec to create new scripts
  2. use chrpath

Solution #1 applied in my spec file:
  http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
  http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.5-2.fc10.src.rpm
Comment 18 Jan ONDREJ 2009-04-28 10:49:42 EDT
PING,

anything else, what I can do to be this package approved? :)
Comment 19 Jan ONDREJ 2009-05-11 16:16:55 EDT
(In reply to comment #12)
> Please feel free to me if I've let something go idle for too long.

Ping again.

No reply more then 4 weeks. :(
Comment 20 Jason Tibbitts 2009-05-11 16:23:06 EDT
I first reviewed this when I had time to do reviews, but because of the delay I now have very little time to do it.  You are welcome to find another reviewer, or wait until I have more time.  Your choice.
Comment 21 Jason Tibbitts 2009-05-16 19:47:33 EDT
OK, I found some time.  Unfortunately the package in comment #17 does not build for me in current F11.  I get a bunch of bizarre errors with libtool such as:

./libtool: line 793: X--tag=CC: command not found
./libtool: line 826: libtool: ignoring unknown tag : command not found

which cascade into:

./libtool: line 1103: X.deps/libirman_la-chunk.Tpo: No such file or directory
./libtool: line 1103: X-c: command not found
./libtool: line 1154: Xlibirman_la-chunk.lo: command not found
./libtool: line 1159: libtool: compile: cannot determine name of library object from `': command not found

To rule out problems with my local mock setup, I did a koji build which failed as well: http://koji.fedoraproject.org/koji/taskinfo?taskID=1358328

It does build in F10, though.  Can you work out what's broken in F11, or would you like me to review an F10 build?
Comment 22 Jan ONDREJ 2009-05-17 03:01:29 EDT
%changelog
* Sun May 17 2009 Ján ONDREJ (SAL) <ondrejj(at)salstar.sk> - 0.4.5-3
- added libtoolize to fix build for f11

Builds for f11 now.

http://www.salstar.sk/pub/fedora/SPECS/libirman.spec
http://www.salstar.sk/pub/fedora/SRPMS/10/libirman-0.4.5-3.fc10.src.rpm
Comment 23 Jason Tibbitts 2009-05-27 16:56:33 EDT
Indeed, this builds fine.  The complaints I had seem to be all solved now, and the package looks OK to me.

APPROVED
Comment 24 Jan ONDREJ 2009-05-28 03:05:33 EDT
Thank you. Looking that Kevin is on holiday, so please add CVS. :-)

New Package CVS Request
=======================
Package Name: libirman
Short Description: Library for IRMAN hardware
Owners: ondrejj
Branches: F-10 F-11
Comment 25 Jason Tibbitts 2009-05-28 12:02:54 EDT
CVS done.
Comment 26 Fedora Update System 2009-05-28 12:33:01 EDT
libirman-0.4.5-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/libirman-0.4.5-3.fc11
Comment 27 Fedora Update System 2009-05-28 12:33:40 EDT
libirman-0.4.5-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libirman-0.4.5-3.fc10
Comment 28 Jarod Wilson 2009-05-28 12:54:27 EDT
Building lirc w/irman support in rawhide now. Not sure if I need to wait a bit before I can do F10/F11 builds...
Comment 29 Jarod Wilson 2009-05-28 13:02:24 EDT
Ugh. Rawhide build failed, can't find libirman-devel yet for some reason. I'll just wait a bit to build.
Comment 30 Jason Tibbitts 2009-05-28 13:16:36 EDT
For rawhide, you just have to wait for a newrepo task to complete.

For the release branches, you need to wait until either the next updates push or request from releng that the package you need be tagged into the buildroot.  Honestly I don't know if or how the not-yet-released status of F11 alters that.
Comment 31 Jan ONDREJ 2009-05-29 09:40:38 EDT
Jarod, libirman has been just moved to F10/F11:

Subject: libirman-0.4.5-3.fc11 successfully moved from                          
        dist-f11-updates-candidate into dist-f11-updates by bodhi               
Subject: libirman-0.4.5-3.fc10 successfully moved from                          
        dist-f10-updates-candidate into dist-f10-updates by bodhi               

These should be available now or in a few minutes.
Comment 32 Jan ONDREJ 2009-05-29 12:51:27 EDT
Thanks to all, lirc-0.8.5-2.fc10.i386 works well with IRMAN. :-)

Closing bug.
Comment 33 Fedora Update System 2009-05-29 22:32:18 EDT
libirman-0.4.5-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 34 Fedora Update System 2009-05-29 22:34:57 EDT
libirman-0.4.5-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 35 Jan ONDREJ 2011-10-06 01:41:39 EDT
Package Change Request
======================
Package Name: libirman
New Branches: el6
Owners: ondrejj

Requested in https://bugzilla.redhat.com/show_bug.cgi?id=743566
Comment 36 Gwyn Ciesla 2011-10-06 08:26:15 EDT
Git done (by process-git-requests).

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