Bug 499319

Summary: Review Request: tcl-snmptools - TCL extension for SNMP support
Product: [Fedora] Fedora Reporter: Bryson Lee <bamablee>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, guido.grazioli, notting, susi.lehtola
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-08-01 16:59:59 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    

Description Bryson Lee 2009-05-06 02:00:26 EDT
Spec URL: http://www.slac.stanford.edu/~blee/tcl-snmptools.spec
SRPM URL: http://www.slac.stanford.edu/~blee/tcl-snmptools-1.0-1.fc10.src.rpm
Description:
Tcl SNMP Tools is a Tcl package that provides SNMP tools for managing
remote Agents. It uses the NetSNMP library and supports all standard
SNMP v1/v2/v3 operations and more: get, set, getnext, walk, bulkget,
bulkwalk, trap, translate, and table.

This is my first package for Fedora, and so I need a sponsor for this submission.
Comment 1 Bryson Lee 2009-05-07 13:32:48 EDT
Spec URL: http://www.slac.stanford.edu/~blee/tcl-snmptools.spec
SRPM URL: http://www.slac.stanford.edu/~blee/tcl-snmptools-1.0-2.fc10.src.rpm

I updated the specfile to include the --disable-threads command-line option for the configure step.  This eliminates a warning from configure when the extension is built against the stock tcl-devel, which disables threading due to fork() problems.
Comment 2 Guido Grazioli 2009-05-07 19:30:51 EDT
Hi Bryson, this is just an informal review, because i'm not a sponsored reviewer yet.

OK - rpmlint
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
OK - The package must be named according to the  Package Naming Guidelines.
OK - The spec file name must match the base package %{name}

NEEDSWORK - The package must meet the Packaging Guidelines
- use %global instead of %define

NEEDSWORK - 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 must be included in %doc
- include the file license.terms in %doc

NEEDSWORK - The package must be licensed with a Fedora approved license and meet the Licensing Guidelines
- some files are under CMU license (BSD like), fix the License tag

NEEDSWORK - Every binary RPM package which stores shared library files must call ldconfig in %post and %postun 
- add ldconfig calls

OK - The package MUST successfully compile and build
koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1342406
OK - The spec file must be written in American English.
OK - The spec file for the package MUST be legible.
OK - The sources used to build the package must match the upstream source, as provided in the spec URL. 
ddbd09e2e39e2efa4155981317ec5394  tcl-snmptools-1.0.tar.gz
NA - The spec file MUST handle locales properly (no translations)
NA - package not relocatable
OK - A package must own all directories that it creates
OK - A Fedora package must not list a file more than once in the spec file's %files listings
OK - Permissions on files must be set properly
OK - Each package must have a %clean section
OK - Each package must consistently use macros
OK - The package must contain code, or permissable content (no content)
NA - Large documentation files must go in a -doc subpackage (no large doc)
OK - If a package includes something as %doc, it must not affect the runtime of the application
NA - Header files must be in a -devel package (no devel package)
NA - Static libraries must be in a -static package (no static package)
NA - Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
OK - Packages must NOT contain any .la libtool archives
NA - Packages containing GUI applications must include a .desktop file (no gui)
OK - At the beginning of %install, each package MUST run rm -rf %{buildroot}
OK - All filenames in rpm packages must be valid UTF-8

Hope that helps, regards
Comment 3 Bryson Lee 2009-05-07 20:33:26 EDT
Hi Guido, thanks for the review!

Spec URL: http://www.slac.stanford.edu/~blee/tcl-snmptools.spec
SRPM URL: http://www.slac.stanford.edu/~blee/tcl-snmptools-1.0-3.fc10.src.rpm

> - use %global instead of %define

Corrected.

> - include the file license.terms in %doc

Corrected.

> - some files are under CMU license (BSD like), fix the License tag

Corrected to "CMU and GPLv3+".


> NEEDSWORK - Every binary RPM package which stores shared library files must
> call ldconfig in %post and %postun
> - add ldconfig calls

Although the guidelines don't say so explicitly, I don't think it makes sense to apply this requirement to shareables that are extension modules for interpreted languages like TCL, since the code isn't activated by ld itself, but rather by the interpreter (e.g. /usr/bin/tclsh) with calls to dlopen().  This specfile was based on an existing Fedora 10 specfile for the tcl-zlib extension, which does not perform ldconfig calls, either.

Best regards,

