Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=428586
Summary: Review Request: ldm - LTSP Display Manager Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: wtogami@redhat.com QAContact: extras-qa@fedoraproject.org CC: fedora-package-review@redhat.com,notting@redhat.com
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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
wtogami@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |188611 nThis| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
a.badger@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |a.badger@gmail.com Status|NEW |ASSIGNED
------- Additional Comments From a.badger@gmail.com 2008-01-15 07:54 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
------- Additional Comments From a.badger@gmail.com 2008-01-15 07:58 EST ------- Created an attachment (id=291708) --> (https://bugzilla.redhat.com/attachment.cgi?id=291708&action=view) 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
wtogami@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
------- Additional Comments From wtogami@redhat.com 2008-01-16 23:53 EST ------- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
a.badger@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
------- Additional Comments From a.badger@gmail.com 2008-01-17 09:23 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
------- Additional Comments From a.badger@gmail.com 2008-01-17 09:25 EST ------- Created an attachment (id=291998) --> (https://bugzilla.redhat.com/attachment.cgi?id=291998&action=view) Fix many warnings issued by gcc.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
------- Additional Comments From wtogami@redhat.com 2008-02-09 13:52 EST ------- http://wtogami.livejournal.com/23144.html Description of blocker in LDM that needs to be fixed before Fedora 9.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: ldm - LTSP Display Manager
https://bugzilla.redhat.com/show_bug.cgi?id=428586
wtogami@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE Flag| |fedora-cvs+
------- Additional Comments From wtogami@redhat.com 2008-03-10 01:24 EST ------- OK well this is in F-9, so closing.
package-review@lists.fedoraproject.org