According to current packaging guidelines
* if upstream uses <vendor_id>, leave it intact, otherwise use fedora as
<vendor_id>.

usage of dekstop-file-install is correct but use "fedora" instead "Fedora" as
vendor id

Do we need to move following files to fbreader-devel?
/usr/lib/libzltext.so.0.8.6d
/usr/lib/libzlcore.so.0.8.6d

review guidelines says
- 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.

Those files end with the suffix (they are named libzl*.so.*, not libzl*.so).
They are actually required for normal operation, so they are not meant for -devel.

I need to fix the vendor string, and then mess with RPM_OPT_FLAGS. Upstreams
with non-standard Makefiles...

What do you think should be done with alternate GUIs? Currently I make the Qt4
frontend a compile-time option (upstream defaults to GTK). Should I provide an
alternate desktop file for it?

(In reply to comment #2)
> Those files end with the suffix (they are named libzl*.so.*, not libzl*.so).
> They are actually required for normal operation, so they are not meant for -devel.
> 
ok.

> I need to fix the vendor string, and then mess with RPM_OPT_FLAGS. Upstreams
> with non-standard Makefiles...
 yes please. use $RPM_OPT_FLAGS

> 
> What do you think should be done with alternate GUIs? Currently I make the Qt4
> frontend a compile-time option (upstream defaults to GTK). Should I provide an
> alternate desktop file for it?

I have not seen such package in my reviews but seen one which is waiting for
review http://folk.ntnu.no/sindrb/packages/chestnut-dialer.spec
you can have a look at this package. 

Spec URL: http://hircus.org/fedora/fbreader/fbreader.spec
SRPM URL: http://hircus.org/fedora/fbreader/fbreader-0.8.6d-2.fc8.src.rpm

I thought about doing something like what chestnut does, but the annoying thing
is that you have to do some rather manual surgeries on the desktop files --
desktop-file-install does not let you change the content of fields, so in
chestnut's case, if you install both the Qt and GTK GUI packages, both have the
same Name and Comment fields. Would be confusing..

I think I'll stick to using GTK for now, and people who want the Qt interface
can rebuild the SRPM themselves.

Removed the vendor tag, since a lot of apps don't use it anymore (e.g.
rhythmbox). the Guidelines make it seem more mandatory than it actually is.

I think that covered everything -- could you review the updated spec? Thanks!

(In reply to comment #4)
> Spec URL: http://hircus.org/fedora/fbreader/fbreader.spec
> SRPM URL: http://hircus.org/fedora/fbreader/fbreader-0.8.6d-2.fc8.src.rpm
> 
> I thought about doing something like what chestnut does, but the annoying thing
> is that you have to do some rather manual surgeries on the desktop files --
> desktop-file-install does not let you change the content of fields, so in
> chestnut's case, if you install both the Qt and GTK GUI packages, both have the
> same Name and Comment fields. Would be confusing..
> 
> I think I'll stick to using GTK for now, and people who want the Qt interface
> can rebuild the SRPM themselves.
> 
> Removed the vendor tag, since a lot of apps don't use it anymore (e.g.
> rhythmbox). the Guidelines make it seem more mandatory than it actually is.
> 
 But good to follow what current guidelines said. So you should add vendor tag.

> I think that covered everything -- could you review the updated spec? Thanks!

I assumed that I should follow vendor_tag as discussed in one thread
https://www.redhat.com/archives/fedora-devel-list/2007-August/msg01785.html

Weird discussion. Ray Strode argued against vendor_tag, and Rex Dieter admitted
to not liking it very much but insisting that it still be applied.

I've re-added the vendor tag; not changing the release number as this has not
escaped to the wild yet. Thanks!

you missed to install /usr/share/applications/fedora-FBReader.desktop instead to
said to install /usr/share/applications/FBReader.desktop

Remember while using vendor tag installed desktop file always gets prefixed with
vendor tag.

Ah yes, should have retested the change, sorry. Files reuploaded -- the debug
patch is no longer in the SRPM, to suppress rpmlint complaining that it's not
applied; it's at

http://hircus.org/fedora/fbreader/fbreader-debug.patch

Thanks.

before final review,
1) Do you think package name and its binary name should be unique?
  e.g. Package name is fbreader and actually your package is installing FBReader
binary. Also, /usr/share/pixmaps/FBReader and /usr/share/FBReader represents
package name which must be FBReader but your package name is fbreader and its
creating /usr/share/doc/fbreader-0.8.6d.
   So What I want to say Can you maintain uniqueness in package name and its binary?

That's a good question. It's been traditionally called "fbreader" elsewhere, and
the upstream tarball is called fbreader.

Referring to Packaging/NamingGuidelines, #6:

"While case sensitivity is not a mandatory requirement, case should only be used
where necessary. Keep in mind to respect the wishes of the upstream maintainers."

It's probably OK -- abiword is packaged the same way:
$ ls /usr/bin/[A..Z]*
/usr/bin/AbiWord-2.4
$ rpm -qf /usr/bin/AbiWord-2.4 
abiword-2.4.6-6.fc8
$ rpm -ql abiword | grep Abi
/usr/lib64/AbiWord-2.4
...
/usr/share/AbiSuite-2.4
...

Review:
+ package builds in mock (development i386).
+ rpmlint is silent for SRPM and RPM.
+ source files match upstream.
816fc0f2322e23bdaf5968443b1cfab9  fbreader-sources-0.8.6d.tgz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ Compiler flags are honoured correctly.
+ defattr usage is correct.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no -devel subpackage exists.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ ldconfig scriptlets are used.
+ Desktop file handled correctly.
+ Package fbreader-0.8.6d-2.fc8 ->
  Requires: libatk-1.0.so.0 libbz2.so.1 libc.so.6 libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.1.3) libcairo.so.2 libdl.so.2
libdl.so.2(GLIBC_2.0) libdl.so.2(GLIBC_2.1) libenca.so.0 libexpat.so.1
libgcc_s.so.1 libgdk-x11-2.0.so.0 libgdk_pixbuf-2.0.so.0 libglib-2.0.so.0
libgmodule-2.0.so.0 libgobject-2.0.so.0 libgtk-x11-2.0.so.0 libjpeg.so.62
libm.so.6 libpango-1.0.so.0 libpangocairo-1.0.so.0 libpng12.so.0 libstdc++.so.6
libstdc++.so.6(CXXABI_1.3) libstdc++.so.6(GLIBCXX_3.4) libz.so.1
libzlcore.so.0.8.6d libzltext.so.0.8.6d rtld(GNU_HASH)
  Provides: libzlcore.so.0.8.6d libzltext.so.0.8.6d zlui-gtk.so
+ GUI app.

APPROVED.

New Package CVS Request
=======================
Package Name: fbreader
Short Description: E-book reader
Owners: salimma
Branches: EL-4 EL-5 F-7
InitialCC: 
Cvsextras Commits: no

cvs done.