Bug 464430 - Review Request: k12linux-quick-start-guide - Doc describing how to enable LTSP on Fedora Live LTSP.
Review Request: k12linux-quick-start-guide - Doc describing how to enable LTS...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Extras Quality Assurance
:
Depends On:
Blocks: K12LTSP
  Show dependency treegraph
 
Reported: 2008-09-28 15:44 EDT by Peter Scheie
Modified: 2009-07-20 15:50 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-20 15:50:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pertusus: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Peter Scheie 2008-09-28 15:44:33 EDT
Spec URL: http://petre.homedns.org/k12linux/ltsp-server-livesetupdocs.spec
SRPM URL: http://petre.homedns.org/k12linux/ltsp-server-livesetupdocs-0.1-0.src.rpm
Description: A single HTML doc describing how to enable the LTSP server pieces on the Fedora Live LTSP system.  Installs a .desktop file to /etc/skel so that the default fedora user will have a link on the desktop.
Comment 1 Jason Tibbitts 2008-10-03 13:31:11 EDT
I don't see you in the packager group; in fact, it doesn't seem as if you've completed the CLA yet.  Setting NEEDSPONSOR.
Comment 2 Warren Togami 2008-10-05 11:10:34 EDT
I'm willing to sponsor Peter after the package is approved.
Comment 3 Warren Togami 2008-10-12 02:59:59 EDT
https://fedoraproject.org/wiki/Packaging/Guidelines
I took a look at the spec file.  Did you even read the Fedora packaging guidelines?

A few suggestions:
* Start from scratch using a .spec template from /etc/rpmdevtools/spectemplate*.spec from the rpmdevtools package.
* The source tarball, include a version number within the tarball name.  Generally like name-version.tar.bz2.
* Do not include a Packager tag.
* https://fedoraproject.org/wiki/Packaging/RPMMacros
  Replace path names like /etc and /usr/share with macros from this page.

You should also want to run rpmlint on both the .src.rpm and .noarch.rpm.  Correct everything that it complains about.

[warren@newcaprica tmp]$ rpmlint ltsp-server-livesetupdocs-0.1-0.src.rpm 
ltsp-server-livesetupdocs.src:10: W: hardcoded-packager-tag peter@scheie.homedns.org
ltsp-server-livesetupdocs.src: W: no-%build-section
ltsp-server-livesetupdocs.src: E: no-changelogname-tag
ltsp-server-livesetupdocs.src: W: invalid-license GPL
ltsp-server-livesetupdocs.src: W: no-url-tag
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

Assigning package review to me.
Comment 4 Warren Togami 2008-10-12 20:45:18 EDT
http://togami.com/~k12linux-temporary/fedora/9/src/ltsp-server-livesetupdocs-0.0.1-1.fc9.src.rpm

I ran out of time and had to spin with something so I made my own package.  Unassigning myself because I cannot review my own package.
Comment 5 Patrice Dumas 2008-10-13 04:01:51 EDT
Is this package to be installed on a specific spin/live cd?

I think that the description is misleading, since this package is not
especially about ltsp, but about fusionning a virtual bridge with 
a real interface. I don't deny that it is of specific help for 
ltsp, but I don't think that this should be stressed that much.
I'd suggest the following:

Summary:        Howto fusion a virtual interface with emphasis on LTSP

%description
A documentation explaining how to fusion a virtual bridge interface
with a real interface. This is required in a LTSP server setup, hence
there is some emphasis put on this case. A desktop file that is
added to all the new user desktops directing to that documentation 
is also part of this package.

The admin can remove this package after the setup is complete.


Also I'd suggest using xdg-open and not firefox in the desktop file,
and depend on xdg-open.

The source should not be only in srpm.
Comment 6 Patrice Dumas 2008-10-13 04:08:09 EDT
On the document itself I would suggest
* removing explicit reference to the Fedora version.

* remove the following paragraph, it is redundant with point 2.
  and, unless I am wrong the media has already been selected:
While LiveDVD is possible, LiveUSB with persitent overlay is recommended because it is more efficient in RAM usage. Be warned that your overlay or ramdisk will fill up and the demo will fail after a while, so do not rely on this Live image for production services. For production please install it onto a real hard drive. 

