Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 428586 - Review Request: ldm - LTSP Display Manager
Summary: Review Request: ldm - LTSP Display Manager
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Toshio Ernie Kuratomi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: K12LTSP
TreeView+ depends on / blocked
 
Reported: 2008-01-13 21:18 UTC by Warren Togami
Modified: 2008-03-10 05:24 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-03-10 05:24:25 UTC
a.badger: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)
spec file solving most review issues (deleted)
2008-01-15 12:58 UTC, Toshio Ernie Kuratomi
no flags Details
Fix many warnings issued by gcc. (deleted)
2008-01-17 14:25 UTC, Toshio Ernie Kuratomi
no flags Details | Diff

Description Warren Togami 2008-01-13 21:18:43 UTC
http://togami.com/~warren/fedora/ldm.spec
http://togami.com/~warren/fedora/ldm-0.1-0.20080113.fc8.src.rpm
Description:
LTSP Display Manager
(exhausted... I will add a better description later)

Comment 1 Toshio Ernie Kuratomi 2008-01-15 12:54:54 UTC
Good:
* spec matches the base package.
* License is GPLv2+
* License is included with package.
* Spec is coherent American English
* Package builds and installs on i386
* Not a library

Needswork:
* Is this a pre-release snapshot?  If so the release should be 0.1.20080114.
  [Fix attached]
* Source needs to have instructions for retrieving from upstream (since it's a
  snapshot)
  [Fix attached]
* Not all BuildRequires are listed: Needs gtk2-devel, does not need libtool
  [Fix attached]
* Needs to own %{_datadir}/ldm
  [Fix attached]
* rpmlint output::
    ldm.i386: E: non-executable-script /usr/share/ldm/ldm-script 0644
  This looks like a script that is intended to start and stop services when
  a user logs into the box.  Does it need to be executable?
  [Not fixed]

Notes:
* When upstream releases a tarball of this package it will likely be named ldm2
  (judging from the tarball that make dist creates).  Since the main binary is
  named ldm and we're not likely to package ldm1 ever, I think naming this
  package ldm is fine.
* No locales are currently installed but there are some translations available.
  When those are installed, be sure to include them using %{find_lang} so that
  they are properly marked as language files.
* Although this is a graphical application, it does not need a .desktop file
  because it does not run from a desktop session.  Instead it is used to
  start a desktop session.

I'll attach a new spec file in a moment that addresses most of the issues.

Comment 2 Toshio Ernie Kuratomi 2008-01-15 12:58:04 UTC
Created attachment 291708 [details]
spec file solving most review issues

This solves all review issues except whether /usr/share/ldm/ldm-script schould
be executable and if so, whether it belongs in /usr/share/ or somewhere else
like /usr/libexec.

Comment 3 Warren Togami 2008-01-17 04:53:16 UTC
http://togami.com/~warren/fedora/ldm.spec
http://togami.com/~warren/fedora/ldm-0.1-0.3.20080116.fc8.src.rpm

Built on top of your upstream changes... you made ldm-script and rc.d install
into libexecdir without the trailing /ldm.  Please check that I fixed this properly?

Comment 4 Toshio Ernie Kuratomi 2008-01-17 14:23:30 UTC
APPROVED

Looks good.  I changed the README to reflect your changes to libexecdir and
pushed upstream.

Only one thing left to be fixed in the spec file:

 * Need to have checkout instructions for getting the source for revision control:
     http://fedoraproject.org/wiki/Packaging/SourceURL

   For instance::

     # bzr snapshot::
     #   bzr checkout --lightweight -r 791
http://bazaar.launchpad.net/~ltsp-upstream/ltsp/ldm-trunk
     #   cd ldm-trunk
     #   ./mkdst --test
     # tarball is ldm-%{version}.tar.bz2

Fix this when you checkin and this package is approved.

Note: I also notice that gcc is throwing several valid warnings.  I'm attaching
a patch for you to review that fixes most of them.  I didn't fix any of the::
  "warning: ignoring return value of ‘write’, declared with attribute
warn_unused_result"

because I'm not sure how you want to deal with those.

Comment 5 Toshio Ernie Kuratomi 2008-01-17 14:25:04 UTC
Created attachment 291998 [details]
Fix many warnings issued by gcc.

Comment 6 Warren Togami 2008-02-09 18:52:42 UTC
http://wtogami.livejournal.com/23144.html
Description of blocker in LDM that needs to be fixed before Fedora 9.

Comment 7 Warren Togami 2008-03-10 05:24:25 UTC
OK well this is in F-9, so closing.



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