Bug 1116028

I'd recommend you to use your real name in %changelog. "Fedora Cloud User" doesn't say much about who is packaging this.

Sure, will fix name at next iteration.

Some notes:

- Unnecessary macro: since there is no %check section, enable_tests is not in use in this package.

- Please consider moving these files into -doc
%{geminstdir}/Rakefile
%{geminstdir}/elasticsearch-transport.gemspec

and LICENCE.txt to main package marked as documentation (%doc).

Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport.spec
SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport-1.0.4-2.fc20.src.rpm

all comments addressed, I also moved the tests to the doc.

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

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


Issues:
=======
- It is not necessary to include %{gem_spec} both in the main package and in the doc subpackage, since the doc subpackage Requires the main one (and thus can only be present if the main one is). I would recommend leaving %{gem_spec} just in doc subpackage.

- There is no such file as %{gem_instdir}/.gitignore in buildroot, so you can get rid of
%files
...
%exclude %{gem_instdir}/.gitignore

- The timestamps of source tarball in the source rpm are not preserved. You need to download it using the -N argument for wget or -R for curl in your build environment (SOURCES directory).

- There seems to be a bug in the test suite. This is the output I get
[makerpm@localhost test]$ ruby test_helper.rb 
test_helper.rb:26:in `block in <main>': uninitialized constant Elasticsearch (NameError)
/usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- shoulda-context (LoadError)
	from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require'
	from test_helper.rb:30:in `<main>'

Please report this to upstream.


===== 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". 28 files have unknown license. Detailed output of
     licensecheck in /home/makerpm/reviews/1116028-rubygem-elasticsearch-
     transport/licensecheck.txt
[-]: 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: No rpmlint messages.
[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]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[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:
[-]: 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).
[x]: Package contains Requires: ruby(release).

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

Generic:
[x]: Avoid bundling fonts in non-fonts packages.
[-]: 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]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: 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.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[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]: Fully versioned dependency in subpackages if applicable.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Ruby:
[x]: Gem should use %gem_install macro.
[!]: Test suite of the library should be run.
[x]: Specfile should use macros from rubygem-devel package.
     Note: The specfile doesn't use these macros:
     /usr/share/gems/specifications/elasticsearch-transport-1.0.4.gemspec,
     %exclude /usr/share/gems/cache/elasticsearch-transport-1.0.4.gem,
     /usr/share/gems/gems/elasticsearch-transport-1.0.4/lib
[x]: Gem package should exclude cached Gem.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rubygem-elasticsearch-transport-1.0.4-2.fc22.noarch.rpm
          rubygem-elasticsearch-transport-doc-1.0.4-2.fc22.noarch.rpm
          rubygem-elasticsearch-transport-1.0.4-2.fc22.src.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rubygem-elasticsearch-transport rubygem-elasticsearch-transport-doc
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
rubygem-elasticsearch-transport (rpmlib, GLIBC filtered):
    ruby(rubygems)
    rubygem(faraday)
    rubygem(multi_json)

rubygem-elasticsearch-transport-doc (rpmlib, GLIBC filtered):
    rubygem-elasticsearch-transport



Provides
--------
rubygem-elasticsearch-transport:
    rubygem(elasticsearch-transport)
    rubygem-elasticsearch-transport

rubygem-elasticsearch-transport-doc:
    rubygem-elasticsearch-transport-doc



Source checksums
----------------
https://rubygems.org/gems/elasticsearch-transport-1.0.4.gem :
  CHECKSUM(SHA256) this package     : cb3fc26956061de4fd806c73b74021bafc3daeafc05575673690b3d5d3b6c3b4
  CHECKSUM(SHA256) upstream package : cb3fc26956061de4fd806c73b74021bafc3daeafc05575673690b3d5d3b6c3b4


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/bin/fedora-review -b 1116028 -m fedora-rawhide-x86_64
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

