Bug 980960 - Review Request: rpmgrill - A utility for catching problems in koji builds
Review Request: rpmgrill - A utility for catching problems in koji builds
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-03 12:36 EDT by Ed Santiago
Modified: 2013-10-28 10:45 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-10-20 22:34:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ed Santiago 2013-07-03 12:36:29 EDT
Spec URL: http://www.edsantiago.com/f/rpmgrill.spec
SRPM URL: http://www.edsantiago.com/f/rpmgrill-0.21-1.fc17.src.rpm
Description: rpmgrill runs a series of tests against a set of RPMs, reporting problems that may require a developer's attention.  For instance: unapplied patches, multilib incompatibilities.
Fedora Account System Username: santiago

This is my first package, and I need a sponsor. I am upstream for rpmgrill. Dhiru Kholia is working on a proposal for resources for this tool:

    https://gist.github.com/kholia/5899572

Koji scratch build:

   http://koji.fedoraproject.org/koji/taskinfo?taskID=5569692

rpmlint on srpm gripes about:

   rpmgrill.src: W: invalid-url Source0: rpmgrill-0.21.tar.bz2

...which I believe can be waived because the package itself is upstream, only really useful for Fedora and RHEL, and there seem to be other examples of this usage at:

   https://fedoraproject.org/wiki/Packaging:SourceURL

Three other rpmlint spelling-error warnings (on both srpm and noarch); all are false positives.
Comment 1 Christopher Meng 2013-07-03 12:55:13 EDT
Aha...

1. Where is the URL tag, that means, where is the program's homepage?

2. Source0 should be a link which can directly be used to download the tarball or other sources. Only this style is for softwares which cannot provide a tarball.

3. "Requires:       perl" is incorrect, please see:

https://fedoraproject.org/wiki/Packaging:Perl#Versioned_MODULE_COMPAT_Requires

4. "Requires: /usr/bin/desktop-file-validate" is right but I think you can change it to desktop-file-utils

5. No need for "%clean section"/"%defattr(-,root,root,-)"/"rm -rf $RPM_BUILD_ROOT"(old habits should be trashed)

6. Do not mix variable and macros, you can change $RPM_BUILD_ROOT to %{buildroot}

7. If there are no docs, you don't need to write %doc with an empty line.

8. /usr/share in %files section can be replaced by %{_datadir}

I'm not a sponsor, but please consider a fix for above issues.
Comment 2 Christopher Meng 2013-07-03 13:00:04 EDT
An additional note, I made the same mistake when I built my first RPM, that is you MUST NOT write the perl BuildRequires/Requires like "perl-Module-Build", don't use dash-style, but like this:

perl(Module::Build)

So perl-IO-Socket-SSL should be something like this:

perl(IO::Socket::SSL)

BTW, if RPM can automatically find these Requires(NOT BuildRequires, you still have to define which to be used for building), just use "Requires:  perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))" is enough.
Comment 3 Ed Santiago 2013-07-03 13:45:05 EDT
Many, many thanks. I believe I've addressed each issue you brought up.

Diffs: https://git.fedorahosted.org/cgit/rpmgrill.git/commit/?id=0d77b8b89517aa150ec8b995389661a711107d48

New spec: http://www.edsantiago.com/f/rpmgrill.spec
New srpm: http://www.edsantiago.com/f/rpmgrill-0.22-1.fc17.src.rpm

New koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5570468

Clarifying note: The requirement on perl(IO::Socket::SSL) was hand-specified because it's dynamically tested for by LWP::UserAgent inside an eval, so RPM couldn't autodetect it. I have removed the requirement from the specfile because that code isn't actually used in this version of rpmgrill.

Again, thanks for your thorough and helpful review. You clarified some packaging points I hadn't understood, and (blush) pointed out some that I missed in my haste.
Comment 4 Michael Scherer 2013-07-04 08:29:28 EDT
You didn't mention your FAS username.
Comment 5 Michael Scherer 2013-07-04 08:30:47 EDT
And BuildRoot is not really needed on Fedora, afaik, nor on EPEL 6
Comment 6 Till Maas 2013-07-06 06:44:12 EDT
see comment:4 and comment:5
Comment 7 Ed Santiago 2013-07-08 07:53:50 EDT
Fedora username: santiago

Buildroot: removed.

