Bug 513754 - Review Request: moblin-session - Moblin User Experience Startup Scripts
Summary: Review Request: moblin-session - Moblin User Experience Startup Scripts
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FedoraMoblin20
TreeView+ depends on / blocked
 
Reported: 2009-07-25 11:50 UTC by Peter Robinson
Modified: 2009-08-17 18:48 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-17 18:48:30 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Peter Robinson 2009-07-25 11:50:13 UTC
SPEC: http://pbrobinson.fedorapeople.org/moblin-session.spec
SRPM: http://pbrobinson.fedorapeople.org/moblin-session-0.12-1.fc11.src.rpm

Description:
moblin-session manages a Moblin desktop session. It starts up the other core 
Moblin components and handles logout and saving the session.

Comment 1 Gareth John 2009-07-28 00:48:49 UTC
MUST: rpmlint must be run on every package. The output should be posted in the review.

The spec will not build without removing source 1 and source 2 lines. After they were removed rpmlint gives the following errors. 

-------------------------------------------------------------------------
moblin-session.i386: E: no-binary

The package should be of the noarch architecture because it doesn't contain any binaries.

Solution:- Then you can add to SPEC file BuildArchitectures: noarch 
--------------------------------------------------------------------------
W: moblin-session.i386: W: non-conffile-in-etc /etc/xdg/moblin/xinitrc

A non-executable file in your package is being installed in /etc, but is not a configuration file. All non-executable files in /etc should be configuration files. Mark the file as %config in the spec file.

Solution:- under %files section you can add %config /etc/xdg/moblin/xinitrc
----------------------------------------------------------------------------
W: moblin-session.i386: E: non-executable-script /etc/xdg/moblin/xinitrc 0644 /bin/sh

Solution:- will come soon unless you find it. 
----------------------------------------------------------------------------

Also following optional guidelines "* SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [27]" The following the mandatory guidelines you should include this file in the %doc section when released upstream.

Comment 2 Gareth John 2009-07-28 00:58:05 UTC
Mock build log gives this feedback for your final rpmlint error

" moblin-session.i386: E: non-executable-script /etc/xdg/moblin/xinitrc 0644 /bin/sh - - - This text file contains a shebang or is located in a path dedicated for executables, but lacks the executable bits and cannot thus be executed. If the file is meant to be an executable script, add the executable bits, otherwise remove the shebang or move the file elsewhere. "

The easiest option would be to make the file executable however im not sure if this error is ok to ignore or not. I assume removing the shebang is probably not helpful as it is a useful file if anyone wanted to make it executable themselves and use it.

Comment 3 Susi Lehtola 2009-07-28 01:00:27 UTC
I am Gareth's sponsor-to-be and will do the formal review after Gareth has done an informal one, so I'm assigning this bug to myself.

Comment 4 Peter Robinson 2009-07-28 13:07:24 UTC
There is already a COPYING file in the rpm so not sure as to the reason for the last paragraph in comment 1

Updated:
- made package noarch (not sure how I managed to miss that one!)
- set mode to 755 on /etc/xdg/moblin/xinitrc
- removed source 1&2 as they're now obsolete anyway

Not done as incorrect:
- Mark the file as %config in the spec file.

All the rest I think are fixed as part of the other fixes. I also removed the library related components from the spec file as they're not needed either.

SPEC: As before
SRPM: http://pbrobinson.fedorapeople.org/moblin-session-0.12-2.fc11.src.rpm

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1550242

Comment 5 Gareth John 2009-07-28 14:20:29 UTC
MUST: rpmlint must be run on every package. The output should be posted in the review....OK
MUST: The package must be named according to the Package Naming Guidelines....OK
MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption....OK
MUST: The package must meet the Packaging Guidelines....OK
MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines....OK
MUST: The License field in the package spec file must match the actual license....OK
MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc....OK
MUST: The spec file must be written in American English....OK
MUST: The spec file for the package MUST be legible....OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this....OK
MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture....OK
MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line....N/A
MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional....OK
MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden....N/A
MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun....N/A
MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker....N/A
MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory....OK
MUST: A Fedora package must not list a file more than once in the spec file's %files listings....OK
MUST: Permissions on files must be set properly. Executables should be set with executable permissions....OK
MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT)....OK
MUST: Each package must consistently use macros....OK
MUST: The package must contain code, or permissable content....OK
MUST: Large documentation files must go in a -doc subpackage....OK
MUST: If a package includes something as %doc, it must not affect the runtime of the application....N/A
MUST: Header files must be in a -devel package....N/A
MUST: Static libraries must be in a -static package....N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability)....N/A
MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package....N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}....N/A
MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built....N/A
MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation....N/A
MUST: Packages must not own files or directories already owned by other packages....OK
MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT)....OK
MUST: All filenames in rpm packages must be valid UTF-8....OK

