Bug 811286

Hi!

Here we go! Some *very preliminary* thoughts after just having skimmed through the spec files:

- Have you missed http://fedoraproject.org/wiki/Packaging:Ruby ? In particular, the empty %prep and %build together with how the "gem install" is performed in %install?

- The only difference between the two spec files is the ruby version. Such a case is better handled with something like  %if 0%{?fedora} > 16 ... in a common spec file.

- The long paths in the %install can be handled by doing a cd  %{buildroot}%{gemdir}/gems/%{gemname}-%{version} before doing the dirty chmod/rm work, improving readability.

- Likewise, you can use doc/images/*.png. 

- Please reorder the tags according to /etc/rpmdevtools/spectemplate-ruby.spec
in rpmdevtools (e. g., move BuildArch: above the Requires:).

- Try to use space in a consistent way e. g. two blank lines between main sections as %prep, %build, %install %check etc., and a single blank line as appropriate in other cases.

- The Group tag is not used by Fedora, and can be skipped. If you want to have it anyway (which is perfectly sane, it's required by the RPM spec) I guess System Environment/Libraries or Development/Libraries is a better choice(?)

- The empty %check can be removed

Run rpmlint on all packages + spec file and submit output together with the new spec versions.

[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ ls
build.log  rubygem-rhc-rest-0.0.7-2.fc16.noarch.rpm  rubygem-rhc-rest-doc-0.0.7-2.fc16.noarch.rpm
root.log   rubygem-rhc-rest-0.0.7-2.fc16.src.rpm     state.log
[12:05 PM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ rpmlint *.rpm
rubygem-rhc-rest.src:46: W: macro-in-comment %setup
3 packages and 0 specfiles checked; 0 errors, 1 warnings.
[12:05 PM]
t.spec rn@Doom:/var/lib/mock/fedora-16-x86_64/result $ rpmlint ~/rpmbuild/SPECS/rubygem-rhc-rest
/home/whearn/rpmbuild/SPECS/rubygem-rhc-rest.spec:46: W: macro-in-comment %setup
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
[12:05 PM]


SRPM: http://jknife.fedorapeople.org/F16/SRPMS/rubygem-rhc-rest-0.0.7-2.fc16.src.rpm
SPEC: http://jknife.fedorapeople.org/F16/SPECS/rubygem-rhc-rest.spec

Err, sorry wrong links:
SPEC: http://jknife.fedorapeople.org/review/rubygem-rhc-rest.spec
SRPM: http://jknife.fedorapeople.org/review/rubygem-rhc-rest-0.0.7-2.fc16.src.rpm

[12:29 PM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ ls
build.log  root.log  rubygem-rhc-rest-0.0.7-2.fc16.noarch.rpm  rubygem-rhc-rest-0.0.7-2.fc16.src.rpm  rubygem-rhc-rest-doc-0.0.7-2.fc16.noarch.rpm  state.log
[12:29 PM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ rpmlint rubygem-rhc*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
[12:29 PM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ rpmlint ~/rpmbuild/SPECS/rubygem-rhc-rest.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[12:29 PM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $

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

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

==== Generic ====
[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.
[-]: 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 Buildroot is not present
[x]: MUST Package contains no bundled libraries.
[!]: MUST Changelog in prescribed format.
     Changes not documented
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: 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.
[!]: MUST Package does not contain duplicates in %files.
     Note: warning: File listed twice: 
     /builddir/.gem/ruby/1.9.1/gems/rhc-rest-0.0.7/LICENSE
[x]: MUST Fully versioned dependency in subpackages, if present.
[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.
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: 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.
[!]: MUST License field in the package spec file matches the actual license.
     ./bin/sample-usage.rb is MIT
[x]: MUST License file installed when any subpackage combination is installed.
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     geminstdir defined but not used.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[!]: MUST Package must own all directories that it creates.
     Does not claim mandatory ruby directories
[!]: MUST Package does not own files or directories owned by other packages.
     Claims ownership of %{gemdir}
[!]: MUST Package installs properly.
     Installation fails on my not so clean F16 and a almost pristine F17
     virtual host.
[?]: MUST Requires correct, justified where necessary.
     Installation problems might be caused by bad requires.
[x]: MUST Rpmlint output is silent.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/mk/tmp/811286/rhc-rest-0.0.7.gem :
  MD5SUM this package     : b3c3f7588bb2540cb690018f86322846
  MD5SUM upstream package : b3c3f7588bb2540cb690018f86322846

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[-]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: 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.
[!]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires)
     Package requires itself(!)
$ rpm -q --provides -p  \
    ~/rpmbuild/RPMS/noarch/rubygem-rhc-rest-0.0.7-2.fc16.noarch.rpm
rubygem(rhc-rest) = 0.0.7
rubygem-rhc-rest = 0.0.7-2.fc16
$ rpm -q --requires -p \
    ~/rpmbuild/RPMS/noarch/rubygem-rhc-rest-0.0.7-2.fc16.noarch.rpm
/usr/bin/env
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
ruby(abi) = 1.8
rubygem(json)
rubygem(rest)
rubygems
rpmlib(PayloadIsXz) <= 5.2-1

[?]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[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.

Issues:
[!]: MUST Package does not contain duplicates in %files.
     Note: warning: File listed twice: /builddir/.gem/ruby/1.9.1/gems/rhc-
     rest-0.0.7/LICENSE. Use %exclude to fix.
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
[!]: MUST Package must own all directories that it creates.
     Does not claim mandatory directories:
        %{gemdir}/cache/%{gemname}-%{version}.gem
        %{gemdir}/specifications/%{gemname}-%{version}.gemspec
        %{gemdir}/gems/%{gemname}-%{version}/
     See http://fedoraproject.org/wiki/Packaging:Ruby
[!]: MUST Package does not own files or directories owned by other packages.
     Claims ownership of %{gemdir} which belongs to rubygems.
[!]: MUST Changelog in prescribed format.
     The format is actually OK, but you have not documented the changes.
     Each link you provide here should have s suitable changelog entry,
     and possibly also a release tag update. See:
     http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
[!]: MUST License field in the package spec file matches the actual license.
     ./bin/sample-usage.rb is MIT, creating a multiple license scenario. See
     http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
[!]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
     geminstdir defined but not used. I suggest that you just remove the
     definition.
[!]: MUST Package installs properly.
     Installation fails on my not so clean F16 and a almost pristine 
     F17 virtual host.
     --> Running transaction check
     ---> Package rubygem-rhc-rest.noarch 0:0.0.7-2.fc17 will be installed
     --> Processing Dependency: rubygem(rest) for package: rubygem-rhc-rest-0.0.7-2.fc17.noarch
     --> Finished Dependency Resolution
     Error: Package: rubygem-rhc-rest-0.0.7-2.fc17.noarch (/rubygem-rhc-rest-0.0.7-2.fc17.noarch)
	       Requires: rubygem(rest)
     You could try using --skip-broken to work around the problem

[!]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires)
     package requires itself(!)
	$ rpm -q --requires -p \
	    ~/rpmbuild/RPMS/noarch/rubygem-rhc-rest-0.0.7-2.fc16.noarch.rpm
	[cut]
	ruby(abi) = 1.8
	rubygem(json)
	---> rubygem(rest)
	rubygems
	rpmlib(PayloadIsXz) <= 5.2-1


Generated by fedora-review 0.1.3
External plugins:

Hm... the licensing is actually somewhat bad, to have more than one license in a gem makes life complicated for those using it. You might want to put the ./bin/sample-usage.rb in a separate sub-package (with a separate License: tag)  to handle this.

Well I think I got everything.

The failed install was a typo on my part :/ rubygem(rest) needed to be rubygem(rest-client). Though I have no clue why FedoraReview is saying it depended on itself. *shrugs* Anywho here is the updated SPEC/SRPM:

SPEC: http://jknife.fedorapeople.org/review/rubygem-rhc-rest.spec
SRPM: http://jknife.fedorapeople.org/review/rubygem-rhc-rest-0.0.7-3.fc16.src.rpm

Well, fedora-review is not the only one involved here;) I took 'rest' for
'rhc-rest', my bad, but the conclusion is the same.

- Suddenly %defattr and 'rm -rf %{buildroot}' is there. Please remove.

- rpmlint on installed package:

$ rpmlint rubygem-rest-client
rubygem-rest-client.noarch: W: spelling-error %description -l en_US
microframework -> micro framework, micro-framework, framework
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/log%3d-c.yaml %3d
rubygem-rest-client.noarch: W: wrong-file-end-of-line-encoding
/usr/lib/ruby/gems/1.8/gems/rest-client-1.6.1/spec/integration/certs/equifax.crt
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/Resource/%5b%5d-i.yaml
%5b
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/Resource/%5b%5d-i.yaml
%5d
rubygem-rest-client.noarch: W: wrong-file-end-of-line-encoding
/usr/lib/ruby/gems/1.8/gems/rest-client-1.6.1/spec/integration/certs/verisign.crt
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/%3c%3c-i.yaml %3c
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/%3c%3c-i.yaml %3c
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/Payload/has_file%3f-i.yaml
%3f
rubygem-rest-client.noarch: W: unexpanded-macro
/usr/lib/ruby/gems/1.8/doc/rest-client-1.6.1/ri/RestClient/AbstractResponse/return%21-i.yaml
%21
rubygem-rest-client.noarch: W: no-manual-page-for-binary restclient
1 packages and 0 specfiles checked; 0 errors, 11 warnings

- You need to take care of the eol warnings, the rest can be ignored (I think).
- For the spelling, what can a Swede say about that?

You ran rpmlint on the wrong package. That one is not mine :P

[12:30 AM]
[0]whearn@Doom:~ $ rpmlint rubygem-rhc-rest 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[12:30 AM]
[0]whearn@Doom:~ $ 

But here is the spec/srpm with the rm -rf %{buildroot} and %defattr removed.

[12:33 AM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ ls
build.log  rubygem-rhc-rest-0.0.7-4.fc16.noarch.rpm  rubygem-rhc-rest-doc-0.0.7-4.fc16.noarch.rpm
root.log   rubygem-rhc-rest-0.0.7-4.fc16.src.rpm     state.log
[12:33 AM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ rpmlint *.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
[12:34 AM]
[0]whearn@Doom:/var/lib/mock/fedora-16-x86_64/result $ 


SPEC: http://jknife.fedorapeople.org/review/rubygem-rhc-rest.spec
SRPM: http://jknife.fedorapeople.org/review/rubygem-rhc-rest-0.0.7-4.fc16.src.rpm

(In reply to comment #8)
> You ran rpmlint on the wrong package. That one is not mine :P
Oops, too late in the evening for me ;)

All issues resolved.

*** Approved

Wesley,

I am checking the spec file for F17 and above and it is not ideal. You should use rubygems-devel package which provide the gemdir, etc. Please follow the guildelines draft here [1].

Also, your .spec file states on the first line that it was generated by gem2rpm from bson gem ....


[1] http://fedoraproject.org/wiki/User:Toshio/RubyPackagingDraft#.25prep

Also, I'd love to see the test suite executed in %check section, if that is available.

> I am checking the spec file for F17 and above and it is not ideal. You should
> use rubygems-devel package which provide the gemdir, etc. Please follow the
> guildelines draft here [1].


Without questioning your judgement, the situation is still somewhat confusing: we have used the official ruby guidelines, and you tell us to use Toshio's draft instead. Whereas this might be perfectly reasonable, it requires some kind of motivation IMHO.

With that said, not using rubygems-devel seems plain wrong. Removing fedora-review flag.

Alec,

There was long discussion between FPC and Ruby-SIG about new guidelines required for Ruby 1.9.3. FPC approved the draft last week [1], so it is just matter of time when the official location will be updated.

[1] https://fedorahosted.org/fpc/ticket/134

Vit: OK, that's a motivation :) 

This review will continue, I appreciate if you look over my shoulder.

Wesley: The remarks in comment #10 needs to be handled...

Unfortunately,  I'm away and disconnected from the net until April 22 (roughly).  Sorry we couldn't close this in time, I was rather convinced we should be able to that. But I was wrong in that too.

--alec

Yeah this is now a no-go as it got merged with the rhc client tools. https://github.com/openshift/os-client-tools/tree/master/express