https://bugzilla.redhat.com/show_bug.cgi?id=839653
Bug ID: 839653 QA Contact: extras-qa@fedoraproject.org Severity: medium Version: rawhide Priority: medium CC: notting@redhat.com, package-review@lists.fedoraproject.org Assignee: nobody@fedoraproject.org Summary: Review Request: rubygem-slim - Slim is a template language Regression: --- Story Points: --- Classification: Fedora OS: Linux Reporter: mzatko@redhat.com Type: --- Documentation: --- Hardware: All Mount Type: --- Status: NEW Component: Package Review Product: Fedora
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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |tdawson@redhat.com
--- Comment #1 from Troy Dawson tdawson@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #2 from Troy Dawson tdawson@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #3 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |tdawson@redhat.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #4 from Troy Dawson tdawson@redhat.com --- - 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:
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vondruch@redhat.com
--- Comment #5 from Vít Ondruch vondruch@redhat.com --- I would appreciate if you can run Slim's test suite. Thank you.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #6 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #7 from Vít Ondruch vondruch@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #8 from Vít Ondruch vondruch@redhat.com --- Btw I am testing against F19/Rawhide and you should as well.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #9 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #10 from Vít Ondruch vondruch@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #11 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #12 from Troy Dawson tdawson@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #13 from Vít Ondruch vondruch@redhat.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #14 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #15 from Vít Ondruch vondruch@redhat.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #16 from Maros Zatko mzatko@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #17 from Vít Ondruch vondruch@redhat.com --- Thank you. I am fine with the .spec file.
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Troy Dawson tdawson@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? | Flags| |fedora-review+
--- Comment #18 from Troy Dawson tdawson@redhat.com --- All of my concerns were addressed as well. Thank you. I am marking this APPROVED
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #19 from Maros Zatko mzatko@redhat.com --- New Package SCM Request ======================= Package Name: rubygem-slim Short Description: Slim is a template language Owners: mzatko Branches: f17 f18 InitialCC:
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Maros Zatko mzatko@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #20 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #21 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #22 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839653
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #23 from Fedora Update System updates@fedoraproject.org --- rubygem-slim-1.2.2-8.fc18 has been pushed to the Fedora 18 testing repository.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839653
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |CURRENTRELEASE Last Closed| |2012-11-20 23:04:53
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #24 from Fedora Update System updates@fedoraproject.org --- rubygem-slim-1.2.2-8.fc17 has been pushed to the Fedora 17 stable repository.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839653
--- Comment #25 from Fedora Update System updates@fedoraproject.org --- rubygem-slim-1.2.2-8.fc18 has been pushed to the Fedora 18 stable repository.
package-review@lists.fedoraproject.org