Bug 507943 - Review Request: moblin-gtk-engine - GTK engine for Moblin
Review Request: moblin-gtk-engine - GTK engine for Moblin
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraMoblin20
  Show dependency treegraph
 
Reported: 2009-06-24 15:40 EDT by Peter Robinson
Modified: 2009-07-18 03:59 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-18 03:59:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Comment 1 Julian Aloofi 2009-07-13 09:50:45 EDT
Hi, here is a review of your package:

rpmlint output of the built RPM returns one warning:
moblin-gtk-engine.i586: W: file-not-utf8 /usr/share/doc/moblin-gtk-engine-0.2.4/README


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. NEEDSWORK
- You could leave some space inbetween the spec file sections

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
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 sources used to build the package must match the upstream source, as
provided in the spec URL. NEEDSWORK
- Where did you get the source code? There doesn't seem to be an upstream 
project page, but the source code must come from somewhere.

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. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package
that owns the directory. NEEDSWORK
The package does not own the following directories:
/usr/share/themes/Moblin-Netbook/
usr/share/themes/Moblin-Netbook/gtk-2.0
usr/share/themes/Moblin-Netbook/metacity-1
It should own them

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
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. OK
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. OK
MUST: Desktop files are installed properly. N/A
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
Comment 2 Susi Lehtola 2009-07-13 09:58:57 EDT
Julian is not sponsored yet, taking over review.
Comment 3 Susi Lehtola 2009-07-13 10:27:01 EDT
rpmlint output:
moblin-gtk-engine.x86_64: W: file-not-utf8 /usr/share/doc/moblin-gtk-engine-0.2.4/README
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- You need to fix this http://fedoraproject.org/wiki/Packaging_tricks#Convert_encoding_to_UTF-8
(use the version that preserves the timestamp)


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
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

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSWORK
- Source URL is missing.
http://fedoraproject.org/wiki/Packaging/SourceURL

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 lost during install. Adding INSTALL="install -p" as argument to make install should do the trick.

