Bug 226186

Summary: Merge Review: ncpfs
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mitr
Target Milestone: ---Flags: j: 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-02-23 09:39:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

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.