Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 173766

Summary: Review Request: taarich - tell the Hebrew (Jewish) date
Product: [Fedora] Fedora Reporter: Dan Kenigsberg <danken>
Component: Package ReviewAssignee: Michael A. Peters <mpeters>
Status: CLOSED NEXTRELEASE QA Contact: David Lawrence <dkl>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-extras-list
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
URL: ftp://ftp.math.technion.ac.il/calendar/gauss/
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2005-11-21 19:21:09 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Dan Kenigsberg 2005-11-20 20:28:21 UTC
Spec Name or Url: http://ivrix.org.il/redhat/taarich.spec
SRPM Name or Url: http://ivrix.org.il/redhat/taarich-1.20051120-1.src.rpm
Description: 
This RPM includes two small utilities by Zvi Har'El.
Taarich displays the current Hebrew date (in English or in Hebrew).
Luach renders a calendar of the current Gregorian month, with Hebrew dates.
Both use Gauss's algorithm to find the Gregorian date of Passover.

As is bug 173683, this is attempt to make Fedora more useful for Hebrew speakers.

Comment 1 Michael A. Peters 2005-11-20 22:58:18 UTC
From the spec file:

%build
make

- Will it build with the RPM_OPT_FLAGS ?
- should use sed to add the $RPM_OPT_FLAGS to the CFLAGS in Makefile

install -d %{buildroot}/{%{_docdir}/%{name},%{_bindir},%{_mandir}/man1}

- Should be

install -d -m755 %{buildroot}%{_bindir}
install -d -m755 %{buildroot}%{_mandir}/man1

- You don't need to install the docdir - the %doc macro will do that for you.
- Splitting it up instead of one liner makes it more readable with the macros
- in there.

gzip luach.man taarich.man
cp luach.man.gz %{buildroot}%{_mandir}/man1/luach.1.gz
cp taarich.man.gz %{buildroot}%{_mandir}/man1/taarich.1.gz

- don't gzip the man pages, rpm will do it for you
- use install to put the man pages in their place

install -m644 {luach.man,taarich.man} %{buildroot}%{_mandir}/man1/

- will install them, and rpm will compress them.

%files
%defattr(-,root,root)
%{_bindir}/taarich
%{_bindir}/luach
%doc gauss.txt reading.txt COPYING
%{_mandir}/man1/taarich.1*
%{_mandir}/man1/luach.1*

- %doc is usually right below %defattr
- %defattr needs to be changed to %defattr(-,root,root,-)
- man pages need to be changed

%files
%defattr(-,root,root,-)
%doc gauss.txt reading.txt COPYING
%{_bindir}/taarich
%{_bindir}/luach
%{_mandir}/man1/*

- would be much better

Comment 2 Dan Kenigsberg 2005-11-21 13:10:08 UTC
Thank you for your suggestions. I applied all of them (I hope so), 
except for the one regarding sed; I did not quite understand what you 
wanted. Is it enough to pass make CFLAGS="$RPM_OPT_FLAGS -s" ?

An updated SRPM sits in http://ivrix.org.il/redhat/taarich-1.20051120-2.src.rpm
and the spec file is again in http://ivrix.org.il/redhat/taarich.spec

Comment 3 Michael A. Peters 2005-11-21 13:29:00 UTC
(In reply to comment #2)
> Thank you for your suggestions. I applied all of them (I hope so), 
> except for the one regarding sed; I did not quite understand what you 
> wanted. Is it enough to pass make CFLAGS="$RPM_OPT_FLAGS -s" ?

Yeah - that works too.

> 
> An updated SRPM sits in http://ivrix.org.il/redhat/taarich-1.20051120-2.src.rpm
> and the spec file is again in http://ivrix.org.il/redhat/taarich.spec

Formal review coming

Comment 4 Ville Skyttä 2005-11-21 13:42:14 UTC
What's that "-s" in CFLAGS for?  If it's for stripping binaries, it should be 
removed as it'll result in a useless -debuginfo package. 

Comment 5 Dan Kenigsberg 2005-11-21 13:49:50 UTC
(in reply to comment 4)
Yep, that's my bad. I mistakenly understood that Michael A. Peters 
wanted that the upstream compilation flags would be maintained.
Will be fixed during next take.

Comment 6 Michael A. Peters 2005-11-21 14:09:33 UTC
(In reply to comment #5)
> (in reply to comment 4)
> Yep, that's my bad. I mistakenly understood that Michael A. Peters 
> wanted that the upstream compilation flags would be maintained.
> Will be fixed during next take.

With that change - Approved


Good:

+ (from reviewer MUST section)
* rpmlint output clean:
[mpeters@utility result]$ ls
build.log       taarich-1.20051120-2.fc4.i386.rpm
mockconfig.log  taarich-1.20051120-2.fc4.src.rpm
root.log        taarich-debuginfo-1.20051120-2.fc4.i386.rpm
[mpeters@utility result]$ rpmlint *.rpm
[mpeters@utility result]$

* Package named according to guidelines
- no upstream tarball name, named according to primary binary
* Spec file name matches binary package name
* Package meets packaging guidelines
* OSI Approved license (MIT) - matches source
* License in %doc
* Spec file written in American English
- Hebrew portions obviously appropriate, and english exists
* Spec file legible
* Sources match upstream
- I mirrored upstream ftp directory to verify md5sum of files
--  lftp -e 'mirror -e' ftp://ftp.math.technion.ac.il/calendar/gauss/
* Package succesfully compiles on i386 FC4
* No un-necessary BuildRequires, builds in mock
* No lang files (no need for %find_lang)
* No shared libraries (no need for ldconfig)
* Proper use of macros for paths
* No duplicate files
* Proper %files section - appropriate permissions/ownership
* Proper %clean
* Consistent use of macros
* Package contains code
* No need for separate doc package
* No need for devel package
* No .la files
* No need for desktop file
+ (from reviewer SHOULD section)
* Builds in mock
* Hebrew description
* Package works
[mpeters@utility result]$ taarich
19 Heshvan 5766
[mpeters@utility result]$ 

-=-
Needs:
There should be a Hebrew summary line since there is a Hebrew description.
This is not a blocker, I'm approving. But adding

Summary(he):  Something in Hebrew

under

Summary: Tells the Hebrew date, Torah readings, and generates calendars

would be advised.