Bug 445153 - Review Request: libacpi - General purpose library for ACPI
Review Request: libacpi - General purpose library for ACPI
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 445152
  Show dependency treegraph
 
Reported: 2008-05-04 18:19 EDT by Sven Lankes
Modified: 2008-10-01 02:35 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-01 02:35:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
modified spec; I was to lazy to add the mandatory changelog entry (2.08 KB, text/plain)
2008-05-05 18:47 EDT, manuel wolfshant
no flags Details
modified Makefile patch. preserves timestamps at install time (1.77 KB, text/plain)
2008-05-05 18:48 EDT, manuel wolfshant
no flags Details

  None (edit)
Description Sven Lankes 2008-05-04 18:19:27 EDT
I need a sponsor.

Spec URL: http://sven.lank.es/Fedora/SPECS/libacpi.spec
SRPM URL: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-4.fc9.src.rpm
Description: libacpi is a general purpose shared library for programs gathering 
ACPI data on Linux. Features: Thermal zones support, Battery support, 
Fan support, AC support

libacpi is a dependency for yacpi (review request: #445152) and few other acpi-reading tools. I intend to package at least http://www.vanheusden.com/acpitail/ in the future.
Comment 1 manuel wolfshant 2008-05-04 20:06:56 EDT
Please do not manually strip the debuginfo from the compiled binary files. Doing
that breaks generation of the -debuginfo package.
You should be consistent in using macros. /usr/share/doc and /usr/share/man
should be replaced by %{_defaultdocdir} and  %{_mandir}, respectively. Same goes
for /usr/bin and /usr/include/.
*.a files, _if_ they they have to be packaged should be placed in a separated
libacpi-static rpm (see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7).

Comment 2 Sven Lankes 2008-05-05 03:37:59 EDT
> Please do not manually strip the debuginfo from the compiled binary files. Doing
> that breaks generation of the -debuginfo package.

This one confuses me a little - how is the file going to get stripped if I don't
do it manually? I have removed the strip for now but now rpmlint (rightfully in
my opinion) complains about unstripped binaries ..

> You should be consistent in using macros.

fixed

> *.a files, _if_ they they have to be packaged should be placed in a separated
libacpi-static

I don't think a -static package is needed here. I have removed the .a-File.

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-5.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 3 manuel wolfshant 2008-05-05 10:53:01 EDT
rpmbuild will strip the binaries when needed.
Comment 4 manuel wolfshant 2008-05-05 11:12:20 EDT
Please fix
CFLAGS   = -fPIC -g --pedantic -Wall -Wextra
 first (you should make sure that the standard Fedora compile flags are used )
and then we'll see why we get
libacpi.x86_64: W: unstripped-binary-or-object /usr/lib64/libacpi.so.0
libacpi-debuginfo.x86_64: E: empty-debuginfo-package
Comment 5 Sven Lankes 2008-05-05 15:20:40 EDT
Seems that my conclusions wrt. stripping were quite a bit off ;)

I have replaced the CFLAGS with $RPM_OPT_FLAGS - this didn't help with the
unstripped .so-file.

Grepping through a bunch of existing specfiles did help - if a library isn't
executable, rpmlint won't strip it. I have added a chmod +x and now everything
works as expected. 

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-6.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 6 manuel wolfshant 2008-05-05 18:46:37 EDT
I am attaching a modified spec + patch which will preserve the timestamps of the
files which are included (as opposed to generated) and also drops the .a
directly at install time
Comment 7 manuel wolfshant 2008-05-05 18:47:40 EDT
Created attachment 304573 [details]
modified spec; I was to lazy to add the mandatory changelog entry
Comment 8 manuel wolfshant 2008-05-05 18:48:14 EDT
Created attachment 304574 [details]
modified Makefile patch. preserves timestamps at install time
Comment 9 Sven Lankes 2008-05-06 17:41:50 EDT
I have incorporated your changes. I have contacted upstream - the author has
added the time-preserving changes to his tree already - they will be in the 
next release (still some time away).

Added a patch from debian (the debian maintainer is upstream) to use the sysfs 
interface for battery information instead of the (deprecated) proc-fs.

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-7.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 10 Michael Schwendt 2008-05-07 17:26:25 EDT
%dir %{_defaultdocdir}/libacpi    is missing in %files list
Comment 11 Sven Lankes 2008-05-07 19:25:09 EDT
I have added %dir-directives to libacpi and libacpi-devel rpms

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-8.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 12 Sven Lankes 2008-05-07 19:53:19 EDT
Docs where installed into %{_defaultdocdir}/name instead of name-version

This update fixes this:

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-9.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 13 Sven Lankes 2008-07-07 14:49:22 EDT
Some minor tweaks to the specfile after some remarks in the yacpi review request:

 * Changed BuildRoot
 * replace some occurences of libacpi with %{name}

New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-10.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec
Comment 14 manuel wolfshant 2008-08-01 18:45:16 EDT
The newer version of rpm has become pickier, so you'll have to replace %patch
-p1 with "%patch0 -p1". Otherwise mock bails out with an error.

Also, you must either modify the second patch (recommended method !) or add
   %define _default_patch_fuzz 2
