Bug 1121082 - Review Request: rubygem-clockwork - A scheduler process to replace cron
Summary: Review Request: rubygem-clockwork - A scheduler process to replace cron
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: František Dvořák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-18 10:29 UTC by Josef Stribny
Modified: 2016-01-04 05:53 UTC (History)
4 users (show)

Fixed In Version: rubygem-clockwork-0.7.7-3.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-08-27 07:34:06 UTC
Type: ---
Embargoed:
valtri: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Josef Stribny 2014-07-18 10:29:41 UTC
Spec URL: http://data-strzibny.rhcloud.com/obs/rubygem-clockwork.spec
SRPM URL: http://data-strzibny.rhcloud.com/obs/rubygem-clockwork-0.7.7-1.fc22.src.rpm
Description: A scheduler process to replace cron, using a more flexible Ruby syntax running
as a single long-running process.  Inspired by rufus-scheduler and
resque-scheduler.
Fedora Account System Username: jstribny

Comment 1 František Dvořák 2014-08-21 11:34:27 UTC
Taking the review...

Can we do review swap with #1131991 (rubygem-logstash-event)?

Comment 2 František Dvořák 2014-08-21 19:46:41 UTC
1) missing license text (some MIT variants even require to distribute it with the sources), upstream needs to be notified; the good thing is the README links to exact MIT variant

[http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text]

(This week I bumped to the same problem with logstasher :-), I'll submit it for review later, there were other issues yet...)


2) missing man-pages: packagers should work with upstream to add them, but it is not strictly required by guidelines

[https://fedoraproject.org/wiki/Packaging:Guidelines?rd=PackagingGuidelines#Man_pages]

I wrote simple man-page for clockworkd (with texts from help a README), it could be used (and maybe improved): http://scientific.zcu.cz/fedora/REVIEWS/clockworkd.1


3) you could prepare commented out exact steps in %check, for minitest I have seen this magic formula in the ruby list:

  ruby -Ilib -e 'Dir.glob "./test/**/*_test.rb", &method(:require)'

I was able to launch the tests this way, after installing dependencies and 'gem install contests'.


4) cosmetic: timestamp of the source gem in .src.rpm should be rather 2014-07-05

curl and wget supports setting of "remote" timestamp, but it is not enabled by default [http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps]

Comment 3 Josef Stribny 2014-08-22 11:17:31 UTC
> 1) missing license text (some MIT variants even require to distribute it with > the sources), upstream needs to be notified; the good thing is the README
> links to exact MIT variant

True, I asked the upstream to include it[1].

> 2) missing man-pages: packagers should work with upstream to add them, but it 
> is not strictly required by guidelines

Yes, that would be nice. Do you have the source file for the man page? For example I used asciidoc format in sdoc man pages. Or did you really wrote this man page as it is? I would rather submit the source to upstream. 

Nevertheless, I included your man page in the spec file. Depending on your answer I will submit it to upstream and link the issue.

>3) you could prepare commented out exact steps in %check, for minitest I have
> seen this magic formula in the ruby list:

Well, you are combing RPM packaged gems with upstream ones. Of course you can also run only upstream test suite with every gem which is why I don't see the reason to put it there. When we have everything in Fedora, I would add the proper check section.

4) cosmetic: timestamp of the source gem in .src.rpm should be rather 2014-07-05

I admit that I don't really care about this much. Would that affect Fedora users somehow? Most of us download .gem files from RubyGems.org by `gem fetch` command.

Spec URL: http://data-strzibny.rhcloud.com/obs/rubygem-clockwork.spec
SRPM URL: http://data-strzibny.rhcloud.com/obs/rubygem-clockwork-0.7.7-2.fc22.src.rpm


[1] https://github.com/tomykaira/clockwork/issues/116

Comment 4 František Dvořák 2014-08-22 12:10:54 UTC
(In reply to Josef Stribny from comment #3)
> > 2) missing man-pages: packagers should work with upstream to add them, but it 
> > is not strictly required by guidelines
> 
> Yes, that would be nice. Do you have the source file for the man page? For
> example I used asciidoc format in sdoc man pages. Or did you really wrote
> this man page as it is? I would rather submit the source to upstream. 
> 
> Nevertheless, I included your man page in the spec file. Depending on your
> answer I will submit it to upstream and link the issue.
> 

