Bug 228894

Summary: Review Request: rpcbind - converts RPC program numbers into universal addresses
Product: [Fedora] Fedora Reporter: Steve Dickson <steved>
Component: Package ReviewAssignee: David Cantrell <dcantrell>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: jakub, mmcgrath
Target Milestone: ---Flags: jkeating: fedora‑review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-04-10 17:47:03 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 188268, 234855    

Description Steve Dickson 2007-02-15 14:40:18 EST
Spec URL: http://people.redhat.com/steved/rpcbind/rpcbind.spec
SRPM URL: http://people.redhat.com/steved/rpcbind/rpcbind-0.1.4-1.fc7.src.rpm
Description:

The rpcbind utility is a server that converts RPC program numbers into
universal addresses.  It must be running on the host to be able to make
RPC calls on a server on that machine.
Comment 1 Jason Tibbitts 2007-02-15 15:16:56 EST
I don't think this should be fedora-review+ until someone actually reviews and
approves it.
Comment 2 Steve Dickson 2007-02-15 17:57:01 EST
Sorry about that... I ment to set the '?' flag, not the '+' flag assuming
that the way things should work... 
Comment 3 Jesse Keating 2007-02-16 11:46:47 EST
Couple issues thus far:

Mixed tabs/spaces in the spec (URL line is tabbed, rest are spaces)
Mixed use of $RPM_BUILD_ROOT and %{buildroot}, please pick one.
Do you need to --prefix %{buildroot} ?  doesn't %configure set a proper prefix
that will be used when you do DESTDIR= in %install?
Init files aren't supposed to be config files are they?
Why is there an 'exit 0' in  your %post ?

Also the build fails in mock:
security.c:24:27: error: rpcsvc/rquota.h: No such file or directory
Comment 4 Steve Dickson 2007-02-16 16:56:35 EST
> Mixed tabs/spaces in the spec (URL line is tabbed, rest are spaces)
changes the tabs to spaces

> Mixed use of $RPM_BUILD_ROOT and %{buildroot}, please pick one.
%{buildroot} won... 

> Do you need to --prefix %{buildroot} ?  doesn't %configure set a proper prefix
> that will be used when you do DESTDIR= in %install?
True, --prefix %{buildroot} is not needed...

> Init files aren't supposed to be config files are they?
Per our IRC converstion, I would like to leave this in
since thats the way a number of other RPC/NFS related 
packages work.

> Why is there an 'exit 0' in  your %post ?
because someone complained about the %post not returning a zero
exit code... but it really not needed in this case so I remove it.

> security.c:24:27: error: rpcsvc/rquota.h: No such file or directory
added a quota requirement

so in the end here is the diff... 
diff -r1.1 rpcbind.spec
9c9
< URL:                          http://nfsv4.bullopensource.org
---
> URL:            http://nfsv4.bullopensource.org
19c19
< Requires: libtirpc
---
> Requires: libtirpc quota
49,50c49
<       --enable-debug \
<       --prefix=%{buildroot}
---
>       --enable-debug
65c64
< rm -rf $RPM_BUILD_ROOT
---
> rm -rf %{buildroot}
79d77
< exit 0

The source rpm and spec on my people page have been updated.

Comment 5 Jesse Keating 2007-02-21 13:26:01 EST
You're supposed to add in the spec comments about the changes you've applied.  

There are still mixed spaces/tabs, (spaces: line 3, tab: line 48)

Requires: libtirpc is redundant.  RPM automatically picks up and adds a
requirement on libtirpc.so.1()(64bit)  (for 64bit builds).  You don't need to
list the libtirpc Requirement.

rpcbind is enabled by default, is there good reason for this?  Must be asked of
any service that is enabled by default.
Comment 6 Steve Dickson 2007-02-21 14:36:47 EST
> There are still mixed spaces/tabs, (spaces: line 3, tab: line 48)
There were a couple other places as well... I think I got them all.. 

> Requires: libtirpc is redundant.  RPM automatically picks up and adds a
> requirement on libtirpc.so.1()(64bit)  (for 64bit builds).  You don't need to
> list the libtirpc Requirement.
Didn't know rpm will do an ldd to figure out which libs are needed...

> rpcbind is enabled by default, is there good reason for this?
Well this is a portmapper replacement so not only should rpcbind
be enable by default, the portmapper needed to be turn off 
or even removed 

One final note, the rpcinfo and its man page that are installed
from glibc-common need to be removed as well... How does one
do that? 
Comment 8 Jesse Keating 2007-02-21 16:29:15 EST
Looks good now.  The last thing would be coordinating the Obsoletes/Provides
with portmap for the removal of portmap.  I'm approving this package (although
I'm still not happy about the init script being marked as a config file)
Comment 9 Jesse Keating 2007-03-05 14:46:58 EST
What is the status of coordinating this with the removal of portmap?
Comment 10 Steve Dickson 2007-03-08 14:59:36 EST
Well I'm ready when you are....

Also, we should synchronize the removal of the
rpcinfo that lives in glibc-common. Note: the
rpcinfo command that lives in the rpcbind package
is currently being installed in /usr/bin/ and probably
should be moved over to /usr/sbin (once the glibc-common
is removed).
Comment 12 Jesse Keating 2007-04-10 17:47:03 EDT
rpcbind was built for rawhide, closing.