Bryson
Comment 4 Guido Grazioli 2009-05-09 05:02:19 EDT
(In reply to comment #3)
> > NEEDSWORK - Every binary RPM package which stores shared library files must
> > call ldconfig in %post and %postun
> > - add ldconfig calls
> 
> Although the guidelines don't say so explicitly, I don't think it makes sense
> to apply this requirement to shareables that are extension modules for
> interpreted languages like TCL, since the code isn't activated by ld itself,
> but rather by the interpreter (e.g. /usr/bin/tclsh) with calls to dlopen(). 
> This specfile was based on an existing Fedora 10 specfile for the tcl-zlib
> extension, which does not perform ldconfig calls, either.

Hello Bryson, ok i think you are right on this one. Some tcl packages have the ldconfig command indeed, however you convinced me that it doesnt actually makes sense. 

About the NetSnmp library you ship and build with your package, isnt it already available on Fedora? you should try to avoid library duplicates in that case.

Cheers
Comment 5 Susi Lehtola 2009-05-09 05:51:33 EDT
I am sponsoring Guido, so I will do the review on this package.

Bryson: do you still need a sponsor? I can sponsor you, if you do some reviews and submit a couple of other packages for review (other than tcl packages, that is).

- Why do you disable threading?
Comment 6 Susi Lehtola 2009-05-09 06:37:06 EDT
(In reply to comment #5)
> - Why do you disable threading?  
Duh, you explaiend it in comment #1.


- You are not providing snmptools, http://fedoraproject.org/wiki/Packaging/Tcl#Naming_Conventions . This is OK, considering the genericish base name of the package.

- Doesn't the version macro at 
 http://fedoraproject.org/wiki/Packaging/Tcl#arch-specific_packages
work? Where does it fail?

- Have you tried the configure argument --libdir=%{tcl_sitearch} to change the default install location?

- Do you really need to specify --with-tcl=%{_libdir}? OK, on multiarch arches with both 32- and 64-bit versions installed you might get into trouble if configure picks up the wrong version. No hurt having this if you think it's necessary.



rpmlint output:
tcl-snmptools.src: W: invalid-license CMU
tcl-snmptools.x86_64: W: invalid-license CMU
tcl-snmptools-debuginfo.x86_64: W: invalid-license CMU
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

- License tag should be MIT (see http://fedoraproject.org/wiki/Licensing ). But this is really not necessary, since CMU is compatible with GPL and License: GPLv3+ is enough.

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSFIX
- See comment above, license tag should be just GPLv3+.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
UST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSFIX
- Even though the files that are installed are generated, it's always nice to preserve time stamps in install phase. Use INSTALL="install -p" as argument to make install.

MUST: Packages containing shared library files must call ldconfig. N/A

MUST: A package must own all directories that it creates or require the package that owns the directory. ~OK
- Instead of
 %dir %{tcl_sitearch}/%{realname}%{version}
 %{tcl_sitearch}/%{realname}%{version}/*.so
 %{tcl_sitearch}/%{realname}%{version}/pkgIndex.tcl
just put
 %{tcl_sitearch}/%{realname}%{version}/
as this will own the directory and everything in it.

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSFIX
- Add AUTHORS. BUGS and TODO should otherwise be included, but now they just contain instructions to grep the code.

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 7 Susi Lehtola 2009-05-09 06:41:54 EDT
(In reply to comment #2)
> NEEDSWORK - The package must be licensed with a Fedora approved license and
> meet the Licensing Guidelines
> - some files are under CMU license (BSD like), fix the License tag

CMU is compatible with GPL, and the CMU files are compiled with the GPL files, resulting in a binary that is solely under GPL. Thus license tag is GPLv3+.
 
> NEEDSWORK - Every binary RPM package which stores shared library files must
> call ldconfig in %post and %postun 
> - add ldconfig calls

This wouldn't do anything since no libraries are installed in standard library directories (and no file is shipped into /etc/ld.so.conf.d/). :)

(In reply to comment #4)
> About the NetSnmp library you ship and build with your package, isnt it already
> available on Fedora? you should try to avoid library duplicates in that case.

There's no netsnmp library in the package, it links to the netsnmp library.
Comment 8 Guido Grazioli 2009-05-09 08:00:48 EDT
(In reply to comment #7)
> (In reply to comment #2)
> > NEEDSWORK - The package must be licensed with a Fedora approved license and
> > meet the Licensing Guidelines
> > - some files are under CMU license (BSD like), fix the License tag
> 
> CMU is compatible with GPL, and the CMU files are compiled with the GPL files,
> resulting in a binary that is solely under GPL. Thus license tag is GPLv3+.
> 

Ok, my bad, got that one now.

> > NEEDSWORK - Every binary RPM package which stores shared library files must
> > call ldconfig in %post and %postun 
> > - add ldconfig calls
> 
> This wouldn't do anything since no libraries are installed in standard library
> directories (and no file is shipped into /etc/ld.so.conf.d/). :)
> 

ok; i tough that you might have /usr/lib/tcl8.5 in a file under /etc/ld.so.conf.d, provided by some other tcl- package.


> (In reply to comment #4)
> > About the NetSnmp library you ship and build with your package, isnt it already
> > available on Fedora? you should try to avoid library duplicates in that case.
> 
> There's no netsnmp library in the package, it links to the netsnmp library.  

Sorry, i was a bit confused here. This package ships the sources of the snmp tools, which are part of the upstream project, and are shipped with Fedora in net-snmp-utils.
Comment 9 Susi Lehtola 2009-05-15 12:15:12 EDT
ping?
Comment 10 Susi Lehtola 2009-06-08 15:00:12 EDT
ping again?
Comment 11 Bryson Lee 2009-06-16 01:19:57 EDT
Sorry for the hiatus...(In reply to comment #6).

> 
> - Doesn't the version macro at 
>  http://fedoraproject.org/wiki/Packaging/Tcl#arch-specific_packages
> work? Where does it fail?
> 
> - Have you tried the configure argument --libdir=%{tcl_sitearch} to change the
> default install location?
>

Still need to investigate these two items.
 
> - Do you really need to specify --with-tcl=%{_libdir}? OK, on multiarch arches
> with both 32- and 64-bit versions installed you might get into trouble if
> configure picks up the wrong version. No hurt having this if you think it's
> necessary.
> 

I had exactly this issue when building originally on an x86_64 machine

> - License tag should be MIT (see http://fedoraproject.org/wiki/Licensing ). But
> this is really not necessary, since CMU is compatible with GPL and License:
> GPLv3+ is enough.
> 

> MUST: The License field in the package spec file must match the actual license.
> NEEDSFIX
> - See comment above, license tag should be just GPLv3+.
> 

It appears that the author has licensed his TCL-specific wrapper code under GPLv3+; however, the NetSNMP source files that are also included in the package explicitly call out the CMU license.  Hence my original choice of "CMU and GPLv3+".  I have no particular axe to grind about this, so if just GPLv3+ is sufficient I'll update the specfile accordingly.

> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. OK
> UST: The package MUST successfully compile and build into binary rpms. OK
> MUST: The spec file MUST handle locales properly. N/A
> 
> MUST: Optflags are used and time stamps preserved. NEEDSFIX
> - Even though the files that are installed are generated, it's always nice to
> preserve time stamps in install phase. Use INSTALL="install -p" as argument to
> make install.
> 

Will do.

> MUST: Packages containing shared library files must call ldconfig. N/A
> 
> MUST: A package must own all directories that it creates or require the package
> that owns the directory. ~OK
> - Instead of
>  %dir %{tcl_sitearch}/%{realname}%{version}
>  %{tcl_sitearch}/%{realname}%{version}/*.so
>  %{tcl_sitearch}/%{realname}%{version}/pkgIndex.tcl
> just put
>  %{tcl_sitearch}/%{realname}%{version}/
> as this will own the directory and everything in it.
> 

Right...wondered about that.  A couple of other Fedora TCL extension packages I looked at did it as in the current iteration of the specfile, so I went with that approach.  Will adjust to use %dir instead.

> MUST: Files only listed once in %files listings. OK
> MUST: Debuginfo package is complete. OK
> MUST: Permissions on files must be set properly. OK
> MUST: Clean section exists. OK
> MUST: Large documentation files must go in a -doc subpackage. N/A
> 
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSFIX
> - Add AUTHORS. BUGS and TODO should otherwise be included, but now they just
> contain instructions to grep the code.
> 

Will do.
Comment 12 Bryson Lee 2009-06-17 01:02:55 EDT
Posted updated spec and SRPM:

http://www.slac.stanford.edu/~blee/tcl-snmptools.spec
http://www.slac.stanford.edu/~blee/tcl-snmptools-1.0-4.fc10.src.rpm

Addressed the following items:

> - Doesn't the version macro at 
>  http://fedoraproject.org/wiki/Packaging/Tcl#arch-specific_packages
> work? Where does it fail?
>

It works when I build locally.  There was a comment in the original specfile I used as a template to the effect that determining the TCL version programmatically failed on the Fedora build system because tclsh was not installed by default.  I've removed the comment and updated to the documented version macro on the assumption that it's working now.
 
> - Have you tried the configure argument --libdir=%{tcl_sitearch} to change the
> default install location?
> 
> - Do you really need to specify --with-tcl=%{_libdir}? OK, on multiarch arches
> with both 32- and 64-bit versions installed you might get into trouble if
> configure picks up the wrong version. No hurt having this if you think it's
> necessary.
> 

--libdir=%{tcl_sitearch} appears to work, including on my multiarch machine.

> this is really not necessary, since CMU is compatible with GPL and License:
> GPLv3+ is enough.
>

> MUST: The License field in the package spec file must match the actual license.
> NEEDSFIX
> - See comment above, license tag should be just GPLv3+.

License tag updated to GPLv3+ only.

> 
> MUST: Optflags are used and time stamps preserved. NEEDSFIX
> - Even though the files that are installed are generated, it's always nice to
> preserve time stamps in install phase. Use INSTALL="install -p" as argument to
> make install.
>

Added INSTALL="install -p" as suggested.
 
> just put
>  %{tcl_sitearch}/%{realname}%{version}/
> as this will own the directory and everything in it.
>

Done.
 
> - Add AUTHORS. BUGS and TODO should otherwise be included, but now they just
> contain instructions to grep the code.

Done.

Thanks,

Bryson
Comment 13 Bryson Lee 2009-06-19 12:20:46 EDT
Posted updated spec and SRPM:

http://www.slac.stanford.edu/~blee/tcl-snmptools.spec
http://www.slac.stanford.edu/~blee/tcl-snmptools-1.0-5.fc10.src.rpm

This update includes a patch called tcl-snmptools-redirection.patch that addresses a somewhat nasty misfeature of the extension whereby any redirections of stdout/stderr for extension clients are lost when commands provided by the extension are invoked.

The extension author freopen()'s stdout / stderr in order to capture output from the underlying NetSNMP library calls for inclusion in the TCL result string, and then does not restore the original streams correctly.

The patch has been submitted upstream as 

tcl-snmptools-Bugs-2808814: stdout, stderr redirections lost
https://sourceforge.net/tracker/?func=detail&atid=1035638&aid=2808814&group_id=215927
Comment 14 Susi Lehtola 2009-06-19 15:11:37 EDT
rpmlint output:
tcl-snmptools.x86_64: W: incoherent-version-in-changelog 1.0-4 ['1.0-5.fc11', '1.0-5']
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- You are missing the latest version from the changelog.

- The line
 install -d %{buildroot}%{tcl_sitearch}
should be dropped since it doesn't do anything (it's after the install command).

- You seem to be missing BR: tcl, since
Mock Version: 0.9.16
ENTER do(['bash', '--login', '-c', 'rpmbuild -bs --target x86_64 --nodeps builddir/build/SPECS/tcl-snmptools.spec'], False, '/var/mock/fedora-11-x86_64/root/', None, 0, True, 0, 500, 494, None, logger=<mock.trace_decorator.getLog object at 0xc86c50>)
Executing command: ['bash', '--login', '-c', 'rpmbuild -bs --target x86_64 --nodeps builddir/build/SPECS/tcl-snmptools.spec']
sh: tclsh: command not found
sh: tclsh: command not found
sh: tclsh: command not found

- A comment on style: I suggest using trailing slashes for directories in the %files section:
 %{tcl_sitearch}/%{realname}%{version}/
instead of
 %{tcl_sitearch}/%{realname}%{version}/


Apart from these minor issues I think the package is good to go.

Do you have other submissions? Have you done informal reviews of other people's packages? Before I sponsor you, you need to show me you're worth it :)
Comment 15 Susi Lehtola 2009-07-05 06:37:27 EDT
ping?
Comment 16 Susi Lehtola 2009-07-22 08:36:37 EDT
ping again Bryson
Comment 17 Susi Lehtola 2009-08-01 16:59:59 EDT
Closing due to inactive submitter.