Bug 571364

Summary: Review Request: man-db - Database cached manual pager suit
Product: [Fedora] Fedora Reporter: Ivana Varekova <varekova>
Component: Package ReviewAssignee: Miloslav Trmač <mitr>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: cjwatson, dcantrell, fedora-package-review, jvcelak, mitr, notting, terjeros
Target Milestone: ---Flags: mitr: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-04-15 13:12:16 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Attachments:
Description Flags
Some spelling fixes none

Description Ivana Varekova 2010-03-08 09:52:37 UTC
Spec URL: http://people.redhat.com/varekova/man-db.spec
SRPM URL: http://people.redhat.com/varekova/man-db-2.5.7-1.fc14.src.rpm
Description: 
man-db package includes five tools for browsing man-pages:
man, whatis, appropos, manpath and lexgrog. man preformat and display
manual pages. whatis search the manual page names. apropos search the
manual page names and descriptions. manpath determine search path
for manual pages. lexgrog directly read header information in
manual pages.

Comment 1 Terje Røsten 2010-03-08 21:05:24 UTC
Not a instant success:

$ rpm x86_64/man-db-2.5.7-1.fc12.x86_64.rpm
Preparing...                ########################################### [100%]
   1:man-db                 ########################################### [100%]

$ man man
man: can't open the manpath configuration file /usr/local/etc/man_db.conf


Some comments

> mkdir $RPM_BUILD_ROOT/etc/cron.daily
> install -m755 %{SOURCE1} $RPM_BUILD_ROOT/etc/cron.daily/man-db.cron

could be one line and use sysconfdir macro:

install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT%{_sysconfdir}/cron.daily/man-db.cron

make CC="gcc $RPM_OPT_FLAGS"

might change to:

make CC="{__cc} $RPM_OPT_FLAGS"

might be more explicit here:

# documentation and translation
%{_mandir}/man1/*
%{_mandir}/man5/*
%{_mandir}/man8/*

Unsure about the %lang stuff, could the %find_lang macro be used.

https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Compile flags is not visible during build, is correct flags used?

Comment 2 Terje Røsten 2010-03-10 11:58:29 UTC
Fixed some issues:
- fix man_db.conf location
- build with various compress support
- explicit file listing
- use find_lang macro
- redo install part
- remove all translations

spec: http://terjeros.fedorapeople.org/man-db/man-db.spec
srpm: http://terjeros.fedorapeople.org/man-db/man-db-2.5.7-2.fc13.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2043624

with changes it works fine here, please feel free to use.

Comment 3 Terje Røsten 2010-03-11 09:14:45 UTC
More fixes:
 - move cron config to /etc/sysconfig/man-db
 - default to quiet in cron script

spec: http://terjeros.fedorapeople.org/man-db/man-db.spec
srpm: http://terjeros.fedorapeople.org/man-db/man-db-2.5.7-3.fc13.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2045636

Please have a look.

Comment 4 Terje Røsten 2010-03-12 08:48:58 UTC
Let's follow Debian and move config to /etc/manpath.config, also enabled
verbose build by adding V=1 to make. 

What to do with the --with-browser option? 
Tried xdg-open, however did not seem to work.

spec: http://terjeros.fedorapeople.org/man-db/man-db.spec
srpm: http://terjeros.fedorapeople.org/man-db/man-db-2.5.7-4.fc13.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2048347

Comment 5 Ivana Varekova 2010-03-18 14:57:58 UTC
Spec URL: http://people.redhat.com/varekova/man-db.spec
SRPM URL: http://people.redhat.com/varekova/man-db-2.5.7-5.fc14.src.rpm

Hello, 
I just go through the comments, the problems which you wrote and is not mentioned below should be fixed in the new version (I hope I don't miss anything), 
I have comments to several points:

> Fixed some issues:
> - fix man_db.conf location
%{_sysconfdir}/man_db.conf seems to be better for me 
(man have %{_sysconfdir}/man.config)

> - build with various compress support
not sure it is needed

> - redo install part
partially done

> - remove all translations
I see no reason to do it

the other change is to incorporate cron configuration varibles to man_db.config file.

Comment 6 Terje Røsten 2010-03-18 21:01:09 UTC
The changes is good and the updated package works fine. It's better than old man package. However, I would prefer a bit more info in the changelog. 
Any wat, it's now ready for a formal review.

Comment 7 Miloslav Trmač 2010-04-01 15:37:55 UTC
rpmlint:

> man-db.src: W: spelling-error %description -l en_US whatis -> whats, what's, what is
> (other "spelling error" warnings, some bogus, some correct)

> man-db.src:34: W: configure-without-libdir-spec
This doesn't seem to be a problem... but see below about %configure

> man-db.x86_64: W: incoherent-version-in-changelog 2.5.7-1 ['2.5.7-5.fc12', '2.5.7-5']
Trivial to fix.

%doc must include docs/COPYING*.   Other files to consider: doc/HACKING, doc/TODO, ChangeLog, NEWS; I think including at least NEWS would be useful.

I'm not sure about "Provides: man = %{version}":  at the very least it should be "%{version}-%{release}".  

Requires: crontabs should be added to handle /etc/cron.daily ownership.

man-db should own /usr/libexec/man-db

Wouldn't it be cleaner to use %configure than to use --with-config-file and make CC= ?

Please use only one of %{buildroot} and $RPM_BUILD_ROOT, and %global instead of %define.

PackagingGuidelines recommend adding BuildRequires: gettext.

Consider using (make install INSTALL='install -p' ...) to preserve timestamps.

"All patches should have an upstream bug link or comment"

Would it make sense to use /etc/sysconfig/man-db instead of adding new options to /etc/man_db.conf ?  For example, if upstream ever tightened the parser to reject invalid values, Fedora would have to loosen the checking again.

Is the man-pages-de translation of man-db man pages better?  Knowing nothing (and not having looked), I'd assume the man pages shipped with the project would be more likely to be up-to-date.

"install -d m 0755  $RPM_BUILD_ROOT%{cache}" seems to be missing a '-' before 'm'.

Comment 8 Miloslav Trmač 2010-04-01 15:38:46 UTC
Created attachment 404027 [details]
Some spelling fixes

Comment 9 Ivana Varekova 2010-04-13 09:33:38 UTC
Thanks for the comments, all incorporated in 
Spec URL: http://people.redhat.com/varekova/man6/man-db.spec
SRPM URL: http://people.redhat.com/varekova/man6/man-db-2.5.7-6.fc14.src.rpm
the only think which differs a bit is the dependency man-pages-reader - the cause is described in thread: 
http://lists.fedoraproject.org/pipermail/devel/2010-March/133056.html

Comment 10 Terje Røsten 2010-04-13 11:47:28 UTC
Seems like you removed the patch, however forgot to remove the comment:
# this patch adds cron job which daily update whatis database

Any reason you don't update %changelog when doing changes?

Ref:

 https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Comment 11 Miloslav Trmač 2010-04-13 12:25:32 UTC
Approved assuing the following is fixed:
* "Requires: crontab" is AFAICS not available (use "crontabs").
* man-db now conflicts with man-pages-de; the conflicting files must be removed
from one of the packages.


Please also try to find all packages that currently directly require "man"
(e.g. rpmdevtools) and get them fixed.


These warnings look somewhat dangerous, in particular the non-debug() calls:
> catman.c:344: warning: format '%d' expects type 'int', but argument 2 has type 'size_t'
> manp.c: In function 'has_mandir':
> manp.c:1112: warning: field precision should have type 'int', but argument 2 has type 'long int'
> manp.c:1124: warning: field precision should have type 'int', but argument 2 has type 'long int'
> manp.c: In function 'create_pathlist':
> manp.c:1215: warning: format '%d' expects type 'int', but argument 3 has type 'long int'
> manp.c:1215: warning: format '%d' expects type 'int', but argument 5 has type 'long int'

Comment 12 Ivana Varekova 2010-04-14 12:59:40 UTC
The required flag is fixed and bg report against man-pages-de created (https://bugzilla.redhat.com/show_bug.cgi?id=582225).
Point 
>Please also try to find all packages that currently directly require "man"
>(e.g. rpmdevtools) and get them fixed.
will be done when the package will be in cvs.
Thanks for the review.

Comment 13 Ivana Varekova 2010-04-14 13:01:49 UTC
New Package CVS Request
=======================
Package Name: man-db
Short Description: Database cached manual pager suite
Owners: varekova
Branches: 
InitialCC:

Comment 14 Kevin Fenzi 2010-04-14 21:50:48 UTC
CVS done (by process-cvs-requests.py).

Comment 15 Jesse Keating 2010-08-15 03:25:49 UTC
Looks like somewhere along the way the Provides for man got lost.  This shouldn't have been let in like this.

Comment 16 Colin Watson 2010-12-02 19:31:41 UTC
(In reply to comment #11)
> These warnings look somewhat dangerous, in particular the non-debug() calls:
> > catman.c:344: warning: format '%d' expects type 'int', but argument 2 has type 'size_t'
> > manp.c: In function 'has_mandir':
> > manp.c:1112: warning: field precision should have type 'int', but argument 2 has type 'long int'
> > manp.c:1124: warning: field precision should have type 'int', but argument 2 has type 'long int'
> > manp.c: In function 'create_pathlist':
> > manp.c:1215: warning: format '%d' expects type 'int', but argument 3 has type 'long int'
> > manp.c:1215: warning: format '%d' expects type 'int', but argument 5 has type 'long int'

Thanks.  Nobody passed these on to me (upstream), but I happened to come across this bug and I've now fixed them in bzr for man-db 2.6.0.