[Bug 1116028] New: Review Request: rubygem-elasticsearch-transport - Elasticsearch transport
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Bug ID: 1116028 Summary: Review Request: rubygem-elasticsearch-transport - Elasticsearch transport Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: steve.traylen@cern.ch QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic... SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic... Description: Transports for elasticsearch
Fedora Account System Username: stevetraylen
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1116030
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1116030 [Bug 1116030] Review Request: rubygem-elasticsearch - Ruby elasticsearch gem
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #1 from Eduardo Mayorga e@mayorgalinux.com --- I'd recommend you to use your real name in %changelog. "Fedora Cloud User" doesn't say much about who is packaging this.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #2 from Steve Traylen steve.traylen@cern.ch --- Sure, will fix name at next iteration.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #3 from Eduardo Mayorga e@mayorgalinux.com --- 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).
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #4 from Steve Traylen steve.traylen@cern.ch --- Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic... SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic...
all comments addressed, I also moved the tests to the doc.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Eduardo Mayorga e@mayorgalinux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |e@mayorgalinux.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #5 from Eduardo Mayorga e@mayorgalinux.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #6 from Eduardo Mayorga e@mayorgalinux.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #7 from Steve Traylen steve.traylen@cern.ch --- (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-elastic... SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #8 from Eduardo Mayorga e@mayorgalinux.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #9 from Steve Traylen steve.traylen@cern.ch --- Spec URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic... SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic...
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Eduardo Mayorga e@mayorgalinux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #10 from Eduardo Mayorga e@mayorgalinux.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #11 from Steve Traylen steve.traylen@cern.ch --- 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-elastic... SRPM URL: http://cern.ch/straylen/rpms/rubygem-elasticsearch-transport/rubygem-elastic...
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #12 from Eduardo Mayorga e@mayorgalinux.com --- The Spec URL is not updated and the SRPM URL does not work.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #13 from Steve Traylen steve.traylen@cern.ch --- Apologies - URLs now have something on the end of them.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Eduardo Mayorga e@mayorgalinux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #14 from Eduardo Mayorga e@mayorgalinux.com --- APPROVED!
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #15 from Steve Traylen steve.traylen@cern.ch --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #16 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #17 from Fedora Update System updates@fedoraproject.org --- 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....
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #18 from Fedora Update System updates@fedoraproject.org --- 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....
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #19 from Fedora Update System updates@fedoraproject.org --- 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....
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #20 from Fedora Update System updates@fedoraproject.org --- rubygem-elasticsearch-transport-1.0.7-1.el7 has been pushed to the Fedora EPEL 7 testing repository.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #21 from Steve Traylen steve.traylen@cern.ch --- EPEL 7 build blocked on lack of faraday
https://bugzilla.redhat.com/show_bug.cgi?id=1206119
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |rubygem-elasticsearch-trans | |port-1.0.7-1.fc22 Resolution|--- |ERRATA Last Closed| |2015-03-31 17:52:01
--- Comment #22 from Fedora Update System updates@fedoraproject.org --- rubygem-elasticsearch-transport-1.0.7-1.fc22 has been pushed to the Fedora 22 stable repository.
https://bugzilla.redhat.com/show_bug.cgi?id=1116028
--- Comment #23 from Fedora Update System updates@fedoraproject.org --- 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.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla