Bug 173766 - Review Request: taarich - tell the Hebrew (Jewish) date
Review Request: taarich - tell the Hebrew (Jewish) date
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael A. Peters
David Lawrence
ftp://ftp.math.technion.ac.il/calenda...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-11-20 15:28 EST by Dan Kenigsberg
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-21 14:21:09 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Dan Kenigsberg 2005-11-20 15:28:21 EST
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 17:58:18 EST
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 08:10:08 EST
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 08:29:00 EST
(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 08:42:14 EST
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 08:49:50 EST
(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 09:09:33 EST
(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.

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