Bug 518318 - Review Request: vanessa_socket - Simplify TCP/IP socket operations
Summary: Review Request: vanessa_socket - Simplify TCP/IP socket operations
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orion Poplawski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 518315
Blocks: 518317
TreeView+ depends on / blocked
 
Reported: 2009-08-19 20:21 UTC by Pavel Alexeev
Modified: 2012-05-15 02:11 UTC (History)
6 users (show)

Fixed In Version: vanessa_socket-0.0.12-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-05-03 22:52:16 UTC
Type: ---
Embargoed:
orion: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Pavel Alexeev 2009-08-19 20:21:10 UTC
Spec URL: http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket-0.0.7-2.fc11.src.rpm
Description:
Library to simplify TCP/IP socket operations. Includes code to
open a socket to a server as a client, to listen on socket for
clients as a server and to pipe information between sockets.

P.S. In spec used 5 space tab width.

Comment 3 Joshua Roys 2009-12-14 17:49:09 UTC
Hello,

Version 0.0.7 seems to be a bit out of date; 0.0.10 is out.  Also, the Source0 line is not accessible (no ftp anymore).

Why tabsize of 5?  It just seems non-standard :)  Don't suppose I could convince you to use 4 or 8 or to just use spaces?
Do we really need that ugly looking thing above the %configure?

Comment 4 Pavel Alexeev 2009-12-20 22:53:36 UTC
(In reply to comment #3)
> Hello,
> 
> Version 0.0.7 seems to be a bit out of date; 0.0.10 is out.  Also, the Source0
> line is not accessible (no ftp anymore).
You are rigth. Updated.

http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket-0.0.10-5.fc11.src.rpm
http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket.spec

> Why tabsize of 5?  It just seems non-standard :)  Don't suppose I could
> convince you to use 4 or 8 or to just use spaces?
> Do we really need that ugly looking thing above the %configure?  
https://fedoraproject.org/wiki/PavelAlexeev/tabsize

Comment 5 Joshua Roys 2010-01-04 15:52:28 UTC
(In reply to comment #4)
> > Why tabsize of 5?  It just seems non-standard :)  Don't suppose I could
> > convince you to use 4 or 8 or to just use spaces?
> https://fedoraproject.org/wiki/PavelAlexeev/tabsize

That's fine - it is your choice at this point.  I was just curious.

> > Do we really need that ugly looking thing above the %configure?
This was a separate question.  Why do we need to run all the autotools by hand?  Does it still work if we remove them?  Do we need to mess with CFLAGS?

You updated the BuildRequires but not the Requires?

You have some single %s in the changelog, 0.0.7-3, line 2, at the end.

Thanks.

