Bug 173683 - Review Request: bidiv (BiDi Viewer) - display logical-Hebrew text
Summary: Review Request: bidiv (BiDi Viewer) - display logical-Hebrew text
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: John Mahowald
QA Contact: David Lawrence
URL: http://ivrix.org.il/redhat/bidiv-1.4-...
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-11-18 21:46 UTC by Dan Kenigsberg
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-01-30 08:51:47 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Dan Kenigsberg 2005-11-18 21:46:17 UTC
Spec Name or Url: http://ivrix.org.il/redhat/bidiv.spec
SRPM Name or Url: http://ivrix.org.il/redhat/bidiv-1.4-2.src.rpm

Description: 
bidiv is a tiny but useful unitility for viewing logical-Hebrew text from within an 8-bit terminal. It is an easy-to-use front-end of fribidi library.

An excerpt of its man page reads:
       bidiv  is  a filter, or viewer, for birectional text stored in logical-
       order, into visual-order 8-bit text which can be  viewed  on  terminals
       that  do  not  handle bidirectionality. The output visual-order text is
       formatted assuming a fixed number of characters per line (automatically
       determined or given with the -w parameter).

       The  current  version  is  oriented towards Hebrew, and therefore reads
       ISO-8859-8-i (logical) encoded text  and  outputs  ISO-8859-8  (visual)
       encoded  text.  However, bidiv can be easily adapted to other encodings
       and right-to-left languages.

I am trying to make Fedora more useful for Hebrew speakers. Adding bidiv to Fedora Extras is one more step in that direction.

Comment 1 John Mahowald 2005-12-26 04:50:14 UTC
Needs work:
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* rpmlint of bidiv: The group is actually Applications/Text. And you can drop
the Provides.
* The package should contain the text of the license
  (wiki: PackageReviewGuidelines)
* You can drop %doc from the man page, rpm knows about man


Good:
* Source bidiv-1.4.tgz is the same as upstream
* Builds fine in mock


Comment 2 Dan Kenigsberg 2005-12-26 08:53:23 UTC
I added smp_flags, corrected the group (even before your review...), dropped the
Provides and %doc for the man page, and added the text of the GPL.
However, there must be a smarter way to add that COPYING file, when it is
missing from the upstream package - I added a new source with a tar.gz of the
GPL. I lack the rpm expertease to do it properly, and I would appreciate a hint.

Please see http://ivrix.org.il/redhat/bidiv-1.4-3.src.rpm and the updated spec.

Comment 3 Roozbeh Pournader 2005-12-26 11:49:52 UTC
(In reply to comment #0)
> bidiv is a tiny but useful unitility for viewing logical-Hebrew text from
within an 8-bit terminal.

8-bit terminals? Do we still have those in Fedora?

(In reply to comment #2)
> However, there must be a smarter way to add that COPYING file, when it is
> missing from the upstream package

I believe you should contact the upstream author and ask him to mention the
version of GPL he is releasing the package in. Nowhere in the package (including
the C source) the version of the GPL is mentioned. The "no warranty" clause is
not there either. This makes the package's license ambiguous. Please direct the
author to the final section in the GPL, How to Apply These Terms to Your New
Programs: http://www.gnu.org/licenses/gpl.html#SEC4

Also, I would recommend:
* Formatting the SPEC file according to the default Fedora format (as created by
fedora-rpmnewspec);
* Discarding the %{?dist} tag, as it is not used in the SPEC file for anything.
I think the dist tag is only for cases when there are different Requires or
BuildRequires for different disto releases (See
http://fedoraproject.org/wiki/DistTag).


Comment 4 Paul Howarth 2005-12-27 11:53:22 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > However, there must be a smarter way to add that COPYING file, when it is
> > missing from the upstream package
> 
> I believe you should contact the upstream author and ask him to mention the
> version of GPL he is releasing the package in. Nowhere in the package (including
> the C source) the version of the GPL is mentioned. The "no warranty" clause is
> not there either. This makes the package's license ambiguous. Please direct the
> author to the final section in the GPL, How to Apply These Terms to Your New
> Programs: http://www.gnu.org/licenses/gpl.html#SEC4

If upstream does not include the license text, you should ask upstream to do so
for future releases but there is no need to add the license text obtained from
elsewhere to your RPM package. The requirement to include the license text
applies only if upstream provide it.

> Also, I would recommend:
> * Formatting the SPEC file according to the default Fedora format (as created by
> fedora-rpmnewspec);
> * Discarding the %{?dist} tag, as it is not used in the SPEC file for anything.
> I think the dist tag is only for cases when there are different Requires or
> BuildRequires for different disto releases (See
> http://fedoraproject.org/wiki/DistTag).

Different "Requires" would include those auto-generated by RPM. Since this is a
binary package and will be linking against system libraries, it's likely that
different distro releases will have different dependencies, so I'd keep the dist
tag.


Comment 5 Roozbeh Pournader 2005-12-27 12:38:54 UTC
(In reply to comment #4)
> If upstream does not include the license text, you should ask upstream to do so
> for future releases but there is no need to add the license text obtained from
> elsewhere to your RPM package. The requirement to include the license text
> applies only if upstream provide it.

That's right, but my point is that the upstream license is unclear (version is
not known), and we better know the license of what we redistribute. So we better
contact the upstream author about that.


Comment 6 Nadav Har'El 2006-01-07 19:33:41 UTC
Thanks. I just released a new version of bidiv, which fixes a bug and the
documentation, and clarifies the license (which is, and always was, the GPL -
but now I also added the COPYING file and mentioned "version 2").

You can take the new version from:
http://ftp.ivrix.org.il/pub/ivrix/src/cmdline/bidiv-1.5.tgz

Comment 7 Dan Kenigsberg 2006-01-07 20:39:03 UTC
And an updated http://ivrix.org.il/redhat/bidiv-1.5-1.src.rpm is also available.

Comment 8 John Mahowald 2006-01-29 01:53:00 UTC
Good:

- rpmlint clean
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on FC4 i386
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Minor: don't need to specify Bidiv - in the Summary. Users already know what
package they are querying.

APPROVED


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