Bug 429006 - Review Request: gnome-subtitles - Subtitle editor for Gnome
Summary: Review Request: gnome-subtitles - Subtitle editor for Gnome
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Alexander Kahl
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 439453
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-01-16 19:06 UTC by Julian Sikorski
Modified: 2008-05-19 19:20 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-05-19 19:20:59 UTC
Type: ---
Embargoed:
fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to install executable files correctly (1.56 KB, patch)
2008-03-24 12:04 UTC, Alexander Kahl
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
GNOME Bugzilla 524765 0 None None None Never

Description Julian Sikorski 2008-01-16 19:06:10 UTC
Spec URL: http://www.belegdol.republika.pl/rpmstuff/gnome-subtitles.spec
SRPM URL: http://www.belegdol.republika.pl/rpmstuff/gnome-subtitles-0.7.2-1.fc8.src.rpm
Description: Gnome Subtitles is a subtitle editor for the GNOME desktop. It supports the most common text-based subtitle formats and allows for subtitle editing, translation and synchronization.

Builds fine in mock fc9/x86_64. rpmlint output:
gnome-subtitles.x86_64: W: non-conffile-in-etc /etc/gconf/schemas/gnome-subtitles.schemas
gnome-subtitles.x86_64: W: unstripped-binary-or-object /usr/lib64/gnome-subtitles/libgstreamer_playbin.so
gnome-subtitles-debuginfo.x86_64: E: empty-debuginfo-package

Please advise, as this is my first mono package.

Comment 1 Alexander Kahl 2008-03-23 13:59:29 UTC
Seems to be an interesting application. Going to review this.

Comment 2 Alexander Kahl 2008-03-24 12:03:38 UTC
Package Review
==============

* rpmlint output:
  gnome-subtitles.x86_64: W: non-conffile-in-etc
/etc/gconf/schemas/gnome-subtitles.schemas
OK according to
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-6c2101d8f810cc95c677c8c27f43573b0bc23cb1

-------------------------------------------------------------------------------
  gnome-subtitles.x86_64: W: unstripped-binary-or-object
  /usr/lib64/gnome-subtitles/libgstreamer_playbin.so
  gnome-subtitles-debuginfo.x86_64: E: empty-debuginfo-package

The first one is caused by the fact the shared object, the assemblies and the
mono executable are installed as *_DATA defined in Makefile.am and
src/Makefile.am:
-rw-r--r-- /usr/lib64/gnome-subtitles/GStreamerPlaybin.dll
-rw-r--r-- /usr/lib64/gnome-subtitles/gnome-subtitles.exe
-rw-r--r-- /usr/lib64/gnome-subtitles/libgstreamer_playbin.so
-rw-r--r-- /usr/lib64/gnome-subtitles/sublib.dll

Thus find-debuginfo.sh cannot find anything since it searches for executable
files after install only which causes the second error, an empty debuginfo
package.
There is an example of how to properly handle mono applications with
automake at
http://www.mono-project.com/Guidelines:Application_Deployment#Auto-tools
where they use *_SCRIPTS to have automake properly handle mono
executables. I've attached a patch made according to the guide that
fixes the issue, you can use it if you want, just an additional
autoreconf call needs to be added in build before configure.
Please also inform upstream about the issue and offer them the patch.
-------------------------------------------------------------------------------

* The package is named according to the Package Naming Guidelines
* The spec file name matches the base package
* Fedora approved license: GPLv2+
* License field in the package spec file matches the actual license
* COPYING included
* The spec file is written in American English
* The spec file is legible
* The sources used to build the package matches the upstream source:
SHA1 0c484af7934a6a2fbed41139221489fbcd687000  gnome-subtitles-0.7.2.tar.gz
* Builds locally on dist-f8 x86_64
* BuildRequires look sane
* Proper locale handling
-------------------------------------------------------------------------------
E Package does not build for ppc64:
http://koji.fedoraproject.org/koji/taskinfo?taskID=527292
mono-devel is not available for ppc64. 
- Please add an ExcludeArch: ppc64
- After this package has been approved, please open a new report (with component
  gnome-subtitles) which tells that gnome-subtitles does not support ppc64 and
  make the bug block FE-ExcludeArch-ppc64.