New spec: http://www.edsantiago.com/f/rpmgrill.spec
New srpm: http://www.edsantiago.com/f/rpmgrill-0.22-2.fc17.src.rpm
Comment 8 Christopher Meng 2013-07-23 06:16:12 EDT
I'll take this.
Comment 9 Christopher Meng 2013-07-23 21:10:46 EDT
This is an informal review, hope you can find a sponsor(I think it's easy for RH people):

1. SPEC itself:

- You can remove this:

find %{buildroot} -depth -type d -exec rmdir {} 2>/dev/null \;


- Requires: /usr/bin/xsltproc

can be 

  Requires: libxslt

- I cannot access 2 bugs mentioned in the spec, can you tell me something about them?

Package itself:

Unable to install:

 # ['/usr/bin/yum', '--installroot', '/var/lib/mock/fedora-rawhide-i386/root/', 'install', '/home/rpmaker/Desktop/rpmgrill/results/rpmgrill-0.22-2.fc20.noarch.rpm']
Error: Package: rpmgrill-0.22-2.fc20.noarch (/rpmgrill-0.22-2.fc20.noarch)
           Requires: perl(RPM::Grill::Util)
Comment 10 Ed Santiago 2013-07-23 22:19:30 EDT
Again, thanks for your feedback. I'm really not sure why I changed the libxslt requirement to a path, but it's back. And the internal BZ references are gone, replaced with what I hope are useful descriptions of why those packages are needed.

The missing RPM::Grill::Util is pretty embarrassing. Sorry about that.

New spec, srpm, noarch rpm :

   http://www.edsantiago.com/f/rpmgrill.spec
   http://www.edsantiago.com/f/rpmgrill-0.23-1.fc19.src.rpm
   http://www.edsantiago.com/f/rpmgrill-0.23-1.fc19.noarch.rpm

(RPM::Grill::Util does not include a perl-findable $VERSION; I've just checked in a fix for that but have not rebuilt the rpm).

You've been very thorough and helpful. Thank you.
Comment 11 Christopher Meng 2013-07-24 21:38:17 EDT
Hint:

http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

This time the package is good.

only

rpmgrill.noarch: E: explicit-lib-dependency libxslt

Is it really needed?
Comment 12 Ed Santiago 2013-07-24 21:50:39 EDT
> Hint:
> 
> http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I'm taking that as "hint: start helping out with reviews" -- I'm still a bit intimidated. But I'm reading. Thank you.

> rpmgrill.noarch: E: explicit-lib-dependency libxslt
> 
> Is it really needed?

The actual requirement is /usr/bin/xsltproc . One of the plugins invokes it via qx{...}. Is there a better way to specify the dependency?
Comment 13 Christopher Meng 2013-07-24 22:07:44 EDT
Not sure, leave it to your sponsor ;)
Comment 14 Michael Schwendt 2013-07-25 03:54:31 EDT
> - Requires: /usr/bin/xsltproc
> 
> can be 
> 
>   Requires: libxslt

Not a big issue, but: There is nothing wrong with requiring /usr/bin/xsltproc. It is more accurate than depending on a package name. The executable could move, for example.
Comment 15 Ed Santiago 2013-07-25 12:57:08 EDT
> Not a big issue, but: There is nothing wrong with requiring
> /usr/bin/xsltproc. It is more accurate than depending on a package name. The
> executable could move, for example.

Thank you. That does seem like the right thing to do, since it removes a potentially confusing (and fragile) level of indirection.
Comment 16 Dhiru Kholia 2013-09-07 15:14:47 EDT
$ rpmlint rpmgrill-0.23-1.fc19.src.rpm 
....
rpmgrill.src: E: specfile-error warning: bogus date in %changelog: Mon Jul 10 2013 Ed Santiago <santiago@redhat.com> 0.22-2
1 packages and 0 specfiles checked; 1 errors, 3 warnings.

Did you mean "Wed" instead of "Mon" ?
Comment 17 Ed Santiago 2013-09-09 07:55:01 EDT
Updated:

   http://www.edsantiago.com/f/rpmgrill.spec
   http://www.edsantiago.com/f/rpmgrill-0.24-1.fc19.src.rpm
   http://www.edsantiago.com/f/rpmgrill-0.24-1.fc19.noarch.rpm

Changelog:

   * Mon Sep  9 2013 Ed Santiago <santiago@redhat.com> 0.24-1
   - test suite: clamav output differs between f17 & f19
   - specfile: fix bad date in changelog; re-update some Requires
Comment 18 Parag AN(पराग) 2013-09-10 01:32:42 EDT
I have sponsored Ed now. Will check this package review.
Comment 19 Parag AN(पराग) 2013-09-11 11:33:41 EDT
review:-

+ mock build is successful

+ rpmlint on rpms gave
rpmgrill.noarch: W: spelling-error Summary(en_US) koji -> Kojak
rpmgrill.noarch: W: spelling-error %description -l en_US unapplied -> unappealing
rpmgrill.noarch: W: spelling-error %description -l en_US multilib -> multilingual
rpmgrill.src: W: spelling-error Summary(en_US) koji -> Kojak
rpmgrill.src: W: spelling-error %description -l en_US unapplied -> unappealing
rpmgrill.src: W: spelling-error %description -l en_US multilib -> multilingual
2 packages and 0 specfiles checked; 0 errors, 6 warnings.

+ source verified with upstream as (sha256sum)
srpm tarball : ace439ae0f1fb7bfcca1172cbdded6bd556a91df05f57ca9283b1ddca1e0e3e2
upstream tarball : ace439ae0f1fb7bfcca1172cbdded6bd556a91df05f57ca9283b1ddca1e0e3e2


+ Package rpmgrill-0.24-1.fc19.noarch
Provides: perl(RPM::Grill) = 0.24 perl(RPM::Grill::Plugin::AAATemplate) = 0.24 perl(RPM::Grill::Plugin::BuildLog) = 0.24 perl(RPM::Grill::Plugin::DesktopLint) = 0.24 perl(RPM::Grill::Plugin::ElfChecks) = 0.24 perl(RPM::Grill::Plugin::LibGather) = 0.24 perl(RPM::Grill::Plugin::ManPages) = 0.24 perl(RPM::Grill::Plugin::Manifest) = 0.24 perl(RPM::Grill::Plugin::Multilib) = 0.24 perl(RPM::Grill::Plugin::Patches) = 0.24 perl(RPM::Grill::Plugin::RpmScripts) = 0.24 perl(RPM::Grill::Plugin::SecurityPolicy) = 0.24 perl(RPM::Grill::Plugin::Setxid) = 0.24 perl(RPM::Grill::Plugin::SpecFileEncoding) = 0.24 perl(RPM::Grill::Plugin::SpecFileSanity) = 0.24 perl(RPM::Grill::Plugin::VirusCheck) = 0.24 perl(RPM::Grill::RPM) = 0.24 perl(RPM::Grill::RPM::Files) = 0.24 perl(RPM::Grill::RPM::Metadata) = 0.24 perl(RPM::Grill::RPM::SpecFile) = 0.24 perl(RPM::Grill::RPM::SpecFile::Line) perl(RPM::Grill::Util) = 0.24 perl(RPM::Grill::dprintf) = 0.24 rpmgrill = 0.24-1.fc19
 
Requires: /usr/bin/perl perl(Algorithm::Diff) perl(CGI) perl(Carp) perl(Encode) perl(Errno) perl(Fcntl) perl(File::Basename) perl(File::Copy) perl(File::LibMagic) perl(File::Path) perl(File::Temp) perl(File::Which) perl(Getopt::Long) perl(HTML::Entities) perl(IPC::Run) perl(JSON::XS) perl(LWP::Simple) perl(List::Util) perl(Pod::POM) perl(Pod::POM::View::HTML) perl(RPM::Grill) perl(RPM::Grill::Plugin::SecurityPolicy) perl(RPM::Grill::RPM) perl(RPM::Grill::RPM::Files) perl(RPM::Grill::RPM::Metadata) perl(RPM::Grill::Util) perl(RPM::Grill::dprintf) perl(Sort::Versions) perl(Text::ParseWords) perl(Tie::File) perl(Time::ParseDate) perl(Time::Piece) perl(XML::Simple) perl(YAML) perl(YAML::Syck) perl(base) perl(constant) perl(open) perl(overload) perl(parent) perl(strict) perl(utf8) perl(warnings)


Looks good but I can't find license file or license text in any perl module file. If you are the upstream then try to include it and release new tarball and package it here.
Comment 21 Parag AN(पराग) 2013-09-11 23:59:00 EDT
You should now package LICENSE file also.
add %doc LICENSE in spec file.


APPROVED.
Comment 22 Ed Santiago 2013-09-13 11:07:51 EDT
LICENSE (and README.AAAA_FIRST) now packaged:

   http://www.edsantiago.com/f/rpmgrill.spec
   http://www.edsantiago.com/f/rpmgrill-0.25-2.fc19.src.rpm
   http://www.edsantiago.com/f/rpmgrill-0.25-2.fc19.noarch.rpm

It looks like I should package a ChangeLog as well; now on my todo list.
Comment 23 Ed Santiago 2013-09-13 11:19:02 EDT
New Package SCM Request
=======================
Package Name: rpmgrill
Short Description: 
Owners: santiago halfie
Branches: f19 f20
InitialCC:
Comment 24 Parag AN(पराग) 2013-09-13 11:31:37 EDT
Hi Ed,
your request should include this
Short Description: A utility for catching problems in koji builds

you can also read how to request SCM for your package at http://fedoraproject.org/wiki/Package_SCM_admin_requests

you should re-submit above request with correct input again.
Comment 25 Ed Santiago 2013-09-13 11:35:24 EDT
New Package SCM Request
=======================
Package Name: rpmgrill
Short Description: A utility for catching problems in koji builds
Owners: santiago halfie
Branches: f19 f20
InitialCC:
Comment 26 Gwyn Ciesla 2013-09-13 11:39:37 EDT
Git done (by process-git-requests).
Comment 27 Christopher Meng 2013-10-20 22:34:19 EDT
Built on rawhide, however no builds on f20/f19.

Closed for Rawhide.
Comment 28 Christopher Meng 2013-10-28 10:29:22 EDT
You are not authorized to access bug #876702.
Comment 29 Ed Santiago 2013-10-28 10:45:18 EDT
> You are not authorized to access bug #876702.

Sorry! It's an internal RFE for opening rpmgrill. As part of closing it, I linked to this bz -- forgetting the symmetrical nature of the link.

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