to the spec file, otherwise the patch does not apply with current koji/mock settings

Everything else seems OK at the first glance. Please fix the above and I'll do a
full review.
Comment 15 manuel wolfshant 2008-08-01 18:49:04 EDT
I also suggest changing 
"Note: This is no portable code, it will just run on a Linux system."
to
"Note: This is no portable code, it will run just on a Linux system."

Given that the developer is not a native English speaker (neither am I, so I
might be wrong here) I think my version express better what he wanted to say.
Comment 16 Sven Lankes 2008-08-02 04:53:16 EDT
New SRPM: http://sven.lank.es/Fedora/SRPM/libacpi-0.2-11.fc10.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/libacpi.spec

I changed 'just run' to 'only run' and redid the patch to apply without fuzz.
Comment 17 manuel wolfshant 2008-08-06 05:32:07 EDT
I have not forgotten about this. I am trying to identify if these libs work on PPC  but for the moment I do not have the hardware at hand.
Comment 18 manuel wolfshant 2008-08-12 08:16:30 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64, devel/PPC
 [x] Rpmlint output:
source RPM: empty
binary RPM:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct ( %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:MIT
 [x] 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] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM of package: 4f2911cb0c737335003c2c13edef76821dee2cd1  /tmp/libacpi-0.2.tar.gz
 [!] Package is not known to require ExcludeArch
See issue 1
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
 [x] Package consistently uses macros.                                      
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.                                                                                            
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64, devel/ppc
 [-] Package should compile and build into binary rpms on all supported architectures.
     Tested on:
 [!] Package functions as described.
see issue 1
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


=== Issues ===
1. the package builds happily on PPC, but I had no chance yet to test if it actually works on that architecture

I'll come back as soon as I can do the test
Comment 19 manuel wolfshant 2008-08-19 09:11:36 EDT
With the invaluable help of David Woodhouse who spared some cycles and his personal time it was proved that this application does not work on ppc because the code is "far too dependent on ACPI nonsense, and not using the proper generic interfaces to stuff."

Therefore the spec file must exclude the ppc/ppc64 architecture. Sven, add the required bits and I'll approve the package.
Comment 20 manuel wolfshant 2008-08-19 09:18:20 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64, devel/PPC
 [x] Rpmlint output:
source RPM: empty
binary RPM:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct ( %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:MIT
 [x] 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] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM of package: 4f2911cb0c737335003c2c13edef76821dee2cd1  /tmp/libacpi-0.2.tar.gz
 [!] Package is known to not work on ppc/ppc64
See issue 1
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64, devel/ppc
 [-] Package should compile and build into binary rpms on all supported architectures.
     Tested on:
 [!] Package functions as described.
See isssue 1
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


=== Issues ===
1. See https://bugzilla.redhat.com/show_bug.cgi?id=445153#c19
Short version: the code is dependent on x86 specific architecture and cannot be used on ppc
Therefore the ppc architecture must be excluded


Sven, add the required excludearch line in the spec and I'll approve the package.
Wishful thinking: it would be nice of you could persuade the upstream author to rewrite the code in a more portable way. Or ar least tell him about the problems .
Comment 21 manuel wolfshant 2008-08-19 09:19:13 EDT
Sorry, please ignore comment #20, it was meant for bug 445152
Comment 22 manuel wolfshant 2008-08-19 09:21:35 EDT
One last suggestion: change "Note: This is no portable code, it will only run on a Linux system." to "Note: This is no portable code, it will only run on a Linux system with an x86 architecture."
Comment 24 manuel wolfshant 2008-08-19 18:15:20 EDT
package APPROVED
Comment 25 Sven Lankes 2008-08-19 18:25:08 EDT
New Package CVS Request
=======================
Package Name: libacpi
Short Description: General purpose library for ACPI 
Owners: slankes
Branches: F-9
InitialCC:
Cvsextras Commits: yes
Comment 26 David Woodhouse 2008-08-20 08:38:59 EDT
For the record, I really don't think we should be shipping this as a library in its current form. It's not generically useful -- it's ACPI-specific and broken.

It should be using _generic_ interfaces -- such as /sys/class/power_supply for battery information, for example -- rather than the ACPI-specific ones which are going to be going away.
Comment 27 Sven Lankes 2008-08-20 15:17:00 EDT
David,

/sys/class/power_supply is used. See libacpi-0.2-sysfs.patch - while this all isn't perfect yet (and quite hackish) and I agree that having something based on e.g. devicekit/power would be much nicer it is something that works now and gets the job done. It will either improve or be replaced by something better over time.
Comment 28 Kevin Fenzi 2008-08-24 15:12:07 EDT
cvs done.
Comment 29 manuel wolfshant 2008-09-17 21:50:01 EDT
Sven, I see that the package was built. Any reason to not close the bug ?
Comment 30 Fedora Update System 2008-09-18 16:34:09 EDT
libacpi-0.2-12.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/libacpi-0.2-12.fc9
Comment 31 Fedora Update System 2008-09-24 20:14:02 EDT
libacpi-0.2-12.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libacpi'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8263
Comment 32 Fedora Update System 2008-10-01 02:35:15 EDT
libacpi-0.2-12.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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