I think that 'Setup Instructions' should be changed to something along
'LTSP server network interface setup instructions'
Comment 7 Patrice Dumas 2008-10-13 04:09:30 EDT
Also point 16. should be removed, it is a bad idea, and the part about adding
a user should certainly be in another document (or a link to a fedora 
user manual that provides those instructions).
Comment 8 Warren Togami 2008-10-13 09:38:08 EDT
I am out of time, departing in less than 18 hours to a conference where I need this media.  Media is already spun.  No time to redo it.

This will be released as "rc1".  Please discuss with Peter to make further improvements to this package for rc2 or final.
Comment 9 Patrice Dumas 2008-10-13 11:20:54 EDT
(In reply to comment #8)
> I am out of time, departing in less than 18 hours to a conference where I need
> this media.  Media is already spun.  No time to redo it.
> 
> This will be released as "rc1".  Please discuss with Peter to make further
> improvements to this package for rc2 or final.

No problem. As long as my comments are taken into account, this may
be later.
Comment 10 Peter Scheie 2008-10-18 20:43:55 EDT
This package is only used on the K12Linux Live image and only exists to make LTSP easier to get working.  So, while it might generically be about binding the virtual bridge to an interface, shifting the focus/name to that would only confuse the very people for whom it is intended, which is new users of K12Linux.

(BTW, is it okay to address multiple comments in one message, or should I create a separate comment for each point raised?)
Comment 11 Patrice Dumas 2008-10-19 04:44:03 EDT
(In reply to comment #10)
> This package is only used on the K12Linux Live image and only exists to make
> LTSP easier to get working.  So, while it might generically be about binding
> the virtual bridge to an interface, shifting the focus/name to that would only
> confuse the very people for whom it is intended, which is new users of
> K12Linux.

I haven't proposed to make that package generic, but to explain better
what it does. I have proposed specific changes and in these changes the
focus is still on ltsp, but there is also an explanation of what the package
does.

> (BTW, is it okay to address multiple comments in one message, or should I
> create a separate comment for each point raised?)

You can address multiple comments in one message.
Comment 12 Peter Scheie 2008-11-08 14:16:43 EST
Calling xdg-open to display the file does not work.  It may have to do with the user ID in live image being generated on the fly (?).
Comment 13 Warren Togami 2008-11-08 15:31:37 EST
> It may have to do with the user ID in live image being generated on the fly (?).

Huh?  No.  xdg-open just was not designed with this in mind, to open local URL's without a protocol.
Comment 15 Peter Scheie 2008-11-16 22:09:08 EST
New version at
http://petre.homedns.org/k12linux/k12linux-quick-start-guide-0.0.3-1.fc9.src.rpm

Note name change to match new title for document.  Covers configuring network for LTSP on LiveUSB & hard drive.  Includes 'rpm -e' command to remove the package so it doesn't appear on user desktops if desired (and no longer suggests just removing the file from /etc/skel).
Comment 16 Patrice Dumas 2008-12-04 09:55:00 EST
you should use xdg-open in the .desktop instead of firefox.

There is a Requires on bluecurve-icon-theme missing for the icons.

You should do a proper package of this documentation, with a proper home page and download.

My comments stating that this package is not that much ltsp specific are, in my opinion, still valid, but anyway, I won't block on that.

There are license informations missing for the html, there should be a license notice at the beginning of the file (images are screen capture, I don't know what license cover them, but it is not an issue).
Comment 17 Warren Togami 2008-12-04 11:22:26 EST
Hmm xdg-open failed in earlier testing on F9, but it seems to work for local HTML files now on F10.  In the past xdg-open couldn't handle local files because it depended on prefixes like "http://".  I would prefer to stick with "firefox" for now to ensure that it actually works.
Comment 18 Patrice Dumas 2008-12-04 11:42:39 EST
(In reply to comment #17)
> Hmm xdg-open failed in earlier testing on F9, but it seems to work for local
> HTML files now on F10.  In the past xdg-open couldn't handle local files
> because it depended on prefixes like "http://".  

Which past? xdg-open is a simple script, and it will fallback to firefox anyway.

Now, maybe one of the framework xdg-open may use didn't worked (gnome-open, kfmclient, exo-open or mimeopen), without clearly erroring such that the BROWSERS list wasn't used, but this is not, in my opinion, a reason not to use xdg-open. 

> I would prefer to stick with
> "firefox" for now to ensure that it actually works.

I don't think it is wise to workaround xdg-open bugs that way; If xdg-open doesn't work it should be investigated, but using firefox, is, in my opinion, an error.
Comment 19 Warren Togami 2008-12-04 11:53:39 EST
That is an unproductive hard-line stance.  I will not risk this particular launcher not working when it is crucial to the entire point of this spin.  A future version will switch to xdg-open only after we've made 100% sure that it is reliable.
Comment 20 Patrice Dumas 2008-12-05 05:03:56 EST
(In reply to comment #19)
> That is an unproductive hard-line stance.  I will not risk this particular
> launcher not working when it is crucial to the entire point of this spin.  A
> future version will switch to xdg-open only after we've made 100% sure that it
> is reliable.

xdg-open is reliable, it is a plain shell script. In gnome it does

[ x"$GNOME_DESKTOP_SESSION_ID" != x"" ] && gnome-open "$1"

So if it doesn't work, it means that this doesn't work, and if it doesn't work, the desktop is certainly screwed.

Now I can accept that you prefer using a plain firefox instead of gnome-open but this has nothing to do with xdg-open.

It is admitedly possible that there is a bug in the xdg-open script itself, but I doubt it.
Comment 21 Peter Scheie 2008-12-07 10:34:43 EST
xdg-open arguably may be reliable, but then gnome-open is not.  gnome-open tries to open the file in abiword, rather than firefox. Abiword balks saying it can't handle the file.  The bottom line is that using xdg-open to launch file the file does not work.
Comment 22 Patrice Dumas 2008-12-08 06:03:09 EST
(In reply to comment #21)
> xdg-open arguably may be reliable, but then gnome-open is not.  gnome-open
> tries to open the file in abiword, rather than firefox. Abiword balks saying it
> can't handle the file.  The bottom line is that using xdg-open to launch file
> the file does not work.

That I can agree with. So ok to use firefox, but don't forget filling bugs to the appropriate components to have that fixed.

Following comments still apply:
There is a Requires on bluecurve-icon-theme missing for the icons.

You should do a proper package of this documentation, with a proper home page
and download.

There are license informations missing for the html, there should be a license
notice at the beginning of the file (images are screen capture, I don't know
what license cover them, but it is not an issue).
Comment 23 Warren Togami 2008-12-15 13:24:29 EST
[Desktop Entry]
Version=1.0
Encoding=UTF-8
Name=K12Linux Quick Start Guide
Type=Application
Terminal=false
Icon[en_US]=/usr/share/icons/Bluecurve/48x48/filesystems/gtk-network.png
Name[en_US]=K12Linux Quick Start Guide
Exec=firefox /usr/share/ltsp-server-livesetupdocs/k12linux-quick-start-guide.html
Icon=/usr/share/icons/Bluecurve/48x48/filesystems/gtk-network.png
GenericName[en_US]=

The pathin the Exec line is wrong.  It was renamed.
Comment 24 Warren Togami 2008-12-16 22:57:54 EST
I am soon ready to spin K12Linux F10 RC1.  This package needs to be finished quickly.

Action items?
1) Fix the path in the desktop file.
2) Got your fedorapeople.org account straightened out?  Upload your versioned tarballs into a permanent location in a directory there and refer to it from your spec file.  Source0: http://......./something.tar.bz2
Comment 25 Patrice Dumas 2008-12-17 04:45:49 EST
Also, it seems to me that gtk-network is a link to  network-workgroup.png
so you'd better use the generic name. Also I don't know if .desktop files for files at the desktop follow the same specification than .desktop in menu, but if it is so, Icon could be

Icon=network-workgroup

In that case you could avoid the dependency on a precise theme, and
either have a dependency on a generic virtual provides, if one exists
or no dependency at all since a theme should be installed.
Comment 26 Patrice Dumas 2008-12-17 04:47:04 EST
Also is
Icon[en_US]=/usr/share/icons/Bluecurve/48x48/filesystems/gtk-network.png
really useful?
Comment 27 Peter Scheie 2008-12-17 09:09:03 EST
gtk-network.png is a link to /usr/share/icons/Bluecurve/48x48/apps/icon-network-systems.png.  I specifically chose that icon because it looks a bit like one large computer connected to two smaller computers, somewhat akin to an LTSP server and its clients.  I rejected other network icons because they don't convey the idea of thin clients.
Comment 28 Peter Scheie 2008-12-17 09:32:15 EST
Icon[en_US]=/usr/share/icons/Bluecurve/48x48/filesystems/gtk-network.png does seem redundant, as does Name[en_US]=K12Linux Quick Start Guide.  I think those were automatically added when I just created the icon on the desktop.  Will remove.
Comment 29 Peter Scheie 2008-12-17 10:09:48 EST
-Fixed broken path in .desktop file.  
-Removed Icon[en_US] and Name[en_US] lines in .desktop because they were redundant.
-Added CC license to .html file as a comment.
-Changed URL to peterscheie.fedorapeople.org/k12linux/quick-start-guide/.
-Uploaded .tar.bz2 file to peterscheie.fedorapeople.org
-Incremented revision number to 0.0.4.
-Uploaded RPM and SRPM files to peterscheie.fedorapeople.org.
Comment 30 Patrice Dumas 2008-12-17 11:48:17 EST
(In reply to comment #27)
> gtk-network.png is a link to
> /usr/share/icons/Bluecurve/48x48/apps/icon-network-systems.png.  I specifically
> chose that icon because it looks a bit like one large computer connected to two
> smaller computers, somewhat akin to an LTSP server and its clients.  I rejected
> other network icons because they don't convey the idea of thin clients.

Ok. I have checked that only Bluecurve provides this icon:

# repoquery -f '*icon-network-systems.png'
bluecurve-icon-theme-0:8.0.2-2.fc10.noarch
bluecurve-icon-theme-0:8.0.2-2.fc10.noarch

Still a bluecurve-icon-theme requires missing.

Also did you try with only

Icon=icon-network-systems
Comment 31 Peter Scheie 2008-12-18 11:24:33 EST
Tried Icon=icon-network-systems, but it doesn't work, I just get a generic document icon (looks like a piece of paper).
Comment 32 Patrice Dumas 2008-12-18 11:31:53 EST
All the points I raised are fixed, except for the missing bluecurve-icon-theme requires.

Please post a link to your srpm during review, it is easier, for example to start wget from a cut and paste in a mail or similar.
Comment 33 Patrice Dumas 2008-12-18 12:01:54 EST
License is not right, should be CC-BY-SA.

Minor remarks:

I think that changelog is more readable with one empty line between each date, like

* Thu Nov 13 2008 Peter Scheie <peter@scheie.homedns.org> - 0.0.2
- Updated screencapture image for step 8 to one in which the interface in question is highlighted.

* Sun Oct 12 2008 Warren Togami <wtogami@redhat.com> - 0.0.1
- initial package

Also you can use %{version} in the Source url to avoid changing it each release, like
Source0:        http://peterscheie.fedorapeople.org/k12linux/quick-start-guide/k12linux-quick-start-guide-%{version}.tar.bz2
Comment 34 Peter Scheie 2008-12-18 14:01:23 EST
Tried Icon=icon-network-systems, but it doesn't work, I just get a generic document icon (looks like a piece of paper).
Comment 35 Peter Scheie 2008-12-18 14:16:44 EST
http://peterscheie.fedorapeople.org/k12linux/quick-start-guide/k12linux-quick-start-guide-0.0.5-1.fc9.src.rpm

I fixed the missing Requires; added a blank space between revisions in the changelog; removed a superfluous GenericName[en_US]= in .desktop file.

Why should the license by CC-BY-SA instead of CC-BY-SA-AT?

Will add %{version} for URL in next release.
Comment 36 Patrice Dumas 2008-12-18 15:12:33 EST
(In reply to comment #35)
> http://peterscheie.fedorapeople.org/k12linux/quick-start-guide/k12linux-quick-start-guide-0.0.5-1.fc9.src.rpm
> 
> I fixed the missing Requires; added a blank space between revisions in the
> changelog; removed a superfluous GenericName[en_US]= in .desktop file.

Why a specific version for the bluecurve-icon-theme requires?

> Why should the license by CC-BY-SA instead of CC-BY-SA-AT?

I said that based on:
http://fedoraproject.org/wiki/Licensing#Good_Licenses_3
But I may be wrong

Rpmlint says:
k12linux-quick-start-guide.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)
k12linux-quick-start-guide.noarch: W: no-documentation
k12linux-quick-start-guide.noarch: W: non-conffile-in-etc /etc/skel/Desktop/K12Linux Quick Start Guide.desktop
k12linux-quick-start-guide.noarch: W: incoherent-version-in-changelog 0.0.5 ['0.0.5-1.fc10', '0.0.5-1']

You can fix the first of you want.

You have to fix the 4rth item, version in changelog must be 0.0.5-1.

The 2 other items are not a problem.
Comment 37 Warren Togami 2008-12-19 23:12:52 EST
> Requires:       bluecurve-icon-theme = 8.0.2

Yes, please remove the version.  Package name alone is fine.
Comment 38 Peter Scheie 2008-12-21 15:45:04 EST
http://peterscheie.fedorapeople.org/k12linux/quick-start-guide/k12linux-quick-start-guide-0.0.6.tar.bz2

- Version number for bludecurve-icon-theme removed in spec.
- The license in the html is already correct.  CC-BY-SA is just the short version of Creative Commons Attribution ShareAlike. 
- Changed the version number in the changelog to 0.0.6-1.
- In the html file changed step #10 and corresponding screenshot.
- Changed tab(s) in spec to spaces.
Comment 39 Patrice Dumas 2008-12-21 16:39:06 EST
The license in the spec file is incorrect, it should be

License:        CC-BY-SA

it is

License:        GPLv2+

If you change that t is APPROVED.
Comment 40 Peter Scheie 2008-12-21 19:52:06 EST
http://peterscheie.fedorapeople.org/k12linux/quick-start-guide/k12linux-quick-start-guide-0.0.7-1.fc9.src.rpm

Fixed license in spec.  Changed one of the screenshot files in docs/.
Comment 41 Patrice Dumas 2008-12-22 03:21:30 EST
Right. Still approved.

In the %changelog, the % in %{version} should be escaped like %%{version}

The spec file is used for changelog entries that are in fact for upstream, like:
- Used a different screenshot for step #10 in html which more clearly shows
  the device names.  Deleted the sentence that said to close the
  window because it was replaced by the File-Save verbage added in
- Changed Step 10 in html doc to use File-Save rather quit, save; replaced
  corresponding screenshot.
- Reworked opening section to cover running from LiveUSB and from hard disk, since
  document is mostly about network config to enable LTSP support.
- Added link to Fedora Installation Guide in reference to adding user IDs.
- Changed name and all allusions from README and ltsp-server-livesetupdocs to
  k12linux-quick-start-guide.
- Changed screenshots so they show 'K12Linux Quick Start Guide' on desktop instead
  of 'README LTSP Server Setup'.

They should be in a NEWS file or Changelog or the like. Not a big deal since the separation between the spec file and the tarball is quite artificial anyway.
Comment 42 Warren Togami 2008-12-22 05:57:51 EST
> They should be in a NEWS file or Changelog or the like. Not a big deal since
> the separation between the spec file and the tarball is quite artificial
> anyway.

I think it is entirely unimportant to do this... but Peter's decision as maintainer.  I plan on importing the tarball into a bzr repo soon.
Comment 43 Warren Togami 2008-12-28 04:33:00 EST
One important correction to make.  The text still says rpm -e of the old package name.  I think we can release after the docs are reviewed one more time and corrected.
Comment 44 Peter Scheie 2008-12-29 20:31:56 EST
New Package CVS Request
=======================
Package Name: k12linux-quick-start-guide
Short Description: Document on how to enable ltsp services on k12linux.
Owners: peterscheie
Branches: F-9 F-10
InitialCC: wtogami
Comment 45 Kevin Fenzi 2008-12-31 00:54:49 EST
cvs done.
Comment 46 Patrice Dumas 2009-01-21 08:04:22 EST
Is this built?
Comment 47 Peter Scheie 2009-01-21 19:37:23 EST
I don't know how to answer the question.  I've run make build & make update for a version 0.0.9 and 0.0.10 in the past couple of weeks; do I need to do something beyond that?  Another change was subsequently necessary;  Warren said he built it, but that it wasn't in the repo.  I don't understand what I should do next.
Comment 48 Warren Togami 2009-01-21 22:30:43 EST
http://kojipkgs.fedoraproject.org/packages/k12linux-quick-start-guide
It is built.

http://admin.fedoraproject.org/updates/
You have to fill out the form to get it into the repository for F-9 and F-10.  Skip testing, as there is nothing left to test in this case.  (Normally you would put a package into testing first.)

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