Bug 226186 - Merge Review: ncpfs
Summary: Merge Review: ncpfs
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:15 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-23 09:39:50 UTC
Type: ---
Embargoed:
j: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 20:15:15 UTC
Fedora Merge Review: ncpfs

http://cvs.fedora.redhat.com/viewcvs/devel/ncpfs/
Initial Owner: mitr

Comment 1 Jason Tibbitts 2007-02-04 05:35:42 UTC
This should be fun.  First, the rpmlint issues:

W: ncpfs no-url-tag
  Is there actually an upstream for this?
  It's probably best to at least add URL: http://ftp.cvut.cz/ncpfs/

W: ncpfs devel-file-in-non-devel-package /usr/lib/libncp.so
W: ncpfs devel-file-in-non-devel-package /usr/include/ncp/obsolete/o_ndslib.h
  And many more .h files.  These should all be placed into a separate
  -devel subpackage.

E: ncpfs setuid-binary /usr/bin/ncplogin root 04755
E: ncpfs non-standard-executable-perm /usr/bin/ncplogin 04755
  A couple like these.  I suppose these are necessary, but has anyone looked
  at the security issues?

Just adding URL: and making the -devel packages would take care of all of the
real rpmlint issues.

More review coming tomorrow after sleep.


Comment 2 Jason Tibbitts 2007-02-04 15:07:21 UTC
Here's the full review; fixing the above issues and the buildroot should be all
that's necessary.

* source files match upstream:
   2837046046bcdb46d77a80c1d17dbfd15e878700e879edab4cda9f080e0337f9
   ncpfs-2.2.6.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
O dist tag is not present (not required)
X build root is not correct; should be
  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* %makeinstall is not used.
* package builds in mock.
* debuginfo package looks complete.
X rpmlint is silent.
* final provides and requires are sane:
   /sbin/ldconfig
   ipxutils
   libncp.so.2.3
   libncp.so.2.3(NCPFS.2.2.0.17)
   libncp.so.2.3(NCPFS.2.2.0.18)
   libncp.so.2.3(NCPFS.INTERNAL)
   libncp.so.2.3(NCPFS_2.2.0.19)
   libncp.so.2.3(NCPFS_2.2.1)
   libncp.so.2.3(NCPFS_2.2.4)
  =
   libncp.so.2.3
   libncp.so.2.3(NCPFS.2.2.0.17)
   libncp.so.2.3(NCPFS.2.2.0.18)
   libncp.so.2.3(NCPFS.INTERNAL)
   libncp.so.2.3(NCPFS.MPILIB)
   libncp.so.2.3(NCPFS_2.2.0.19)
   libncp.so.2.3(NCPFS_2.2.1)
   libncp.so.2.3(NCPFS_2.2.4)
   ncpfs = 2.2.6-6
  (ipxutils provides only ipxutils = 2.2.6-6)

* %check is present; no test suite upstream.
* shared libraries are present; ldconfig called as necessary.
X unversioned .so files should be in -devel subpackage.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets present are OK (ldconfig).
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
X headers present and should be in -devel package.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.

Comment 3 Miloslav Trmač 2007-02-08 05:38:09 UTC
Thanks!

W: ncpfs no-url-tag
FTP URL added

W: ncpfs devel-file-in-non-devel-package /usr/lib/libncp.so
W: ncpfs devel-file-in-non-devel-package /usr/include/ncp/obsolete/o_ndslib.h
> And many more .h files.  These should all be placed into a separate -devel
> subpackage.
Done.

E: ncpfs setuid-binary /usr/bin/ncplogin root 04755
E: ncpfs non-standard-executable-perm /usr/bin/ncplogin 04755
> A couple like these.  I suppose these are necessary, but has anyone looked
> at the security issues?
A few security issues in ncpfs have been discovered, so at least someone has :)

X build root is not correct; should be
  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Fixed.
X unversioned .so files should be in -devel subpackage.
X headers present and should be in -devel package.
Both fixed.

Please review ncpfs-2.2.6-7.

Comment 4 Kevin Fenzi 2007-02-09 03:47:46 UTC
Hey mitr... 

I think the procedure we are using currently is to reassign back to the reviewer
and reset the fedora-review flag to ? when you want the reviewer to check back
on changes... I am doing so here so tibbs can continue. 

Comment 5 Jason Tibbitts 2007-02-18 05:44:13 UTC
OK, this builds fine and all that remains of the rpmlint output are the
complaints about setuid binaries.

So BuildRoot is good, rpmlint output is OK, headers and unversioned .so files
are in a -devel subpackage, and everything else still looks good.

APPROVED

At this point I'm not sure if we're still ping-ponging the ticket assignments,
so I'll leave this assigned to myself.

Comment 6 Miloslav Trmač 2007-02-20 15:41:45 UTC
Thanks.  http://www.fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags says it
should be assigned to me, so let's see what happens ;)

Comment 7 Jason Tibbitts 2007-02-22 18:26:18 UTC
Actually not.  Just close this bug when the fixed package is built for rawhide.
 I think it already is.

Comment 8 Miloslav Trmač 2007-02-23 09:39:50 UTC
Thanks for the correction.


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