Bug 228894 - Review Request: rpcbind - converts RPC program numbers into universal addresses
Summary: Review Request: rpcbind - converts RPC program numbers into universal addresses
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: David Cantrell
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FC-ACCEPT 234855
TreeView+ depends on / blocked
 
Reported: 2007-02-15 19:40 UTC by Steve Dickson
Modified: 2013-01-10 01:35 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-04-10 21:47:03 UTC
Type: ---
Embargoed:
jkeating: fedora-review+


Attachments (Terms of Use)

Description Steve Dickson 2007-02-15 19:40:18 UTC
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 20:16:56 UTC
I don't think this should be fedora-review+ until someone actually reviews and
approves it.

Comment 2 Steve Dickson 2007-02-15 22:57:01 UTC
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 16:46:47 UTC
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 21:56:35 UTC
> 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 18:26:01 UTC
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 19:36:47 UTC
> 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 21:29:15 UTC
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 19:46:58 UTC
What is the status of coordinating this with the removal of portmap?

Comment 10 Steve Dickson 2007-03-08 19:59:36 UTC
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 21:47:03 UTC
rpcbind was built for rawhide, closing.


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