Bug 891941 - Review Request: istatd - Daemon serving statistics to your iStat iPhone application
Summary: Review Request: istatd - Daemon serving statistics to your iStat iPhone appli...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tomasz Torcz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-01-04 15:19 UTC by Lorenzo Dalrio
Modified: 2013-04-24 13:45 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-04-24 13:45:52 UTC
Type: ---
Embargoed:
tomek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Lorenzo Dalrio 2013-01-04 15:19:23 UTC
Spec URL: http://lorenzodalrio.fedorapeople.org/istatd.spec
SRPM URL: http://lorenzodalrio.fedorapeople.org/istatd-0.5.8-2.fc17.src.rpm
Description: istatd is a daemon serving statistics to your iStat iPhone
application from Linux, Solaris & FreeBSD.
istatd collects data such as CPU, memory, network and disk
usage and keeps the history.
Once connecting from the iPhone and entering the lock code
this data will be sent to the iPhone and shown in fancy graphs.
Fedora Account System Username: lorenzodalrio

Comment 1 Lorenzo Dalrio 2013-01-04 15:53:44 UTC
Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4840142

Comment 2 Tomasz Torcz 2013-04-09 12:44:39 UTC
===== SHOULD items =====

Generic:
[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[ ]: Package functions as described.
[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]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[!]: Files in /run, var/run and /var/lock uses tmpfiles.d when appropriate
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: istatd-0.5.8-2.fc17.x86_64.rpm
istatd.x86_64: W: spelling-error Summary(en_US) iStat -> i Stat, stat, is tat
istatd.x86_64: W: spelling-error %description -l en_US iStat -> i Stat, stat, is tat
istatd.x86_64: W: only-non-binary-in-usr-lib
istatd.x86_64: W: manual-page-warning /usr/share/man/man1/istatd.1.gz 48: warning: macro `list-type-stack0' not defined
istatd.x86_64: W: non-standard-uid /var/run/istat istat
istatd.x86_64: W: non-standard-gid /var/run/istat istat
istatd.x86_64: E: non-standard-dir-perm /var/run/istat 0750L
istatd.x86_64: W: non-standard-uid /var/cache/istat istat
istatd.x86_64: W: non-standard-gid /var/cache/istat istat
istatd.x86_64: E: non-standard-dir-perm /var/cache/istat 0750L
1 packages and 0 specfiles checked; 2 errors, 8 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint istatd
istatd.x86_64: W: spelling-error Summary(en_US) iStat -> i Stat, stat, is tat
istatd.x86_64: W: spelling-error %description -l en_US iStat -> i Stat, stat, is tat
istatd.x86_64: W: only-non-binary-in-usr-lib
istatd.x86_64: W: manual-page-warning /usr/share/man/man1/istatd.1.gz 48: warning: macro `list-type-stack0' not defined
istatd.x86_64: W: non-standard-uid /var/run/istat istat
istatd.x86_64: W: non-standard-gid /var/run/istat istat
istatd.x86_64: E: non-standard-dir-perm /var/run/istat 0750L
istatd.x86_64: W: non-standard-uid /var/cache/istat istat
istatd.x86_64: W: non-standard-gid /var/cache/istat istat
istatd.x86_64: E: non-standard-dir-perm /var/cache/istat 0750L
1 packages and 0 specfiles checked; 2 errors, 8 warnings.
# echo 'rpmlint-done:'


Ok, so this package looks fairly good. rpmlint output is not important.
Nevertheless, some work is still needed:

1) /var/run is a symlink to /run.  Please change %{_var}/run into /run

2) consider %ghost for /run/istat, as /run is on tmpfs and directories there disappear after reboot

3) because of tmpfs, you have to provide config file and snippet for /run/istat creation: https://fedoraproject.org/wiki/Packaging:Tmpfiles.d

4) I see you've created package for F17. You already have 3 versions to build package for (F18, F19 and F20/rawhide) and F17 will go away quite soon. Maybe we won't be able to get this package into F17 before it EOLs. So I suggest you should target F18 - convert systemctl invocation into snippets: https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

Please ask upstream:
- to provide LICENSE file
- to package your unit and tmpfiles.d files; man 7 daemon on Fedora will show you autoconf scriptlets to use

Comment 3 Tomasz Torcz 2013-04-09 13:19:53 UTC
BTW, you can shorten systemd.unit file: remove "-d" switch and then you can remove Type and guesspid lines (defaults will work fine).

Comment 4 Lorenzo Dalrio 2013-04-11 13:57:49 UTC
Hi Tomasz,

1) Changed

2) Since istatd does not install any file in /run/istat i have not applied %ghost, the directory creation is managed by systemd-tmpfiles now.

3) Done

4) Modified to target F18, F17 was the current release when i submitted the package for review, i do not need F17 support.

I have also applied your suggestions to unit file and modified unit description.

Spec URL: http://lorenzodalrio.fedorapeople.org/istatd.spec
SRPM URL: http://lorenzodalrio.fedorapeople.org/istatd-0.5.8-3.fc18.src.rpm

Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5241323

I am working to submit a pull request to upstream to include missing LICENSE file and systemd unit file.

Comment 5 Tomasz Torcz 2013-04-12 15:59:05 UTC
Almost perfect, but rpmlint errors now:

istatd.spec:43: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d
istatd.spec:45: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d/istatd.conf
istatd.spec:74: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d/istatd.conf


please uset %{_tmpfilesdir} macro instead of %{_prefix}/lib/tmpfiles.d.

Comment 6 Lorenzo Dalrio 2013-04-12 17:20:51 UTC
Modified to use correct tmpfiles.d macro.

Spec URL: http://lorenzodalrio.fedorapeople.org/istatd.spec
SRPM URL: http://lorenzodalrio.fedorapeople.org/istatd-0.5.8-4.fc18.src.rpm

Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5246379

Comment 7 Tomasz Torcz 2013-04-13 10:52:23 UTC
OK, look fine for me now. Package approved, thank you for making Fedora better.

Can I ask you to review owfs as swap? It is quite more complicated, though.
https://bugzilla.redhat.com/show_bug.cgi?id=927237

Comment 8 Lorenzo Dalrio 2013-04-24 08:40:05 UTC
New Package SCM Request
=======================
Package Name: istatd
Short Description: Daemon serving statistics to your iStat iPhone application
Owners: lorenzodalrio
Branches: f18 f19 el6
InitialCC:

Comment 9 Gwyn Ciesla 2013-04-24 12:05:47 UTC
Git done (by process-git-requests).


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