Fedora Account System
Red Hat Associate
Red Hat Customer
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-1.fc20.src.rpm Description: Transports for elasticsearch Fedora Account System Username: stevetraylen
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.