Comment 6 Pavel Alexeev 2010-01-05 10:37:31 UTC
(In reply to comment #5)

At least we are talking about the subject.

> This was a separate question.  Why do we need to run all the autotools by hand?
>  Does it still work if we remove them?  Do we need to mess with CFLAGS?
Thanks, I remove all this stuff.

> You updated the BuildRequires but not the Requires?
Thanks, fixed.

> You have some single %s in the changelog, 0.0.7-3, line 2, at the end.
Sorry, I don't understand about what "%s" you are speak. I search and in spec "%s" is not found at all.

http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket-0.0.10-6.fc11.src.rpm

Comment 7 Garrett Holmstrom 2010-01-13 04:04:55 UTC
Here are some more comments that should be useful:

- libvanessa_socket/vanessa_socket_daemon.c is GPLv2+, not LGPLv2+, so I'm fairly certain the license field needs to change.

- You actually do need to run autotools because the stock configure script adds an RPATH:
vanessa_socket.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/vanessa_gethostbyname ['/usr/lib64']
vanessa_socket-pipe.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/vanessa_socket_pipe ['/usr/lib64']

- The only other complaint rpmlint has relates to a lack of %doc files in the devel subpackage; I'm not sure how important that is here.
vanessa_socket-devel.x86_64: W: no-documentation

Comment 8 Joshua Roys 2010-01-13 13:22:34 UTC
Concerning rpath, see if the following works before doing the autotools by hand, please:

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

Note the "Removing Rpath" section.

Comment 9 Michael Schwendt 2010-01-15 12:24:05 UTC
> Requires:               vanessa_logger >= 0.0.8

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


> %package                devel
> ...
> Requires:               vanessa_logger-devel >= 0.0.5

Questionable version here. Above it's 0.0.8, in the BuildRequires it's 0.0.8, and here it's 0.0.5.


> Group:          Development/Libraries

Run-time libraries still belong into group "System Environment/Libraries".


> vanessa_socket_daemon.c

This file is in need of a licence clarification by its author. It's likely he just forgot to replace the GPLv2+ header, but as long as that one is present, the following guideline applies:

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Mixed_Source_Licensing_Scenario


> mkdir -p %{buildroot}/{etc,%{prefix}/{lib,bin,doc}}

This doesn't look right.  If creating %prefix/lib is really needed, the build will fail on 64-bit where %prefix/lib64 would not be created.  And %prefix/doc isn't used by the %files section.


> %doc README

The file is duplicated in two packages.  In the base library package the file contents refers to "vanessa_socket_pipe", which is not included in that package. Nothing explains where to find the missing vanessa_socket_pipe.  And in the separate vanessa_socket-pipe package, the same README file refers to the library. => Splitting off the vanessa_socket_pipe program is highly questionable. Especially, since the base library package contains another program in /usr/bin already, so nothing is won by splitting off one tool.

Comment 10 Pavel Alexeev 2010-01-24 22:20:34 UTC
(In reply to comment #7)
> Here are some more comments that should be useful:
> 
> - libvanessa_socket/vanessa_socket_daemon.c is GPLv2+, not LGPLv2+, so I'm
> fairly certain the license field needs to change.
You are right.

> - You actually do need to run autotools because the stock configure script adds
> an RPATH:
> vanessa_socket.x86_64: E: binary-or-shlib-defines-rpath
> /usr/bin/vanessa_gethostbyname ['/usr/lib64']
> vanessa_socket-pipe.x86_64: E: binary-or-shlib-defines-rpath
> /usr/bin/vanessa_socket_pipe ['/usr/lib64']
Hm, it is strange, because is not appeared in i586 architecture! Thank you.
Joshua Roys, also thanks, I follow this guidelines to cut off it.

> - The only other complaint rpmlint has relates to a lack of %doc files in the
> devel subpackage; I'm not sure how important that is here.
> vanessa_socket-devel.x86_64: W: no-documentation    
It is not important in -devel sub-package.

(In reply to comment #9)
> > Requires:               vanessa_logger >= 0.0.8
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
> 
> 
> > %package                devel
> > ...
> > Requires:               vanessa_logger-devel >= 0.0.5
> 
> Questionable version here. Above it's 0.0.8, in the BuildRequires it's 0.0.8,
> and here it's 0.0.5.
Fixed, thanks.
> 
> > Group:          Development/Libraries
> 
> Run-time libraries still belong into group "System Environment/Libraries".
Ok, changed.
Please, can you provide link where I can read about it more?
> 
> > vanessa_socket_daemon.c
> This file is in need of a licence clarification by its author. It's likely he
> just forgot to replace the GPLv2+ header, but as long as that one is present,
> the following guideline applies:
> 
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Mixed_Source_Licensing_Scenario

I ask Simon Horman, wait answer.


> > mkdir -p %{buildroot}/{etc,%{prefix}/{lib,bin,doc}}
> 
> This doesn't look right.  If creating %prefix/lib is really needed, the build
> will fail on 64-bit where %prefix/lib64 would not be created. And %prefix/doc
> isn't used by the %files section.
It was built on x86_64 successfully.

But you are right, it is ambiguous line at all.

> 
> 
> > %doc README
> 
> The file is duplicated in two packages.  In the base library package the file
> contents refers to "vanessa_socket_pipe", which is not included in that
> package. Nothing explains where to find the missing vanessa_socket_pipe.  And
> in the separate vanessa_socket-pipe package, the same README file refers to the
> library. => Splitting off the vanessa_socket_pipe program is highly
> questionable. Especially, since the base library package contains another
> program in /usr/bin already, so nothing is won by splitting off one tool.    
This splitting and README duplication comes from history based author spec. READMY contains generic information about author, licensing and both subpackages. I think there no error include it in both.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1941649
http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket-0.0.10-7.fc11.src.rpm

Comment 11 Frank DiPrete 2010-01-25 13:55:58 UTC
> mkdir -p %{buildroot}/{etc,%{prefix}/{lib,bin,doc}}

libdir=%{?buildroot:%{buildroot}}%{_libdir}
bindir=%{?buildroot:%{buildroot}}%{_bindir}
_defaultdocdir     %{_usr}/share/doc

from rpmbuild --showrc

Comment 12 Pavel Alexeev 2010-01-25 16:31:42 UTC
Frank DiPrete, I don't understand what you want say.
Explicit directory creation deleted and have not use in spec now.

I got answer from upstream developer. Simon Horman wrote what it is really mistake and license should be LGPLv2+! I revert back it.

http://hubbitus.net.ru/rpm/Fedora11/vanessa_socket/vanessa_socket-0.0.10-8.fc11.src.rpm

Comment 14 Orion Poplawski 2012-02-27 18:29:34 UTC
None of the spec/src.rpm links above work anymore.  Pavel - Are you still around?

Comment 15 Pavel Alexeev 2012-03-01 19:29:11 UTC
Sorry, domain changed.

Yes, I ready continue. Package updated to last version.

http://hubbitus.info/rpm/Fedora16/vanessa_socket/vanessa_socket-0.0.12-1.fc16.src.rpm
http://hubbitus.info/rpm/Fedora16/vanessa_socket/vanessa_socket.spec

Comment 16 Orion Poplawski 2012-03-14 18:27:19 UTC
rpmlint:

vanessa_socket.i686: W: unused-direct-shlib-dependency /usr/lib/libvanessa_socket.so.2.1.0 /lib/libdl.so.2

Looks like libvanessa_socket.so.2.1.0 does not need to be linked against -ldl.

vanessa_socket.i686: E: incorrect-fsf-address /usr/share/doc/vanessa_socket-0.0.12/README
vanessa_socket-pipe.i686: E: incorrect-fsf-address /usr/share/doc/vanessa_socket-pipe-0.0.12/COPYING

FSF now just uses their url:

    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.

Please notify upstream.

More to come....

Comment 17 Pavel Alexeev 2012-03-17 13:01:51 UTC
Thanks.
Both questions mailed to upstream author.

Comment 18 Orion Poplawski 2012-04-18 19:25:26 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[!]: MUST Buildroot is not present
     Note: Invalid buildroot found:
     %{_tmppath}/%{name}-%{version}-%{release}-%(%{__id_u} -n)
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
[x]: MUST Sources contain only permissible code or content.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files devel section. This is OK if
     packaging for EPEL5. Otherwise not needed
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/export/home/orion/redhat/vanessa_socket-0.0.12/vanessa_socket-0.0.12.tar.bz2 :
  MD5SUM this package     : 5ff674c1c129c0551acdebdd663d58f8
  MD5SUM upstream package : 5ff674c1c129c0551acdebdd663d58f8

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[-]: SHOULD Spec use %global instead of %define.
 
Issues:
[!]: MUST Buildroot is not present
     Note: Invalid buildroot found:
     %{_tmppath}/%{name}-%{version}-%{release}-%(%{__id_u} -n)
See: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
[!]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL
See: http://fedoraproject.org/wiki/Packaging/Guidelines#.25clean
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files devel section. This is OK if
     packaging for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
See: None

Just a little bit to bring it up to modern spec (unless supporting EPEL 5).  See if you can remove the unneeded -ldl as well.  Then we are set.

Comment 19 Orion Poplawski 2012-04-18 19:26:34 UTC
I should note that I would like to see this in EPEL 6 and will maintain it there if you don't want to.

Comment 20 Pavel Alexeev 2012-04-22 18:45:14 UTC
Yes, I'll plan maintain it for Epel6, but also for Epel5. So, there should stay buildroot specification, manual cleanup and other stuff mentioned as error.

FTI it builds fine: http://koji.fedoraproject.org/koji/taskinfo?taskID=4013481

Comment 21 Orion Poplawski 2012-04-23 17:19:45 UTC
Okay, APPROVED.

Comment 22 Pavel Alexeev 2012-04-24 11:04:54 UTC
Orion, thank you very much for the review.

New Package SCM Request
=======================
Package Name: vanessa_socket
Short Description: Simplify TCP/IP socket operations
Owners: hubbitus
Branches: F-16 F-17 EL-5 EL-6
InitialCC:

Comment 23 Gwyn Ciesla 2012-04-24 12:42:57 UTC
Git done (by process-git-requests).

Comment 24 Fedora Update System 2012-04-24 19:49:14 UTC
vanessa_socket-0.0.12-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/vanessa_socket-0.0.12-1.el5

Comment 25 Fedora Update System 2012-04-24 19:58:44 UTC
vanessa_socket-0.0.12-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/vanessa_socket-0.0.12-1.el6

Comment 26 Fedora Update System 2012-04-24 20:08:47 UTC
vanessa_socket-0.0.12-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/vanessa_socket-0.0.12-1.fc16

Comment 27 Fedora Update System 2012-04-25 06:06:19 UTC
vanessa_socket-0.0.12-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/vanessa_socket-0.0.12-1.fc17

Comment 28 Fedora Update System 2012-04-26 03:32:32 UTC
Package vanessa_socket-0.0.12-1.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing vanessa_socket-0.0.12-1.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-6632/vanessa_socket-0.0.12-1.fc16
then log in and leave karma (feedback).

Comment 29 Fedora Update System 2012-05-03 22:52:16 UTC
vanessa_socket-0.0.12-1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2012-05-04 23:05:46 UTC
vanessa_socket-0.0.12-1.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2012-05-15 02:08:52 UTC
vanessa_socket-0.0.12-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2012-05-15 02:11:50 UTC
vanessa_socket-0.0.12-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


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