Bug 331531 - Review Request: ltspswapd - Daemon that uses nbd to provide swap space for LTSP thin clients
Review Request: ltspswapd - Daemon that uses nbd to provide swap space for LT...
Status: CLOSED DUPLICATE of bug 331731
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-14 18:21 EDT by Eric Harrison
Modified: 2007-11-30 17:12 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-19 23:01:36 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Eric Harrison 2007-10-14 18:21:01 EDT
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 05:39:56 EDT
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 05:41:27 EDT
Also ltspswapd.init lacks a lsb init header.
Comment 4 Patrice Dumas 2007-10-16 19:44:07 EDT
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 04:34:40 EDT
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 00:25:16 EDT
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 05:20:30 EDT
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 11:18:54 EDT
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-19 20:19:45 EDT
(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 02:01:41 EDT
-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 12:19:56 EDT
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-07 23:20:41 EST
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-19 23:01:36 EST
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.