Bug 839653 - Review Request: rubygem-slim - Slim is a template language
Review Request: rubygem-slim - Slim is a template language
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Troy Dawson
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-12 09:46 EDT by Maros Zatko
Modified: 2013-01-23 11:06 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-20 23:04:53 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tdawson: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Maros Zatko 2012-07-12 09:46:55 EDT
Spec URL: http://v3.sk/~hexo/rpm/slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-1.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 1 Troy Dawson 2012-07-13 17:17:31 EDT
Informal Review:
The spec file should match the rpm name (rubygem-slim.spec)
The License is MIT

You should attach an rpmlint to this review request

$ rpmlint slim.spec rubygem-slim-1.2.2-1.fc17.src.rpm
rubygem-slim.src: E: invalid-spec-name
1 packages and 1 specfiles checked; 1 errors, 0 warnings.

Other than that, this looks very good.
Comment 2 Troy Dawson 2012-07-18 11:41:34 EDT
Informal Review (continued):

I am unable to get this to build in mock or by hand.  It fails at the end of rpm building with 

RPM build errors:
    File not found: /builddir/build/BUILDROOT/rubygem-slim-1.2.2-1.fc17.x86_64/usr/share/gems/gems/slim-1.2.2/.yardoc/checksums
    File not found: /builddir/build/BUILDROOT/rubygem-slim-1.2.2-1.fc17.x86_64/usr/share/gems/gems/slim-1.2.2/.yardoc/objects/root.dat
    File not found: /builddir/build/BUILDROOT/rubygem-slim-1.2.2-1.fc17.x86_64/usr/share/gems/gems/slim-1.2.2/.yardoc/proxy_types

Please fix this.
Comment 3 Maros Zatko 2012-09-03 11:37:54 EDT
Thank you for review, there is a updated spec & srpm

Spec URL: http://v3.sk/~hexo/rpm/slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-2.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 4 Troy Dawson 2012-09-11 17:28:18 EDT
- LICENSE must be a doc
   replace %{gem_instdir}/LICENSE  with %doc %{gem_instdir}/LICENSE
- spec file must be in the format %{name}.spec
   slim.spec should be rubygem-slim.spec
- 3 non-executable scripts
   /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-render.rb
   /usr/share/gems/gems/slim-1.2.2/benchmarks/run-benchmarks.rb
   /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-parser.rb
- There is a test suite and it should be ran, at least what can be run.
   https://fedoraproject.org/wiki/Packaging/Ruby#Testing_frameworks_usage


