Bug 236849 - Review Request: lcdproc - LCDproc displays real-time system information on a 20x4 backlit LCD
Summary: Review Request: lcdproc - LCDproc displays real-time system information on a ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: manuel wolfshant
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-17 23:29 UTC by Nicolas Chauvet (kwizart)
Modified: 2007-11-30 22:12 UTC (History)
0 users

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-05-22 00:16:10 UTC
Type: ---
Embargoed:
manuel.wolfshant: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)
F7 x86_64 mock build.log (136.96 KB, text/plain)
2007-05-20 13:12 UTC, Nicolas Chauvet (kwizart)
no flags Details

Description Nicolas Chauvet (kwizart) 2007-04-17 23:29:36 UTC
Spec URL: 
http://kwizart.free.fr/fedora/6/testing/lcdpro/lcdproc.spec
SRPM URL: 
http://kwizart.free.fr/fedora/6/testing/lcdpro/lcdproc-0.5.1-1.kwizart.fc6.src.rpm
Description: LCDproc displays real-time system information on a 20x4 backlit LCD

There is still a need to tweak init scripts
W: lcdproc no-reload-entry /etc/rc.d/init.d/LCDd
W: lcdproc service-default-enabled /etc/rc.d/init.d/LCDd

Comment 1 manuel wolfshant 2007-05-19 23:57:27 UTC
The URLs above do not seem valid, the site is not accessible. Could you please
take care of that, Nicolas?

Comment 2 Nicolas Chauvet (kwizart) 2007-05-20 00:09:09 UTC
Well... indeed my website is down (isp server side).
I really hope this can be solved quickly

Until then, here is the SRPMS:
http://kwizartchitecture.free.fr/fedora/6/SRPMS/lcdproc-0.5.1-1.kwizart.fc6.src.rpm
And the spec file from my personnal workstation (mean not always online - but
often )

SPECS: http://kwizart.hd.free.fr/SPECS/browser/SPECS/lcdproc.spec



Comment 3 manuel wolfshant 2007-05-20 00:19:06 UTC
I've mirrored both files on my personal server, which is more or less always
online, just in case someone wishes to do a review and your site(s) are not
available.

http://wdl.lug.ro/linux/rpms/lcdproc/lcdproc.spec
http://wdl.lug.ro/linux/rpms/lcdproc/lcdproc-0.5.1-1.kwizart.fc6.src.rpm

Nicolas, let me know if you do not agree with this and I'll remove them at once.

Comment 4 manuel wolfshant 2007-05-20 01:14:31 UTC
Just a few notes, before a more comprehensive review:

- version 0.5.2 has been released by upstream a couple of weeks ago. any reason
to not update the rpm, too ?
- the following quote from the spec does not seem right:
 %makeinstall
 make install DESTDIR=$RPM_BUILD_ROOT
Since the whole ./configure process is done twice, it looks like the first line
is just a leftover from a previous version or something...
- I'd say the file CREDITS should also be included as %doc
- I get a lot of warnings during configure:

checking for libg15render.h... no
configure: WARNING: The g15driver needs libg15render.h
checking for main in -lirman... no
configure: WARNING: The irman driver needs the irman library.
checking for main in -llirc_client... no
configure: WARNING: The lirc driver needs the lirc client library
checking for vga.h... no
configure: WARNING: The svga driver needs vga.h and vgagl.h
checking for main in -lftdi... no
configure: WARNING: The ula200 driver needs the ftdi library

... and others. They look to me like missing build requires, or at least
screaming for a couple more packages needed. I do not know about some of them,
but lirc at least is available in Fedora, any reason to not use it?
- rpmlint has a small cosmetic complaint:
W: lcdproc mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 1)
- however rpmlint of lcdproc is a bit more serious:
W: lcdproc service-default-enabled /etc/rc.d/init.d/lcdproc
--> please fix that, a one line sed to patch the script should do it
W: lcdproc no-reload-entry /etc/rc.d/init.d/lcdproc
--> can be ignored, but it would be nice to update the initscript instead
W: lcdproc service-default-enabled /etc/rc.d/init.d/LCDd
W: lcdproc no-reload-entry /etc/rc.d/init.d/LCDd
-->same as above

- the postuninstall scriptlet is not particulary friendly. Shouldn't the service
be stopped prior to deleting it?
- I did not look very attentive, but there seem to exist some additional docs
and programs/examples available (and built by the make scripts). Any particular
reason to not include those (eventually as separate packages) ?


Comment 5 Nicolas Chauvet (kwizart) 2007-05-20 01:21:31 UTC
Well thx for you interest!
I was working on 5.2 and noticed some of theses errors..
I will update another version tomorrow..
thx
Nicolas

Comment 6 Nicolas Chauvet (kwizart) 2007-05-20 13:08:58 UTC
SRPMS:
http://kwizartchitecture.free.fr/fedora/7/testing/lcdproc/lcdproc-0.5.2-1.kwizart.fc7.src.rpm
SPECS:
http://kwizartchitecture.free.fr/fedora/7/testing/lcdproc/lcdproc.spec
Description: LCDproc displays real-time system information on a 20x4 backlit LCD

There is others BR that are not present in Fedora yet, indeed.
I May take a look on these, but this is not the first topic on my TODO list.
lcdproc can be used for freevo (currently testing on my test repository)

Since i do not own such lcd screen i'm not able to test it..
I will provide it on my repository until someone report it work fine...

Feel free to enahance this spec file...
(about doc , i'v installed them but not uses a separate sub-package, this could
be done eventually - examples are installed in bindir but they uses name that
are too generics, there is maybe a need to rename them...)



Comment 7 Nicolas Chauvet (kwizart) 2007-05-20 13:12:12 UTC
Created attachment 155060 [details]
F7 x86_64 mock build.log

Comment 8 manuel wolfshant 2007-05-20 16:40:35 UTC
Not very important but we have a duplicate BR: libX11-devel (by xosd-devel),
libXext-devel (by xosd-devel).

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
 [x] Rpmlint output: empty for all packages
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [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:GPL
 [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: 924fca84eb5a07464a3d38df86d9a4427fd06dd7
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [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.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] 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 %{buildroot} (or
$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.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [-] 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
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:devel/x86_64
 [! ] Package functions as described. See Note 1
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [x] File based requires are sane.


=== Issues ===
1.

=== Final Notes ===
1. I have not the ability to actually test the program for the simple reason
that I do not have access to the required hardware.
2. Acording to the build log, there are some unfulfilled dependencies. However
none of the needed resources seem to be available in Fedora, so unless someone
adds packages with them, the situation is as good as it could be.
3. Given the rather large number of files, I would split the developer-guide and
the user-guide to a separate -doc package. However, I would not consider this as
a blocker, because the mere size is small.


I see no blockers so I unless someone else sees something I've missed, I approve
this package.

================
*** APPROVED ***
================


Comment 9 Nicolas Chauvet (kwizart) 2007-05-20 23:29:30 UTC
New Package CVS Request
=======================
Package Name: lcdproc
Short Description: LCDproc displays real-time system information on a 20x4
backlit LCD
Owners: kwizart
Branches: FC-6 F-7 devel
InitialCC: 


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