Bug 292691 - Review Request: fbreader - E-book reader
Review Request: fbreader - E-book reader
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-17 00:34 EDT by Michel Alexandre Salim
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-29 23:13:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michel Alexandre Salim 2007-09-17 00:34:34 EDT
Spec URL: http://hircus.org/fedora/fbreader/fbreader.spec
SRPM URL: http://hircus.org/fedora/fbreader/fbreader-0.8.6d-1.fc8.src.rpm
Description:

FBReader is an e-book reader, with the following main features:

* Supports several formats: fb2, HTML, CHM, plucker, Palmdoc, zTxt
  (Weasel), TCR (psion), RTF, OEB, OpenReader, mobipocket, plain text.
* Direct reading from tar, zip, gzip and bzip2 archives. (Multiple
  books in one archive are supported.)
* Automatic library building.
* Automatic encoding detection is supported.
* Automatically generated contents table.
* Embedded images support.
* Footnotes/hyperlinks support.
* Position indicator.
* Keeps the last open book and the last read positions for all opened
  books between runs.
* List of last opened books.
* Automatic hyphenations. Liang's algorithm is used. The same
  algorithm is used in TeX, and TeX hyphenation patterns are used in
  FBReader. Patterns for Czech, English, Esperanto, French, German and
  Russian are included in the current version.
* Text search.
* Full-screen mode.
* Screen rotation by 90, 180 and 270 degrees.
Comment 1 Parag AN(पराग) 2007-09-18 03:21:58 EDT
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.
Comment 2 Michel Alexandre Salim 2007-09-20 03:09:04 EDT
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?
Comment 3 Parag AN(पराग) 2007-09-21 03:02:14 EDT
(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. 
Comment 4 Michel Alexandre Salim 2007-09-21 12:07:10 EDT
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!
Comment 5 Parag AN(पराग) 2007-09-22 06:17:24 EDT
(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!

Comment 6 Parag AN(पराग) 2007-09-22 06:25:42 EDT
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
Comment 7 Michel Alexandre Salim 2007-09-22 10:31:28 EDT
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!
Comment 8 Parag AN(पराग) 2007-09-24 01:09:54 EDT
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.
Comment 9 Michel Alexandre Salim 2007-09-25 15:39:42 EDT
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.
Comment 10 Parag AN(पराग) 2007-09-26 00:45:58 EDT
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?
Comment 11 Michel Alexandre Salim 2007-09-26 17:34:24 EDT
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
...
Comment 12 Parag AN(पराग) 2007-09-26 21:42:03 EDT
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.
Comment 13 Michel Alexandre Salim 2007-09-29 12:29:16 EDT
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
Comment 14 Kevin Fenzi 2007-09-29 14:45:23 EDT
cvs done.

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