(In reply to Eduardo Mayorga from comment #5)
> - The timestamps of source tarball in the source rpm are not preserved. You
> need to download it using the -N argument for wget or -R for curl in your
> build environment (SOURCES directory).

Well, I meant "gem" here.

(In reply to Eduardo Mayorga from comment #5)

> 
> Issues:
> =======
> - It is not necessary to include %{gem_spec} both in the main package and in
> the doc subpackage, since the doc subpackage Requires the main one (and thus
> can only be present if the main one is). I would recommend leaving
> %{gem_spec} just in doc subpackage.
> 

I left the %{gem_spec} in the main package. It's needed for gem
to operate. Dropped from doc package.


> - There is no such file as %{gem_instdir}/.gitignore in buildroot, so you
> can get rid of
> %files
> ...
> %exclude %{gem_instdir}/.gitignore
> 

?

It is there, dropping the explicit %exclude.

Error: Installed (but unpackaged) file(s) found:
   /usr/share/gems/gems/elasticsearch-transport-1.0.4/.gitignore


> - The timestamps of source tarball in the source rpm are not preserved. You
> need to download it using the -N argument for wget or -R for curl in your
> build environment (SOURCES directory).
> 

ack, change made.

> - There seems to be a bug in the test suite. This is the output I get
> [makerpm@localhost test]$ ruby test_helper.rb 
> test_helper.rb:26:in `block in <main>': uninitialized constant Elasticsearch
> (NameError)
> /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require':
> cannot load such file -- shoulda-context (LoadError)
> 	from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require'
> 	from test_helper.rb:30:in `<main>'
> 

So the this is just saying the gem 'rubygem-shoulda-context' is not installed.
I could add it to the requirements of the -doc subpackage but once installed
you only run into the next one that rubygem-turn is not installed and 
this is not available.

I guess this comes down to should the tests be included at all if they 
don't work... Given they have been marked as doc anyway they don't have to
work I would say, they are still present as a reference.

I will look at packaging rubygem-turn though.

Looking at the ruby guidelines it's unclear as to if test suites
should be included, only that they should be run in possible which is 
currently not the case.


Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport.spec
SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport-1.0.4-2.fc20.src.rpm

In addition I will once this package is okay look at updating the other elasticsearch reviews. I expect all of your comments may well apply
there also.

(In reply to Steve Traylen from comment #7)
> (In reply to Eduardo Mayorga from comment #5)
> 
> > 
> > Issues:
> > =======
> > - It is not necessary to include %{gem_spec} both in the main package and in
> > the doc subpackage, since the doc subpackage Requires the main one (and thus
> > can only be present if the main one is). I would recommend leaving
> > %{gem_spec} just in doc subpackage.
> > 
> 
> I left the %{gem_spec} in the main package. It's needed for gem
> to operate. Dropped from doc package.

OK.

> > - There is no such file as %{gem_instdir}/.gitignore in buildroot, so you
> > can get rid of
> > %files
> > ...
> > %exclude %{gem_instdir}/.gitignore
> > 
> 
> ?
> 
> It is there, dropping the explicit %exclude.
> 
> Error: Installed (but unpackaged) file(s) found:
>    /usr/share/gems/gems/elasticsearch-transport-1.0.4/.gitignore

You're right. This was an overlook on my side.

It's OK.

> > - The timestamps of source tarball in the source rpm are not preserved. You
> > need to download it using the -N argument for wget or -R for curl in your
> > build environment (SOURCES directory).
> > 
> 
> ack, change made.

OK.

> > - There seems to be a bug in the test suite. This is the output I get
> > [makerpm@localhost test]$ ruby test_helper.rb 
> > test_helper.rb:26:in `block in <main>': uninitialized constant Elasticsearch
> > (NameError)
> > /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require':
> > cannot load such file -- shoulda-context (LoadError)
> > 	from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require'
> > 	from test_helper.rb:30:in `<main>'
> > 
> 
> So the this is just saying the gem 'rubygem-shoulda-context' is not
> installed.
> I could add it to the requirements of the -doc subpackage but once installed
> you only run into the next one that rubygem-turn is not installed and 
> this is not available.

It seems that's not the problem, because I'm unable to run the test even having that gem installed.

> I guess this comes down to should the tests be included at all if they 
> don't work... Given they have been marked as doc anyway they don't have to
> work I would say, they are still present as a reference.
> 
> I will look at packaging rubygem-turn though.
> 
> Looking at the ruby guidelines it's unclear as to if test suites
> should be included, only that they should be run in possible which is 
> currently not the case.

Yeah, it's ok to leave it as it is for now.

> Spec URL:
> http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-
> elasticsearch-transport.spec
> SRPM URL:
> http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-
> elasticsearch-transport-1.0.4-2.fc20.src.rpm

Please update the SRPM URL.

> In addition I will once this package is okay look at updating the other
> elasticsearch reviews. I expect all of your comments may well apply
> there also.

Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport.spec
SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport-1.0.4-3.fc20.src.rpm

Just one more thing: as I see from this
> %if 0%{?fc19} || 0%{?fc20} || 0%{?el6} || 0%{?el7}

you're going for EPEL6 too. In that case, you need to add conditionalized: 
Requires: ruby(abi) = 1.8
This is only needed for EPEL6 and earlier.
See: http://fedoraproject.org/wiki/Packaging:Old_Ruby

Otherwise, it looks good.

I'm skipping .el6, the ruby version itself is to old, I had removed it from 
the other conditional but have removed from this one also.

Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport.spec
SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elasticsearch-transport-1.0.4-4.fc20.src.rpm

The Spec URL is not updated and the SRPM URL does not work.

Apologies - URLs now have something on the end of them.

APPROVED!

New Package SCM Request
=======================
Package Name: rubygem-elasticsearch-transport
Short Description: Elasticsearch transport
Upstream URL: https://github.com/elasticsearch/elasticsearch-ruby
Owners: stevetraylen
Branches: f20 f21 epel7
InitialCC:


Eduardo, many thanks for the review.

Git done (by process-git-requests).

rubygem-elasticsearch-transport-1.0.7-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/rubygem-elasticsearch-transport-1.0.7-1.el7

rubygem-elasticsearch-transport-1.0.7-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/rubygem-elasticsearch-transport-1.0.7-1.fc21

rubygem-elasticsearch-transport-1.0.7-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/rubygem-elasticsearch-transport-1.0.7-1.fc22

rubygem-elasticsearch-transport-1.0.7-1.el7 has been pushed to the Fedora EPEL 7 testing repository.

EPEL 7 build blocked on lack of faraday 

https://bugzilla.redhat.com/show_bug.cgi?id=1206119

rubygem-elasticsearch-transport-1.0.7-1.fc22 has been pushed to the Fedora 22 stable repository.

rubygem-elasticsearch-transport-1.0.7-1.fc21 has been pushed to the Fedora 21 stable repository. If problems still persist, please make note of it in this bug report.