As far as I see all guidelines are met hope I havent missed anything Jussi.

Comment 6 Susi Lehtola 2009-07-28 21:08:26 UTC
Note the errors in %build
 + make -j4
 make: execvp: git: Permission denied
 make: Nothing to be done for `all'.

and in %install
+ make install DESTDIR=/builddir/build/BUILDROOT/moblin-session-0.12-2.fc11.x86_64
make: execvp: git: Permission denied

however the makefile doesn't seem to use these for anything. In fact, the only thing the makefile does is it installs three files, so I suggest the following:

 %prep
 %setup -q

 %build
 # Nothing is built

 %install
 rm -rf %{buildroot}
 install -D -p -m 755 moblin-xinitrc %{buildroot}%{_sysconfdir}/xdg/moblin/xninitrc
 install -D -p -m 755 startmoblin %{buildroot}%{_bindir}/startmoblin
 desktop-file-install --dir=%{buildroot}%{_datadir}/applications moblin.desktop

 %files
 %defattr(-,root,root,-)
 %doc COPYING
 %{_sysconfdir}/xdg/moblin/
 %{_bindir}/startmoblin
 %{_datadir}/xsessions/moblin.desktop

(You need to add BR: desktop-file-utils.)

**

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. OK
- Instead of
 %attr(0755,root,root)
in %files I recommend adding
 chmod 755 %{buildroot}%{_sysconfdir}/xdg/moblin/xinitrc
at the end of %install.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license.  OK
- startmoblin declares itself to be GPLv2 only, the other files don't contain a license. The attached COPYING is GPLv2, which should apply to everything.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Time stamps are not preserved, please use method above.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. N/A
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. NEEDSWORK
- Desktop file not installed properly. Please use method given above.

MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

**

Gareth: you missed the desktop file install part. (The time stamp issue is minor, but still one should keep them.)

**

This package seems rather trivial; I wonder why it has not been merged with some other core moblin stuff by upstream...

Comment 7 Peter Robinson 2009-08-10 22:00:32 UTC
I've fixed all the above mentioned. There was an issue with the included moblin.desktop because it used an incorrect value in the file so I've updated in as a separate file and emailed the upstream author (there's no bugzilla component for it).

SRPM: http://pbrobinson.fedorapeople.org/moblin-session-0.12-3.fc11.src.rpm

> This package seems rather trivial; I wonder why it has not been merged with
> some other core moblin stuff by upstream...  

Yes, I agree but I suppose a the session startup manager is a separate component no matter how small.

Comment 8 Peter Robinson 2009-08-11 12:11:34 UTC
SPEC: http://pbrobinson.fedorapeople.org/moblin-session-0.12-4.fc11.src.rpm

A minor fix to the last one to start the mutter WM not metacity.

Comment 9 Susi Lehtola 2009-08-16 09:17:49 UTC
Drop the
 %attr(0755,root,root)
line from %files, as it serves no purpose (the file already has these permissions).

With this modification the package is

APPROVED

Comment 10 Peter Robinson 2009-08-16 10:36:10 UTC
Thanks Jussi. The attr is now dropped from my local version.

New Package CVS Request
=======================
Package Name: moblin-session
Short Description: Moblin User Experience Startup Scripts
Owners: pbrobinson
Branches: F-11
InitialCC:

Comment 11 Kevin Fenzi 2009-08-17 18:18:14 UTC
cvs done.

Comment 12 Peter Robinson 2009-08-17 18:48:30 UTC
Built in koji.


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