Bug 920751 - Review Request: satyr - A set of tools to create anonymous, machine-friendly problem reports
Summary: Review Request: satyr - A set of tools to create anonymous, machine-friendly ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-03-12 16:25 UTC by Martin Milata
Modified: 2013-03-21 13:01 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-03-21 13:01:13 UTC
Type: ---
Embargoed:
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Martin Milata 2013-03-12 16:25:13 UTC
Spec URL: http://mmilata.fedorapeople.org/satyr.spec
SRPM URL: http://mmilata.fedorapeople.org/satyr-0.1-1.fc19.src.rpm
Fedora Account System Username: mmilata
Description:

satyr is a descendant of the btparser package that aims to create and process lightweight anonymous bug reports which can then be submitted to services such as https://retrace.fedoraproject.org/faf/

Comment 1 Martin Milata 2013-03-14 16:40:48 UTC
Based on input from IRC, I have changed package summary and description, and uploaded new versions of spec and SRPM.

Spec URL: http://mmilata.fedorapeople.org/satyr.spec
SRPM URL: http://mmilata.fedorapeople.org/satyr-0.1-1.fc19.src.rpm
Fedora Account System Username: mmilata
Description:

Satyr is a library that can be used to create and process microreports.
Microreports consist of structured data suitable to be analyzed in a fully
automated manner, though they do not necessarily contain sufficient information
to fix the underlying problem. The reports are designed not to contain any
potentially sensitive data to eliminate the need for review before submission.
Included is a tool that can create microreports and perform some basic
operations on them.

Comment 2 Michael Schwendt 2013-03-16 22:20:21 UTC
* Run "rpmlint -i" on the src.rpm and all built rpms. Some of the reported warnings/errors may be dubious (false positives), however. Be aware of that.


> Source0: https://fedorahosted.org/released/abrt/satyr-%{version}.tar.xz

Checksum of included tarball doesn't match upstream. The sources downloaded with "spectool -g satyr.spec" are two days older and include an older satyr.spec.in file.


> Requires: elfutils-libelf, elfutils-libs, binutils
> Requires: libunwind >= 1.1

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


> Summary: A set of tools to create anonymous, machine-friendly problem reports

  Summary: Tools to create anonymous, machine-friendly problem reports

Would be even more concise (and dropping those leading articles is good with regard to displaying package summaries in Anaconda and in package installer tools). Interestingly, the spec file included within the source tarball contains the following summary:

  Summary: Automatic problem management with anonymous reports


> Name: satyr
> Group: Development/Libraries

"System Environment/Libraries" is the group for shared library base packages (regardless of whether it includes also tools).


> find %{buildroot} -regex ".*\.la$" | xargs rm -f --

Amazingly/unnecessarily complex. Basic wildcard expression should suffice, and "rm" doesn't need "-f" because if no files were found, xargs would not run "rm" at all:

  find %{buildroot} -name \*.la | xargs rm


> %{_mandir}/man1/%{name}.1.gz

%{_mandir}/man1/%{name}.1*   to allow for changing/reconfigured compression or even no compression.


> THIS MANPAGE IS NOT OUT OF DATE AND THEREFORE USELESS.

Uh? ;-)


> %doc README NEWS COPYING TODO ChangeLog

"btparser 0.18"? That's odd, because a newer btparser (0.25) is in F-19 development, so something ought to be done about these %doc files. The name "satyr" does not appear anywhere in them.


* The build.log contains failing searches for "pdflatex", "doxygen" and "dot" for building the optional API documentation files.


> /usr/include/satyr/config.h

Caution. Exposing this file as a public API header bears a risk of leading to trouble (because some of definitions in it will likely conflict with a libsatyr based program that will use an own config.h header).

$ grep config.h *
callgraph.h:#include "config.h"
config.h:/* lib/config.h.  Generated from config.h.in by configure.  */
config.h:/* lib/config.h.in.  Generated from configure.ac by autoheader.  */
core_fingerprint.h:#include "config.h"
disasm.h:#include "config.h"

Comment 3 Martin Milata 2013-03-18 17:31:42 UTC
Thank you for your feedback, Michael! I tried to address the issues and uploaded new spec and SRPM.

Spec URL: http://mmilata.fedorapeople.org/satyr.spec
SRPM URL: http://mmilata.fedorapeople.org/satyr-0.2-1.fc20.src.rpm