Yes, I wrote it directly. Of course using some preferred ruby way instead would be also possible improvement.

> >3) you could prepare commented out exact steps in %check, for minitest I have
> > seen this magic formula in the ruby list:
> 
> Well, you are combing RPM packaged gems with upstream ones. Of course you
> can also run only upstream test suite with every gem which is why I don't
> see the reason to put it there. When we have everything in Fedora, I would
> add the proper check section.
> 

Yes, I liked the idea have to have it ready in .spec just to uncomment, but that's really up to package maintainer. :-)

> 4) cosmetic: timestamp of the source gem in .src.rpm should be rather
> 2014-07-05
> 
> I admit that I don't really care about this much. Would that affect Fedora
> users somehow? Most of us download .gem files from RubyGems.org by `gem
> fetch` command.
> 

Right, that's just cosmetic and I don't think it will affect anything.

Comment 5 Josef Stribny 2014-08-22 14:22:46 UTC
Ok, I asked upstream to include it[1]. I would include this link above the source in the spec file before pushing. Anything else?


[1] https://github.com/tomykaira/clockwork/pull/117

Comment 6 František Dvořák 2014-08-22 16:58:06 UTC
1) there is still a problem with the license:

In this case we should also include local copy of the license text, because this MIT variant requires it:

"The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software."

The most important work of contacting upstream is done, though...


2) you can consider removing "ruby(release)" and "ruby" BuildRequires (rubygems already picks it)


And could you review https://bugzilla.redhat.com/show_bug.cgi?id=1131991 (rubygem-logstash-event)? :-)

Comment 7 Josef Stribny 2014-08-25 06:05:49 UTC
> 1) there is still a problem with the license:

I have done what I had to: ask upstream to include it. I didn't create the file in the end at they link to a template that they should fill in (author & year) and I am not sure what to put there.

> 2) you can consider removing "ruby(release)" and "ruby" BuildRequires (rubygems already picks it)

Yes, it's redundant, I will remove it.

Comment 8 Josef Stribny 2014-08-25 06:54:02 UTC
I am sorry, it was just added by upstream[0]. I am going to include it and submit an updated srpm.


[0] https://github.com/tomykaira/clockwork/commit/13e78ddb31413fdbc26028577d5fcfbb6913714f

Comment 10 František Dvořák 2014-08-25 12:55:27 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- Sources used to build the package match the upstream source, as provided in
  the spec URL.
  Note: Upstream MD5sum check error, diff is in /home/valtri/fedora-
  scm/REVIEWS/rubygem-clockwork/1121082-rubygem-clockwork/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL

  Updated manpage.

- You should add '-p' parameter to install commands to preserve timestamps.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 11 files have unknown license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/gems,
     /usr/share/gems/doc
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Ruby:
[x]: Platform dependent files must all go under %{gem_extdir_mri}, platform
     independent under %{gem_dir}.
[x]: Gem package must not define a non-gem subpackage
[x]: Macro %{gem_extdir} is deprecated.
[x]: Gem package is named rubygem-%{gem_name}
[x]: Package contains BuildRequires: rubygems-devel.
[x]: Gem package must define %{gem_name} macro.
[x]: Pure Ruby package must be built as noarch
[x]: Package does not contain Requires: ruby(abi).

===== SHOULD items =====

Generic:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Avoid bundling fonts in non-fonts packages.
     Note: Package contains font files

They're in the generated docs.

[x]: 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]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Package functions as described.

Tested:
  ruby -e 'require "clockwork"'
  clockwork example.rb