Package Review
==============

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



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[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.
[X]: MUST Package contains no bundled libraries.
[X]: MUST Changelog in prescribed format.
[X]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[X]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[-]: MUST Development files must be in a -devel package
[-]: 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 Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[X]: MUST Large documentation files are in a -doc subpackage, if required.
[!]: 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.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
[X]: MUST License file installed when any subpackage combination is installed.
[X]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[X]: MUST Package is named according to the Package Naming Guidelines.
[X]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[X]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[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 Package is not relocatable.
[X]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[X]: MUST Spec file is legible and written in American English.
[!]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
     Note: slim.spec should be rubygem-slim.spec
[-]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL5 is required
[-]: 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.
[!]: SHOULD Latest version is packaged.
[X]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX tarball generation or download is documented.
[X]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: It is correct according to the ruby gem guidelines
[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.
[X]: 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.
[x]: SHOULD Spec use %global instead of %define.


==== Language ====
[x]: MUST Gem package must not define a non-gem subpackage
[x]: MUST Gem package must exclude cached Gem.
[X]: MUST Platform dependent files must all go under %{gem_extdir}, platform
     independent under %{gem_dir}.
[x]: MUST Gem package is named rubygem-%{gem_name}
[x]: MUST Package contains BuildRequires: rubygems-devel.
[x]: MUST Gem package must define %{gem_name} macro.
[x]: MUST Pure Ruby package must be built as noarch
[x]: MUST Package contains Requires: ruby(abi).
[x]: SHOULD Specfile should utilize macros from rubygem-devel package.
[x]: SHOULD Test suite should not be run by rake.
[!]: SHOULD Test suite of the library should be run.

Issues:
[!]: 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.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
[!]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
     Note: slim.spec should be rubygem-slim.spec
See: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Spec_file_name

Rpmlint
-------
Checking: rubygem-slim-1.2.2-2.fc18.noarch.rpm
          rubygem-slim-doc-1.2.2-2.fc18.noarch.rpm
          rubygem-slim-1.2.2-2.fc18.src.rpm
rubygem-slim.noarch: W: no-documentation
rubygem-slim.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-render.rb 0644L /usr/bin/env
rubygem-slim.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/run-benchmarks.rb 0644L /usr/bin/env
rubygem-slim.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-parser.rb 0644L /usr/bin/env
rubygem-slim.noarch: W: no-manual-page-for-binary slimrb
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/%5b%5d-i.ri %5b
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/%5b%5d-i.ri %5d
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/empty%3f-i.ri %3f
rubygem-slim.src: E: invalid-spec-name
3 packages and 0 specfiles checked; 4 errors, 5 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint rubygem-slim-doc
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/%5b%5d-i.ri %5b
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/%5b%5d-i.ri %5d
rubygem-slim-doc.noarch: W: unexpanded-macro /usr/share/gems/doc/slim-1.2.2/ri/Slim/Wrapper/empty%3f-i.ri %3f
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
# echo 'rpmlint-done:'

Requires
--------
rubygem-slim-1.2.2-2.fc18.noarch.rpm (rpmlib, GLIBC filtered):
    
    /usr/bin/env
    /usr/bin/ruby
    ruby
    ruby(abi) = 1.9.1
    ruby(rubygems)
    rubygem(temple) < 0.5
    rubygem(temple) >= 0.4.0
    rubygem(tilt) < 1.4
    rubygem(tilt) >= 1.3.3

rubygem-slim-doc-1.2.2-2.fc18.noarch.rpm (rpmlib, GLIBC filtered):
    
    rubygem-slim = 1.2.2-2.fc18

Provides
--------
rubygem-slim-1.2.2-2.fc18.noarch.rpm:
    
    rubygem(slim) = 1.2.2
    rubygem-slim = 1.2.2-2.fc18

rubygem-slim-doc-1.2.2-2.fc18.noarch.rpm:
    
    rubygem-slim-doc = 1.2.2-2.fc18

MD5-sum check
-------------
http://rubygems.org/gems/slim-1.2.2.gem :
  CHECKSUM(SHA256) this package     : 50103264688314dc1bfcf3a3f42dfc2504d31ee2391acde34bc7ae9542eeecca
  CHECKSUM(SHA256) upstream package : 50103264688314dc1bfcf3a3f42dfc2504d31ee2391acde34bc7ae9542eeecca


Generated by fedora-review 0.2.2 (9f8c0e5) last change: 2012-08-09
Command line :/bin/fedora-review -b 839653
External plugins:
Comment 5 Vít Ondruch 2012-09-18 01:36:00 EDT
I would appreciate if you can run Slim's test suite. Thank you.
Comment 6 Maros Zatko 2012-10-09 10:44:05 EDT
Thank you for review, there is a updated spec & srpm.
I don't run test suite because of missing dependencies.

Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-3.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 7 Vít Ondruch 2012-10-10 04:39:22 EDT
(In reply to comment #6)
> I don't run test suite because of missing dependencies.

I see just 3 test failures because of
1) missing creole
2) missing wikicloth
3) seems like bug in either test or slim itself, you should investigate.

Please enable the test suite and run it using:

testrb -Ilib test/rails/
testrb -Ilib test/slim/ | grep "216 tests, 234 assertions, 1 failures, 2 errors, 0 skips"

Thank you.
Comment 8 Vít Ondruch 2012-10-10 04:42:29 EDT
Btw I am testing against F19/Rawhide and you should as well.
Comment 9 Maros Zatko 2012-10-10 11:17:27 EDT
Now runs tests. Rebuilt against F17 and Rawhide in mock.

Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-4.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 10 Vít Ondruch 2012-10-11 01:32:17 EDT
Thank you Maros. However, did you have a chance to investigate the Markdown error and report it upstream? Would be nice to know what is the cause of the error. That is the purpose of the test suite.

BTW You have wrong your files section:

%{gem_instdir}/test/rails/*
%{gem_instdir}/test/slim/*
%{gem_instdir}/benchmarks/*

This means that you own only the files inside directories, not the directories itself [1]. You should do

%{gem_instdir}/test/
%{gem_instdir}/benchmarks/

instead. Then you own the directories and their content.



[1] http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics
Comment 11 Maros Zatko 2012-10-16 12:23:03 EDT
fixed markdown test, owning my directories, is there something more to fix?

Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-6.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 12 Troy Dawson 2012-10-16 15:46:04 EDT
Very close.  I am only getting one problem, it's in the rpmlint section.

Rpmlint
-------
Checking: rubygem-slim-doc-1.2.2-6.fc19.noarch.rpm
          rubygem-slim-1.2.2-6.fc19.noarch.rpm
          rubygem-slim-1.2.2-6.fc19.src.rpm
rubygem-slim-doc.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-render.rb 0644L /usr/bin/env
rubygem-slim-doc.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/run-benchmarks.rb 0644L /usr/bin/env
rubygem-slim-doc.noarch: E: non-executable-script /usr/share/gems/gems/slim-1.2.2/benchmarks/profile-parser.rb 0644L /usr/bin/env
rubygem-slim.noarch: W: no-manual-page-for-binary slimrb
rubygem-slim.src:63: W: macro-in-comment %{SOURCE0}
3 packages and 0 specfiles checked; 3 errors, 2 warnings.

It looks like if you add

grep -rl /usr/bin/env %{buildroot}%{gem_instdir}/benchmarks  | xargs chmod a+x

that should fix it up.  And if Vit has no objections, I'll approve it.

Getting rid of that #%{SOURCE0} would make things even better, but it's not so important that I won't approve it.
Comment 13 Vít Ondruch 2012-10-17 06:19:42 EDT
(In reply to comment #11)
* You still do not own the %{gem_instdir}/test/ directory

* You fixed one failure but there are still other two errors + You should reference issue sending the patch upstream or upstream error report alternatively.

* Place the content into %prep and %build sections according to guidelines [1]



[1] https://fedoraproject.org/wiki/Packaging:Ruby#Building_gems
Comment 14 Maros Zatko 2012-10-17 12:44:07 EDT
It took me a while to notice I missed the %build section in sample from guidelines. Is it good enough, now?

Wrt noticing upstream - I'll investigate if it's worth to send that patch.

Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-7.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 15 Vít Ondruch 2012-10-18 01:13:50 EDT
(In reply to comment #14)
> It took me a while to notice I missed the %build section in sample from
> guidelines. Is it good enough, now?

Good. Thank you.

> Wrt noticing upstream - I'll investigate if it's worth to send that patch.

Thank you.

> Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
> SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-7.fc17.src.rpm
> Description: Slim is a template language whose goal is reduce the syntax to
> the essential parts without becoming cryptic.
> Fedora Account System Username: mzatko

Just to minor non-blcoking nits:

* The %check section should be traditionally placed after %install section

* Please exclude the %{gem_cache}, i.e. prefix it with %exclude macro
Comment 16 Maros Zatko 2012-10-19 09:04:16 EDT
Excluded gem cache + check now goes after install.

Thanks for your patience.

Spec URL: http://v3.sk/~hexo/rpm/rubygem-slim.spec
SRPM URL: http://v3.sk/~hexo/rpm/rubygem-slim-1.2.2-8.fc17.src.rpm
Description: Slim is a template language whose goal is reduce the syntax to the essential parts without becoming cryptic.
Fedora Account System Username: mzatko
Comment 17 Vít Ondruch 2012-10-19 09:30:46 EDT
Thank you. I am fine with the .spec file.
Comment 18 Troy Dawson 2012-10-19 09:53:43 EDT
All of my concerns were addressed as well.  Thank you.  I am marking this
APPROVED
Comment 19 Maros Zatko 2012-10-22 10:43:48 EDT
New Package SCM Request
=======================
Package Name: rubygem-slim
Short Description: Slim is a template language
Owners: mzatko
Branches: f17 f18
InitialCC:
Comment 20 Gwyn Ciesla 2012-10-24 06:59:05 EDT
Git done (by process-git-requests).
Comment 21 Fedora Update System 2012-11-08 05:36:08 EST
rubygem-slim-1.2.2-8.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-slim-1.2.2-8.fc18
Comment 22 Fedora Update System 2012-11-08 05:36:20 EST
rubygem-slim-1.2.2-8.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-slim-1.2.2-8.fc17
Comment 23 Fedora Update System 2012-11-08 22:20:11 EST
rubygem-slim-1.2.2-8.fc18 has been pushed to the Fedora 18 testing repository.
Comment 24 Fedora Update System 2012-11-20 23:04:55 EST
rubygem-slim-1.2.2-8.fc17 has been pushed to the Fedora 17 stable repository.
Comment 25 Fedora Update System 2013-01-23 11:06:55 EST
rubygem-slim-1.2.2-8.fc18 has been pushed to the Fedora 18 stable repository.

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