Bug 445152 - Review Request: yacpi - ncurses based acpi viewer
Review Request: yacpi - ncurses based acpi viewer
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: 445153
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-04 18:15 EDT by Sven Lankes
Modified: 2008-10-01 02:36 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-01 02:36:18 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)

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

Spec URL: http://sven.lank.es/Fedora/SPECS/yacpi.spec
SRPM URL: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-5.fc9.src.rpm
Description: Yacpi is an acpi monitoring program for Linux. It displays various acpi information like battery status and ac status on notebooks. 

yacpi depends on libacpi for which I am going to create a separate package review.
Comment 1 manuel wolfshant 2008-05-04 20:18:54 EDT
It is recommended to use macros instead of using fixed paths. Therefore
"/usr/share/doc/" should be replaced by %{_defaultdocdir} and "/usr/share/man/"
by %{_mandir}

Based on the header of get_cpu.* files, I think that the correct license tag is
GPLv2+.

The presence of the explicit CFLAGS line in the Makefile prohibits the build
process to use the mandatory Fedora compile flags.
Comment 2 Sven Lankes 2008-05-05 03:03:11 EDT
> It is recommended to use macros instead of using fixed paths.

Fixed. I'm also now using %{_defaultdocdir} while converting the changelog to
UTF8 - is that correct there?

> Based on the header of get_cpu.* files, I think that the correct 
> license tag is GPLv2+.

You are right. I don't know why I entered GPLv2

> The presence of the explicit CFLAGS line in the Makefile prohibits the build
> process to use the mandatory Fedora compile flags.

Removed.

New SRPM: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-6.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/yacpi.spec
Comment 3 Michael Schwendt 2008-05-07 17:35:32 EDT
The patch is dangerous. If it were a larger program, there would
be the risk that DESTDIR finds its way into compiled files.
prefix should become /usr, and only the install paths (in the
install Makefile target) should start with DESTDIR.


> Requires: ncurses

Drop this. There's an automatic dependency on the ncurses
library SONAME added by rpmbuild. Query the binary rpm to see.


%dir %{_defaultdocdir}/yacpi  is missing in %files list.


manual pages are marked as %doc automatically.


CFLAGS still don't include $RPM_OPT_FLAGS.
Comment 4 Sven Lankes 2008-05-07 19:17:35 EDT
Should be all fixed now. Thanks.

New Version:

New SRPM: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-7.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/yacpi.spec
Comment 5 Sven Lankes 2008-05-07 19:51:56 EDT
The package installed into /usr/share/doc/yacpi instead of yacpi-version until now.

The following update fixes this:

New SRPM: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-8.fc9.src.rpm
SPEC: http://sven.lank.es/Fedora/SPECS/yacpi.spec
Comment 6 Rakesh Pandit 2008-07-05 08:19:25 EDT
I am not sponsored yet, so cannot review. Just have some helpful remarks 

Could not build:
No package libacpi-devel

Without building I have a small suggestions:

1. You should consistently use %{name} e.g Source0 link or in %files (man page)
2. For single file encoding change this one looks lot simple and clean
 iconv -f iso-8859-1 -t utf-8 -o CHANGELOG{.utf8,}
 mv CHANGELOG{.utf-8}

Yours is also okay, it is your choice.

3. description needs a bit correction:
    This part "battery status and ac status" is mentioned twice.

4. Preferred Buildroot tag is %(mktemp -ud %{_tmppath}/%{name}-%{version}-
%{release}-XXXXXX)

Comment 7 Rakesh Pandit 2008-07-05 08:42:59 EDT
due to missing build requirements (libacpi-devel) that is not available in 
Fedora.  If you've or are planning to submitted it for review, then it should 
be a blocker on this bug.
Comment 8 Sven Lankes 2008-07-07 14:47:13 EDT
Hello Rakesh, 

thanks for your remarks. I have updated the specfile:

SPEC: http://sven.lank.es/Fedora/SPECS/yacpi.spec
New SRPM: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-9.fc9.src.rpm

0. no package libacpi-devel: libacpi is a separate review request - #445153 -
445153 blocks this bug so it looks correct in my eyes?
1. fixed
2. changed, I agree - two lines less is better
3. fixed
4. changed
Comment 9 manuel wolfshant 2008-08-02 00:46:28 EDT
Due to the stricter checking implemented now in rpm, you'll have to replace
"%patch -p1" with "%patch0 -p1", otherwise the patch is not applied.


Two more serious issues
- Neither rpm_opt_flags nor parallel build flags are taken into consideration
while actually compiling
- the created debuginfo rpm is empty
Comment 10 Sven Lankes 2008-08-02 05:47:33 EDT
> Due to the stricter checking implemented now in rpm, you'll have to replace
> "%patch -p1" with "%patch0 -p1", otherwise the patch is not applied.

fixed

- Neither rpm_opt_flags 

Wow - good catch. Fixed. I've also notified upstream.

- nor parallel build flags are taken into consideration

Fixed

- the created debuginfo rpm is empty

... because the makefile was stripping the binary. Fixed.
Comment 11 Sven Lankes 2008-08-02 05:48:15 EDT
forgot the new URLs:

Spec URL: http://sven.lank.es/Fedora/SPECS/yacpi.spec
SRPM URL: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-10.fc10.src.rpm
Comment 12 manuel wolfshant 2008-08-06 05:33:06 EDT
https://bugzilla.redhat.com/show_bug.cgi?id=445153#c17 applies here too, for the moment. will come back once I have news.
Comment 13 manuel wolfshant 2008-08-19 09:19:55 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, please 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 14 Sven Lankes 2008-08-19 17:48:32 EDT
ExcludeArch added. 

libacpi/yacpi can hopefully be replaced by something based on DeviceKitPower in the future but until that happens the tools are still very useful.

Spec URL: http://sven.lank.es/Fedora/SPECS/yacpi.spec
SRPM URL: http://sven.lank.es/Fedora/SRPM/yacpi-3.0.1-11.fc10.src.rpm
Comment 15 manuel wolfshant 2008-08-19 18:14:53 EDT
package APPROVED
Comment 16 Sven Lankes 2008-08-19 18:27:21 EDT
New Package CVS Request
=======================
Package Name: yacpi
Short Description: Yet Another Configuration and Power Interface 
Owners: slankes
Branches: F-9
InitialCC:
Cvsextras Commits: yes
Comment 17 Kevin Fenzi 2008-08-23 14:12:48 EDT
cvs done.
Comment 18 manuel wolfshant 2008-09-17 21:50:11 EDT
Sven, I see that the package was built. Any reason to not close the bug ?
Comment 19 Fedora Update System 2008-09-18 16:34:55 EDT
yacpi-3.0.1-11.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/yacpi-3.0.1-11.fc9
Comment 20 Fedora Update System 2008-09-24 20:24:01 EDT
yacpi-3.0.1-11.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 yacpi'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8337
Comment 21 Fedora Update System 2008-10-01 02:36:14 EDT
yacpi-3.0.1-11.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.