[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.

Solved with upstream, good work!

[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[!]: Packages should try to preserve timestamps of original installed files.

You should add '-p' parameter to install.

[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Ruby:
[!]: Test suite of the library should be run.

OK, explained in comments there is missing dependency.

[x]: Specfile should use macros from rubygem-devel package.
[x]: Gem package should exclude cached Gem.
[x]: Gem should use %gem_install macro.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rubygem-clockwork-0.7.7-3.fc22.noarch.rpm
          rubygem-clockwork-doc-0.7.7-3.fc22.noarch.rpm
          rubygem-clockwork-0.7.7-3.fc22.src.rpm
rubygem-clockwork.noarch: W: spelling-error Summary(en_US) cron -> corn, con, crone
rubygem-clockwork.noarch: W: spelling-error %description -l en_US cron -> corn, con, crone
rubygem-clockwork.noarch: W: spelling-error %description -l en_US rufus -> Rufus, ruffs
rubygem-clockwork.noarch: W: spelling-error %description -l en_US resque -> risque, rescue, Esquire
rubygem-clockwork.noarch: W: no-manual-page-for-binary clockwork
rubygem-clockwork.src: W: spelling-error Summary(en_US) cron -> corn, con, crone
rubygem-clockwork.src: W: spelling-error %description -l en_US cron -> corn, con, crone
rubygem-clockwork.src: W: spelling-error %description -l en_US rufus -> Rufus, ruffs
rubygem-clockwork.src: W: spelling-error %description -l en_US resque -> risque, rescue, Esquire
3 packages and 0 specfiles checked; 0 errors, 9 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rubygem-clockwork rubygem-clockwork-doc
rubygem-clockwork.noarch: W: spelling-error Summary(en_US) cron -> corn, con, crone
rubygem-clockwork.noarch: W: spelling-error %description -l en_US cron -> corn, con, crone
rubygem-clockwork.noarch: W: spelling-error %description -l en_US rufus -> Rufus, ruffs
rubygem-clockwork.noarch: W: spelling-error %description -l en_US resque -> risque, rescue, Esquire
rubygem-clockwork.noarch: W: no-manual-page-for-binary clockwork
2 packages and 0 specfiles checked; 0 errors, 5 warnings.
# echo 'rpmlint-done:'



Requires
--------
rubygem-clockwork (rpmlib, GLIBC filtered):
    /usr/bin/env
    /usr/bin/ruby
    ruby(rubygems)
    rubygem(activesupport)
    rubygem(tzinfo)

rubygem-clockwork-doc (rpmlib, GLIBC filtered):
    rubygem-clockwork



Provides
--------
rubygem-clockwork:
    rubygem(clockwork)
    rubygem-clockwork

rubygem-clockwork-doc:
    rubygem-clockwork-doc



Source checksums
----------------
https://rubygems.org/gems/clockwork-0.7.7.gem :
  CHECKSUM(SHA256) this package     : 1d7686ce0b4e02a1fb60ad8663aa81e9f2b52fd2c298ebd15bbe37e2eb352f95
  CHECKSUM(SHA256) upstream package : 1d7686ce0b4e02a1fb60ad8663aa81e9f2b52fd2c298ebd15bbe37e2eb352f95
Using local file /home/valtri/fedora-scm/REVIEWS/rubygem-clockwork/clockworkd.1 as upstream
file:///home/valtri/fedora-scm/REVIEWS/rubygem-clockwork/clockworkd.1 :
  CHECKSUM(SHA256) this package     : 360c6f1039c1de59c3a8bb82d0918ae42dc2635089fb065febff8f11e3de4791
  CHECKSUM(SHA256) upstream package : 117470218f0bf06b35c2d5f5a39d69b3c3afb1fdfa2d61a52becdebb91ba831e
diff -r also reports differences


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 1121082
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Ruby, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

====

Remaining details can be resolved post-review.

Package approved! Good work.

Comment 11 Josef Stribny 2014-08-25 14:04:39 UTC
Thank you for the review.


New Package SCM Request
=======================
Package Name: rubygem-clockwork
Short Description: A scheduler process to replace cron
Upstream URL: http://github.com/tomykaira/clockwork
Owners: jstribny
Branches: f21
InitialCC:

Comment 12 Gwyn Ciesla 2014-08-26 12:11:32 UTC
Git done (by process-git-requests).


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