MUST: A package must own all directories that it creates or require the package that owns the directory. NEEDSWORK
- Package should own %{_datadir}/themes/Moblin-Netbook/ as it doesn't seem to be provided by any package. (%{_datadir}/themes is owned by the filesystem package)

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
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. NEEDSWORK
- Missing WORK.

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. OK
MUST: Desktop files are installed properly. N/A
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
Comment 4 Peter Robinson 2009-07-13 13:08:12 EDT
(In reply to comment #3)
> rpmlint output:
> moblin-gtk-engine.x86_64: W: file-not-utf8
> /usr/share/doc/moblin-gtk-engine-0.2.4/README
> 3 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> - You need to fix this
> http://fedoraproject.org/wiki/Packaging_tricks#Convert_encoding_to_UTF-8
> (use the version that preserves the timestamp)

FIXED

> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. NEEDSWORK
> - Source URL is missing.
> http://fedoraproject.org/wiki/Packaging/SourceURL

FIXED: Updated to the moblin git url for source tarballs. Updated to the latest release as well. I thought I'd updated this but must have missed it.

> MUST: Optflags are used and time stamps preserved. NEEDSWORK
> - Time stamps are lost during install. Adding INSTALL="install -p" as argument
> to make install should do the trick.

I couldn't find anything about this in packaging guidelines.

> MUST: A package must own all directories that it creates or require the package
> that owns the directory. NEEDSWORK
> - Package should own %{_datadir}/themes/Moblin-Netbook/ as it doesn't seem to
> be provided by any package. (%{_datadir}/themes is owned by the filesystem
> package)

FIXED

> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSWORK
> - Missing WORK.

Not sure what's missing here. There's no WORK file. I've added the NEWS file.

Updates here
SPEC: http://pbrobinson.fedorapeople.org/moblin-gtk-engine.spec
SRPM: http://pbrobinson.fedorapeople.org/moblin-gtk-engine-0.3.0-1.fc11.src.rpm
Comment 5 Peter Robinson 2009-07-13 14:35:14 EDT
koji build here http://koji.fedoraproject.org/koji/taskinfo?taskID=1471743
Comment 6 Susi Lehtola 2009-07-13 15:20:03 EDT
(In reply to comment #4)
> > MUST: The sources used to build the package must match the upstream source, as
> > provided in the spec URL. NEEDSWORK
> > - Source URL is missing.
> > http://fedoraproject.org/wiki/Packaging/SourceURL
> 
> FIXED: Updated to the moblin git url for source tarballs. Updated to the latest
> release as well. I thought I'd updated this but must have missed it.

If you're using a git tarball, then you need to adjust the version and release accordingly.
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages

Judging from the URL this seems to be a stable release. However the git part makes me think this is a daily snapshot. If it is, then you should get the source from git:
http://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

> > MUST: Optflags are used and time stamps preserved. NEEDSWORK
> > - Time stamps are lost during install. Adding INSTALL="install -p" as argument
> > to make install should do the trick.
> 
> I couldn't find anything about this in packaging guidelines.

It's sort of implicitly assumed in
https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

I sent a request to add the mention of it to the FPC.

> > MUST: All relevant items are included in %doc. Items in %doc do not affect
> > runtime of application. NEEDSWORK
> > - Missing WORK.
> 
> Not sure what's missing here. There's no WORK file. I've added the NEWS file.

Ugh. That's what I meant :)
Comment 7 Peter Robinson 2009-07-13 15:38:53 EDT
> If you're using a git tarball, then you need to adjust the version and release
> accordingly.
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
> 
> Judging from the URL this seems to be a stable release. However the git part
> makes me think this is a daily snapshot. If it is, then you should get the
> source from git:
> http://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

Its a proper release but the moblin guys don't currently release tarballs so only do it via the git method (very annoying but getting more common now days). Because they are tagged releases the tarballs are able to be recreated by downloading them so they don't require the pre-release tagging (like for eg the geoclue package I maintain).

> > > MUST: Optflags are used and time stamps preserved. NEEDSWORK
> > > - Time stamps are lost during install. Adding INSTALL="install -p" as argument
> > > to make install should do the trick.
> > 
> > I couldn't find anything about this in packaging guidelines.
> 
> It's sort of implicitly assumed in
> https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
> 
> I sent a request to add the mention of it to the FPC.

OK, I'll update it when its in the packaging guidelines. I don't see that it should block the review as I only see it there when copying files within the install section. The only one that does that is the utf-8 stuff which preserves it as per the details you provided above.

> > > MUST: All relevant items are included in %doc. Items in %doc do not affect
> > > runtime of application. NEEDSWORK
> > > - Missing WORK.
> > 
> > Not sure what's missing here. There's no WORK file. I've added the NEWS file.
> 
> Ugh. That's what I meant :)  

:-)
Comment 8 Susi Lehtola 2009-07-14 05:00:32 EDT
(In reply to comment #7)
> > > > MUST: Optflags are used and time stamps preserved. NEEDSWORK
> > > > - Time stamps are lost during install. Adding INSTALL="install -p" as argument
> > > > to make install should do the trick.
> > > 
> > > I couldn't find anything about this in packaging guidelines.
> > 
> > It's sort of implicitly assumed in
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
> > 
> > I sent a request to add the mention of it to the FPC.
> 
> OK, I'll update it when its in the packaging guidelines. I don't see that it
> should block the review as I only see it there when copying files within the
> install section. The only one that does that is the utf-8 stuff which preserves
> it as per the details you provided above.

The purpose is to have the same time stamps on files that are not architecture specific in order to avoid trouble with multilib/multiarch packages. Adding the INSTALL="install -p" doesn't harm anything.

I guess I can't flunk this review on that grounds, so the package is

APPROVED
Comment 9 Peter Robinson 2009-07-14 05:18:51 EDT
> > OK, I'll update it when its in the packaging guidelines. I don't see that it
> > should block the review as I only see it there when copying files within the
> > install section. The only one that does that is the utf-8 stuff which preserves
> > it as per the details you provided above.
> 
> The purpose is to have the same time stamps on files that are not architecture
> specific in order to avoid trouble with multilib/multiarch packages. Adding the
> INSTALL="install -p" doesn't harm anything.
> 
> I guess I can't flunk this review on that grounds, so the package is

Thanks. That makes sense (and is good to know) for noarch packages. I'll add it to my notes :-)
Comment 10 Peter Robinson 2009-07-14 05:19:47 EDT
New Package CVS Request
=======================
Package Name:  moblin-gtk-engine
Short Description: GTK engine for Moblin
Owners: pbrobinson
Branches: F-11 F-10
InitialCC:
Comment 11 Susi Lehtola 2009-07-14 05:29:51 EDT
(In reply to comment #9)
> > > OK, I'll update it when its in the packaging guidelines. I don't see that it
> > > should block the review as I only see it there when copying files within the
> > > install section. The only one that does that is the utf-8 stuff which preserves
> > > it as per the details you provided above.
> > 
> > The purpose is to have the same time stamps on files that are not architecture
> > specific in order to avoid trouble with multilib/multiarch packages. Adding the
> > INSTALL="install -p" doesn't harm anything.
> > 
> > I guess I can't flunk this review on that grounds, so the package is
> 
> Thanks. That makes sense (and is good to know) for noarch packages. I'll add it
> to my notes :-)  

Not really, more for -devel packages that exist e.g. on both i386 and x86_64 and the two can be installed in the same time. (They contain both architecture independent headers and architecture dependent libraries.)
Comment 12 Jason Tibbitts 2009-07-14 22:32:12 EDT
CVS done.
Comment 13 Peter Robinson 2009-07-18 03:59:53 EDT
Built and in rawhide. Thanks!

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