--------------------------------------------------------------------------------
* No libraries in dynamic linker's default paths
* No relocatable libraries
* Package owns all directories it creates
* No duplicates in %files
E File permissions are not sane, see above
* %clean starts with build root clean
-------------------------------------------------------------------------------
E Consistent use of macros
Please replace:
- rm with %{__rm}
- make with %{__make}
-------------------------------------------------------------------------------
* The package contains code
* Documentation not large, no -doc subpackage needed
* No header files
* No static libs
* No pkgconfig files
* No suffixed library files
* No -devel package
* No libtool archives
* Desktop file handled properly (BuildRequires, %install, vendor id)
* Package does not own files or directories already owned by other packages
* %install starts with build root clean
* All filenames are valid UTF-8


Additional Checks
=================
* Latest version is being packaged: 0.7.2
* Dist tag is present                                                          
          
* Group tag is valid                                                           
          
* Build root is correct:                                                       
          
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)                 
          
* Scriptlets look sane                                                         
          
E Does not build in mock (due to ppc64, see above)
* GConf schema files handled properly:
  - %configure --disable-schemas-install present
  - GConf2 in Requires({pre,post,preun})
  - Scriplets in %pre, %post and %preun present
* Scrollkeeper documentation handled properly:
  - scrollkeeper in BuildRequires, Requries(post{,un})
  - Scriplets in %post and %postun present
* Requires is complete, all dependencies resolved automatically