(In reply to comment #2)
> * Run "rpmlint -i" on the src.rpm and all built rpms. Some of the reported
> warnings/errors may be dubious (false positives), however. Be aware of that.

rpmlint still gives me spelling-error, shared-lib-calls-exit (to be fixed later
with other error handling issues), no-documentation (for the subpackages, the
main package has documentation) and private-shared-object-provides (caused by
the python bindings).

> > Source0: https://fedorahosted.org/released/abrt/satyr-%{version}.tar.xz
> 
> Checksum of included tarball doesn't match upstream. The sources downloaded
> with "spectool -g satyr.spec" are two days older and include an older
> satyr.spec.in file.

Should not happen with the new version.

> > Requires: elfutils-libelf, elfutils-libs, binutils
> > Requires: libunwind >= 1.1
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

The requires were removed.

> > Summary: A set of tools to create anonymous, machine-friendly problem reports
> 
>   Summary: Tools to create anonymous, machine-friendly problem reports
> 
> Would be even more concise (and dropping those leading articles is good with
> regard to displaying package summaries in Anaconda and in package installer
> tools). Interestingly, the spec file included within the source tarball
> contains the following summary:
> 
>   Summary: Automatic problem management with anonymous reports

Changed, thanks for the suggestion.

> > Name: satyr
> > Group: Development/Libraries
> 
> "System Environment/Libraries" is the group for shared library base packages
> (regardless of whether it includes also tools).

Changed.

> > find %{buildroot} -regex ".*\.la$" | xargs rm -f --
> 
> Amazingly/unnecessarily complex. Basic wildcard expression should suffice,
> and "rm" doesn't need "-f" because if no files were found, xargs would not
> run "rm" at all:
> 
>   find %{buildroot} -name \*.la | xargs rm

Fixed.

> > %{_mandir}/man1/%{name}.1.gz
> 
> %{_mandir}/man1/%{name}.1*   to allow for changing/reconfigured compression
> or even no compression.

Fixed.

> > THIS MANPAGE IS NOT OUT OF DATE AND THEREFORE USELESS.
> 
> Uh? ;-)

Ehm:) I replaced the manual page with slightly saner version.

> > %doc README NEWS COPYING TODO ChangeLog
> 
> "btparser 0.18"? That's odd, because a newer btparser (0.25) is in F-19
> development, so something ought to be done about these %doc files. The name
> "satyr" does not appear anywhere in them.
> 
> 
> * The build.log contains failing searches for "pdflatex", "doxygen" and
> "dot" for building the optional API documentation files.

I cleaned up the docs a bit, they should not refer to btparser anymore.
Building the API documentation is disabled for the time being.

> > /usr/include/satyr/config.h
> 
> Caution. Exposing this file as a public API header bears a risk of leading
> to trouble (because some of definitions in it will likely conflict with a
> libsatyr based program that will use an own config.h header).
> 
> $ grep config.h *
> callgraph.h:#include "config.h"
> config.h:/* lib/config.h.  Generated from config.h.in by configure.  */
> config.h:/* lib/config.h.in.  Generated from configure.ac by autoheader.  */
> core_fingerprint.h:#include "config.h"
> disasm.h:#include "config.h"

Thanks for the tip. I removed config.h from the set of exported headers and
from the files included by the exported headers.

Comment 4 Michael Schwendt 2013-03-20 20:16:21 UTC
The %changelog could have mentioned the update from 0.1 to 0.2 more explicitly. It is good habit to explicitly mention that the source tarball has been replaced and to sum up what sort of update it is (in case it's possible to classify the update in a concise way), e.g. whether it's just a maintenance release, whether it includes just a few bug-fixes or also new features, major rewrites or minor changes.

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

[...]

Other than that, you've fixed the only issues with the packaging, and hereby it is

APPROVED

Comment 5 Martin Milata 2013-03-21 09:57:38 UTC
Thank you for your time. I'll try to keep these suggestions in mind.

Comment 6 Martin Milata 2013-03-21 09:59:58 UTC
New Package SCM Request
=======================
Package Name: satyr
Short Description: Tools to create anonymous, machine-friendly problem reports
Owners: mmilata
Branches: f18 f19 el6
InitialCC: abrt-team jmoskovc

Comment 7 Gwyn Ciesla 2013-03-21 12:20:04 UTC
Git done (by process-git-requests).


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