Bug 331531 - Review Request: ltspswapd - Daemon that uses nbd to provide swap space for LTSP thin clients
Summary: Review Request: ltspswapd - Daemon that uses nbd to provide swap space for LT...
Keywords:
Status: CLOSED DUPLICATE of bug 331731
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-10-14 22:21 UTC by Eric Harrison
Modified: 2007-11-30 22:12 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-11-20 04:01:36 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Eric Harrison 2007-10-14 22:21:01 UTC
Spec URL: http://k12linux.mesd.k12.or.us/K12LTSP/source/ltsp-ltspswapd/ltsp-ltspswapd.spec
SRPM URL: http://k12linux.mesd.k12.or.us/K12LTSP/source/ltsp-ltspswapd/ltsp-ltspswapd-0-20071014cvs.fc8.src.rpm
Description: Daemon that uses nbd to provide swap space for LTSP thin clients

Comment 1 Patrice Dumas 2007-10-15 09:39:56 UTC
The version/release of the package isn't right, have a look at
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-cfd71146dbb6f00cec9fe3623ea619f843394837

Regarding the name, why not simply ltspswapd?

For the source url, it is almost right, but please have 
a look at 
http://fedoraproject.org/wiki/Packaging/SourceURL
for an even better style.

I don't think that %{_localstatedir}/opt/ is the right var directory 
for the swap files. Maybe %{_localstatedir}/lib/ or %{_localstatedir}/spool
(with appropriate subdirectories), but /opt in general should always
be left empty by rpm installations.

You should use the -p switch to install, to keep timestamps, like
install -p -m 0644 %{SOURCE1} ${RPM_BUILD_ROOT}/%{_sysconfdir}/sysconfig/ltspswapd

The Source3 file is a bit strange. Isn't it a remnant of something
else?

Suggestion:
the %{_*dir} marcos already have a leading /, so it is not needed
in ${RPM_BUILD_ROOT}/%{_sysconfdir}/sysconfig/, or in 
/%{_sysconfdir}/sysconfig/ltspswapd

Comment 2 Patrice Dumas 2007-10-15 09:41:27 UTC
Also ltspswapd.init lacks a lsb init header.

Comment 4 Patrice Dumas 2007-10-16 23:44:07 UTC
You should change the LTSPSWAPD_FILEPATH in ltspswapd.sysinit,
something along
sed -e 's;LTSPSWAPD_FILEPATH=.*;LTSPSWAPD_FILEPATH=%{swapdir};' %{SOURCE1} >
${RPM_BUILD_ROOT}%{_sysconfdir}/sysconfig/ltspswapd

(and if you are pedantic about timestamps, like I do)
touch -r %{SOURCE1} ${RPM_BUILD_ROOT}%{_sysconfdir}/sysconfig/ltspswapd

No need for /sbin/service in Requires(post)

In the cvs checkout command there should be a switch that makes
sure that the cvs version is the same one than the one in the rpm.

%{_localstatedir}/lib/ltsp/ is unowned.

Does ltspswapd really requires nbd? It looks like it has its own
nbd-server implementation.

Comment 5 Patrice Dumas 2007-10-18 08:34:40 UTC
Now that there is a central configuration for ltsp5, I think
that that package should use it too. It means that the
config file won't be there is ltsp-server isn't installed, but 
this should never happen.

Comment 6 Eric Harrison 2007-10-19 04:25:16 UTC
Spec URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd.spec
SPRM URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd-0-0.3.20071016cvs.fc8.src.rpm

- source /etc/ltsp5.conf to find swap directory
- require /etc/ltsp5.conf
- move /var/opt/ltsp5 to /var/lib/ltsp5


Comment 7 Patrice Dumas 2007-10-19 09:20:30 UTC
I think that it is wrong to require /etc/ltsp.conf. A user
may want to use ltspswapd alone. Therefore, I think that 
%{_sysconfdir}/sysconfig/ltspswapd should still used, but only 
if there is no /etc/ltsp5.conf. And it should be said in a
comment in %{_sysconfdir}/sysconfig/ltspswapd.

The corresponding code in init file would be

# Source ltspswapd configuration.
[ -f /etc/sysconfig/ltspswapd ] && . /etc/sysconfig/ltspswapd
[ -f /etc/ltsp5.conf ] && . /etc/ltsp5.conf

A dot is missing at the end of the %description

a -r or similar for the cvs command is also still missing.

Only a suggestion, but in general the %define are put at
the very beginning of the spec file. 

Comment 8 Dan Young 2007-10-19 15:18:54 UTC
Spec URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd.spec
SRPM URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd-0-0.4.20071019cvs.fc8.src.rpm

- source /etc/sysconfig/ltspswapd, then /etc/ltsp5.conf in initscript
- read-only checkout from CVS
- fix description, move %%define to top


Comment 9 Patrice Dumas 2007-10-20 00:19:45 UTC
(In reply to comment #8)

> - read-only checkout from CVS

It is not what I asked for, I asked for the -r option
of checkout:
        -r rev  Check out revision or tag. (implies -P) (is sticky)
You can also use -D.

Comment 10 Eric Harrison 2007-10-24 06:01:41 UTC
-added "-D <date>" as per comment #9
-updated config file location to match changes in ltsp-server

Spec URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd.spec
SRPM URL:
http://k12linux.mesd.k12.or.us/K12LTSP/source/ltspswapd/ltspswapd-0-0.5.20071023cvs.fc8.src.rpm


Comment 11 Patrice Dumas 2007-10-24 16:19:56 UTC
You should remove the dependency on  /etc/ltsp5/ltsp.conf such 
that the package may be installed on its own. And in my
opinion the comment in sysconfig/ltspswapd should be along

# 
# the LTSP_SWAP_DIR variable is preferably set in /etc/ltsp5/ltsp.conf 
# to have a central config file. If you don't want to use /etc/ltsp5/ltsp.conf 
# you can also set this variable in this file
#

%{_localstatedir}/lib is already owned by the filesystem package.

You could also add LTSP_SWAP_DIR in a comment in sysconfig/ltspswapd,
like

# LTSP_SWAP_DIR=/var/lib/ltsp5/swapfiles/

The following lines are not useful if the comment is not there:
sed -e 's;LTSPSWAPD_FILEPATH=.*;LTSPSWAPD_FILEPATH=%{swapdir};' %{SOURCE1} >
${RPM_BUILD_ROOT}%{_sysconfdir}/sysconfig/ltspswapd
touch -r %{SOURCE1} ${RPM_BUILD_ROOT}%{_sysconfdir}/sysconfig/ltspswapd

And if the comment is there, LTSPSWAPD_FILEPATH should be 
changed to LTSP_SWAP_DIR

Comment 12 Warren Togami 2007-11-08 04:20:41 UTC
nbdswapd is provided in the upstream ltsp repository, which builds into
sub-packages ltsp-client and ltsp-server.  We should verify that the upstream
nbdswapd suits our needs, then cancel this review.


Comment 13 Warren Togami 2007-11-20 04:01:36 UTC
OK, it is pretty clear that we will use upstream's script integrated into
ltsp-server instead.  Closing this as duplicate against ltsp-server.


*** This bug has been marked as a duplicate of 331731 ***


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