Mono-specific Checks
====================
* Assembly location is correct:
%{_libdir}/%{name}
* No global assemblies are registered with gacutil
* Package builds without source version of mono
* No pkgconfig files
E debuginfo error (see above)
* No prebuilt assemblies distributed
* No library distributed
W Distributing .DLLs from other projects
-------------------------------------------------------------------------------
gnome-subtitles includes code from
SubLib (http://sublib.sf.net)
NCharDet (http://www.conceptdevelopment.net/Localization/NCharDet)
Fuse Media Centre (http://sf.net/projects/fusemc)

If I get
http://fedoraproject.org/wiki/Packaging/Mono#head-6635c736011474fcbbc900285de0d9b0977cde1b
right, this is considered poor practice, if not even a
blocker. However since upstream is not building those libraries into
separate assemblies but directly into in their source, thus taking
over responsibility to fix everything themselves, I consider this to
be OK.
-------------------------------------------------------------------------------
* _libdir not redefined
* Package not noarch

Comment 3 Alexander Kahl 2008-03-24 12:04:44 UTC
Created attachment 298883 [details]
Patch to install executable files correctly

Comment 4 Alexander Kahl 2008-03-24 18:27:32 UTC
I've made a revision of the gnome-subtitles tarball and determined the sublib
library is directly included from its upstream project with minor modifications
in the build files but not the actual source, distributed as distinct assembly
sublib.dll. Exactly as told at
http://fedoraproject.org/wiki/Packaging/Mono
this is a showstopper and must be fixed, i.e. please create a new Review Request
for sublib blocking this bug and patch gnome-subtitles to load sublib from the
GAC, the URL above also contains instructions how to register assemblies in the
GAC properly.
Finally upstream should be informed and convinced that the inclusion of sublib
is generally considered bad practice.

Can you handle this?

Comment 5 Julian Sikorski 2008-03-26 11:52:13 UTC
I was away, hence the delayed response. I'll try to address all comments ASAP.

Comment 6 Julian Sikorski 2008-03-28 08:46:20 UTC
I'm having problems with packaging sublib. The makefile does not have a working
make install section, so I'm nout sure if I need to install anything except
sublib.dll

Comment 7 Alexander Kahl 2008-03-28 09:26:49 UTC
I've also checked the tarball and indeed, make install does nothing. Just copy
the sublib.dll to %{_libdir}/mono/%{name} and follow the instructions on
http://fedoraproject.org/wiki/Packaging/Mono how to create the GAC files and
install them along sublib.dll.
The situation is worth creating an additional bug report for upstream since
they're claiming to provide a make-docs target which doesn't exist and make
install should always work.

Comment 8 Julian Sikorski 2008-03-28 09:37:53 UTC
New release:
http://belegdol.fedorapeople.org/gnome-subtitles-0.7.2-2.fc8.src.rpm
http://belegdol.fedorapeople.org/gnome-subtitles.spec

Changes:
- Added patch fixing empty debuginfo issue
- Added ExcludeArch: ppc64
- Replaced make and rm invocations with macros

The sublib problem is still unsolved, it might take longer as I hit another
problem - gacutil complains about non-strong name or something like that.

Comment 9 Alexander Kahl 2008-03-28 14:06:08 UTC
Could you please create a review request for sublib as described earlier? I'm
sure it's easier for me and others to help that way. Thank you

Comment 10 Alexander Kahl 2008-04-01 22:20:14 UTC
Adding external reference for the report you've created at bugzilla.gnome.org.

Do you want me to file the other bug demanding the exclusion of sublib from
gnome-subtitles?

Comment 11 Julian Sikorski 2008-04-01 22:31:47 UTC
Well, I don't think exclusion is necessary, but it should be possible to build
against a system lib. Keep in mind that both pieces of software seem to be
developed by the same people.

Comment 12 Julian Sikorski 2008-04-01 22:32:33 UTC
And yes, please file a bug report. I'm a bit busy these days.

Comment 13 Matthias Saou 2008-05-17 20:10:40 UTC
Note that 0.8 has been released. Is this a version which now uses the external
sublib by default? That would be great news!

Regarding the latest spec file from the 0.7.2-2 package :
- The man page is included with the .gz compression explicitly. Please fix,
since this is configurable at rpm build time, so using foo.1* instead of
foo.1.gz is less likely to break, thus better practice.
- The COPYING file *must* be included as %doc as required by the packaging
guidelines. The other documentation files *should* be too.

Comment 14 Julian Sikorski 2008-05-17 20:31:26 UTC
New release:
Spec URL: http://belegdol.fedorapeople.org/gnome-subtitles.spec
SRPM URL: http://belegdol.fedorapeople.org/gnome-subtitles-8-1.fc9.src.rpm

Changes:
- Updated to 0.8
- Dropped upstreamed patches
- Dropped SMP build, seems to cause problems
- Added missing %%doc
- Added wildcard to manpage install location

Comment 15 Julian Sikorski 2008-05-18 22:00:18 UTC
Gah, forgot about this one:
Spec URL: http://belegdol.fedorapeople.org/gnome-subtitles.spec
SRPM URL: http://belegdol.fedorapeople.org/gnome-subtitles-0.8-2.fc9.src.rpm

Changes:
- Added sublib-devel to BuildRequires

Comment 16 Alexander Kahl 2008-05-18 23:41:33 UTC
@Matthias:
Thank you for the help :) I've tried to reach you directly via email before
several times but never got a reply.

@Julian:

Package Review (pt. 2)
=====================
Only re-checking rpmlint, errors from last review and diff between old and new
spec.

* rpmlint output:
  gnome-subtitles.x86_64: W: non-conffile-in-etc
/etc/gconf/schemas/gnome-subtitles.schemas
  OK, see last review
-------------------------------------------------------------------------------
* The sources used to build the package matches the upstream source:
SHA1 14fe9eabf4288fc23072988f4423b5f447642352  gnome-subtitles-0.8.tar.gz
* Builds locally on dist-f9 x86_64
* BuildRequires look sane
* Builds on all Fedora supported architectures but ppc64 (excluded)
* Package owns all directories it creates
* No duplicates in %files
* File permissions are sane

Mono-specific Checks
====================
* Not distributing .DLLs from other projects


As soon as sublib is in and its bug closed, I'll give you approval.

Comment 17 Alexander Kahl 2008-05-19 17:36:32 UTC
Good work!

==============================
This package is APPROVED by me
==============================

Comment 18 Julian Sikorski 2008-05-19 17:48:49 UTC
New Package CVS Request
=======================
Package Name: gnome-subtitles
Short Description: Subtitle editor for gnome
Owners: belegdol
Branches: F-8 F-9
InitialCC: 
Cvsextras Commits: yes

Comment 19 Kevin Fenzi 2008-05-19 18:47:08 UTC
cvs done.


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