https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Bug ID: 1372718 Summary: Review Request: rubygem-puma - A simple, fast, threaded, and highly concurrent HTTP 1.1 server Product: Fedora Version: rawhide Component: Package Review Assignee: nobody@fedoraproject.org Reporter: jaruga@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://people.redhat.com/jaruga/git/rubygem-puma/rubygem-puma.spec SRPM URL: http://people.redhat.com/jaruga/git/rubygem-puma/rubygem-puma-3.6.0-1.fc26.s... Description: Puma is a simple, fast, threaded, and highly concurrent HTTP 1.1 server Fedora Account System Username: jaruga
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=15470988 Other files (Patch files, puma-3.6.0-tests.tgz): http://people.redhat.com/jaruga/git/rubygem-puma/
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Pavel Valena pvalena@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |pvalena@redhat.com Assignee|nobody@fedoraproject.org |pvalena@redhat.com
--- Comment #1 from Pavel Valena pvalena@redhat.com --- Hi,
I will do the review.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Pavel Valena pvalena@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Pavel Valena pvalena@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #2 from Pavel Valena pvalena@redhat.com --- Package Review ==============
Issues ====== *** spec file * Is it necessary to unpack+rebuild the gem? ``` gem unpack %{SOURCE0} . . . gem build %{gem_name}.gemspec ``` instead of ``` %setup -q -c -T %gem_install -n %{SOURCE0} ```
* %description contains usage recommendations, which should be better included in README.Fedora file[1] instead of %description.
* %files include ``` %{gem_instdir}/DEPLOYMENT.md %{gem_instdir}/Manifest.txt ``` which should be IMO in doc subpackages.
*** build.log errors * In %build section there is ``` To see why this extension failed to compile, please check the mkmf.log which can be found here: /builddir/build/BUILD/puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0/mkmf.log ``` But the .so is packaged. Is this false positive?
* In %install section: ``` cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/http11_parser.c: Cannot stat: No such file or directory cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/http11_parser.rl: Cannot stat: No such file or directory ``` Is this also false positive?
References ========== [1] https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#...
Result ====== * I have tested the package with a sample app. Works as expected. * Above are only recommendations. * There are no blockers, therefore I APPROVE this package.
Legend ====== [x] = Pass, [!] = Fail, [-] = Not applicable
===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
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. [x]: License file installed when any subpackage combination is installed. [x]: Package must own all directories that it creates. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Sources contain only permissible code or content. [x]: 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]: Useful -debuginfo package or justification otherwise. [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. [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 %license. [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]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [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 does not use a name that already exists. [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. [x]: Packages must not store files under /srv, /opt or /usr/local
Rpmlint ------- Checking: rubygem-puma-3.6.0-1.fc26.x86_64.rpm rubygem-puma-doc-3.6.0-1.fc26.noarch.rpm rubygem-puma-debuginfo-3.6.0-1.fc26.x86_64.rpm rubygem-puma-3.6.0-1.fc26.src.rpm rubygem-puma.x86_64: W: no-documentation rubygem-puma.x86_64: E: zero-length /usr/lib64/gems/ruby/puma-3.6.0/gem.build_complete rubygem-puma.x86_64: W: no-manual-page-for-binary puma rubygem-puma.x86_64: W: no-manual-page-for-binary pumactl rubygem-puma.src: W: patch-not-applied Patch0: rubygem-puma-3.6.0-enable-log-for-tests.patch rubygem-puma.src: W: patch-not-applied Patch1: rubygem-puma-3.6.0-update-testhelp-path.patch rubygem-puma.src: W: invalid-url Source1: puma-3.6.0-tests.tgz 4 packages and 0 specfiles checked; 1 errors, 6 warnings.
Rpmlint (debuginfo) ------------------- Checking: rubygem-puma-debuginfo-3.6.0-1.fc26.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- rubygem-puma.x86_64: W: no-documentation rubygem-puma.x86_64: E: zero-length /usr/lib64/gems/ruby/puma-3.6.0/gem.build_complete rubygem-puma.x86_64: W: no-manual-page-for-binary puma rubygem-puma.x86_64: W: no-manual-page-for-binary pumactl 3 packages and 0 specfiles checked; 1 errors, 3 warnings.
Source checksums ---------------- https://rubygems.org/gems/puma-3.6.0.gem : CHECKSUM(SHA256) this package : 298d7b9122fd9c4aa78bf01d8a635122083963e53262975c0052862c87b7ace6 CHECKSUM(SHA256) upstream package : 298d7b9122fd9c4aa78bf01d8a635122083963e53262975c0052862c87b7ace6
Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #3 from Jun Aruga jaruga@redhat.com --- Thank you for your review.
(In reply to Pavel Valena from comment #2)
- Is it necessary to unpack+rebuild the gem?
gem unpack %{SOURCE0} . . . gem build %{gem_name}.gemspec
instead of
%setup -q -c -T %gem_install -n %{SOURCE0}
The "unpack+rebuild" is based on the template of gem2rpm. But I will change its logic to your suggested simpler one.
- %description contains usage recommendations, which should be better
included in README.Fedora file[1] instead of %description.
Ok, I will update %description to be shorten. And add README.Fedora to %files doc section.
- %files include
%{gem_instdir}/DEPLOYMENT.md %{gem_instdir}/Manifest.txt
which should be IMO in doc subpackages.
Yes, you are correct. It was my mistake. I will move those to the doc subpackage.
*** build.log errors
- In %build section there is
To see why this extension failed to compile, please check the mkmf.log which can be found here: /builddir/build/BUILD/puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0/mkmf.log
But the .so is packaged. Is this false positive?
Let me check it.
- In %install section:
cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/ http11_parser.c: Cannot stat: No such file or directory cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/ http11_parser.rl: Cannot stat: No such file or directory
Is this also false positive?
Let me check it.
References
[1] https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/ Guidelines#Summary_and_description
Result
- I have tested the package with a sample app. Works as expected.
- Above are only recommendations.
- There are no blockers, therefore I APPROVE this package.
Ok, thanks!
Legend
[x] = Pass, [!] = Fail, [-] = Not applicable
===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
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. [x]: License file installed when any subpackage combination is installed. [x]: Package must own all directories that it creates. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Sources contain only permissible code or content. [x]: 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]: Useful -debuginfo package or justification otherwise. [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. [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 %license. [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]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [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 does not use a name that already exists. [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. [x]: Packages must not store files under /srv, /opt or /usr/local
Rpmlint
Checking: rubygem-puma-3.6.0-1.fc26.x86_64.rpm rubygem-puma-doc-3.6.0-1.fc26.noarch.rpm rubygem-puma-debuginfo-3.6.0-1.fc26.x86_64.rpm rubygem-puma-3.6.0-1.fc26.src.rpm rubygem-puma.x86_64: W: no-documentation rubygem-puma.x86_64: E: zero-length /usr/lib64/gems/ruby/puma-3.6.0/gem.build_complete rubygem-puma.x86_64: W: no-manual-page-for-binary puma rubygem-puma.x86_64: W: no-manual-page-for-binary pumactl rubygem-puma.src: W: patch-not-applied Patch0: rubygem-puma-3.6.0-enable-log-for-tests.patch rubygem-puma.src: W: patch-not-applied Patch1: rubygem-puma-3.6.0-update-testhelp-path.patch rubygem-puma.src: W: invalid-url Source1: puma-3.6.0-tests.tgz 4 packages and 0 specfiles checked; 1 errors, 6 warnings.
Rpmlint (debuginfo)
Checking: rubygem-puma-debuginfo-3.6.0-1.fc26.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages)
rubygem-puma.x86_64: W: no-documentation rubygem-puma.x86_64: E: zero-length /usr/lib64/gems/ruby/puma-3.6.0/gem.build_complete rubygem-puma.x86_64: W: no-manual-page-for-binary puma rubygem-puma.x86_64: W: no-manual-page-for-binary pumactl 3 packages and 0 specfiles checked; 1 errors, 3 warnings.
Source checksums
https://rubygems.org/gems/puma-3.6.0.gem : CHECKSUM(SHA256) this package : 298d7b9122fd9c4aa78bf01d8a635122083963e53262975c0052862c87b7ace6 CHECKSUM(SHA256) upstream package : 298d7b9122fd9c4aa78bf01d8a635122083963e53262975c0052862c87b7ace6
Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #4 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-puma
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #5 from Jun Aruga jaruga@redhat.com --- We may need to add openssl-develop as a package dependency to use below libraries.
- /usr/lib64/libcrypto.so - /usr/lib64/libssl.so
I am asking now. https://github.com/puma/puma/issues/1074
I updated files for your review items.
Spec URL: http://people.redhat.com/jaruga/git/rubygem-puma/rubygem-puma.spec SRPM URL: http://people.redhat.com/jaruga/git/rubygem-puma/rubygem-puma-3.6.0-1.fc26.s...
http://people.redhat.com/jaruga/git/rubygem-puma/README.Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vondruch@redhat.com
--- Comment #6 from Vít Ondruch vondruch@redhat.com --- Jun, first of all, could you please bump the release between the review iterations? Coming late to the review, it is impossible to see what did you actually changed in the .spec file.
* Mark %{gem_instdir}/Manifest.txt as a %doc - %{gem_instdir}/Manifest.txt should be %doc, shouldn't be?
* Move %{gem_instdir}/tools into -doc - What is the content of %{gem_instdir}/tools directory? It does not look essential to me, hence I'd suggest to move it into -doc subpackage.
* Remove unnecessary seds - Please remove the seds, where you are replacing the BUILD_EXT_DIR and use following command to execute the test suite instead:
``` RUBYOPT=-I.:lib:$(dirs +1)%{gem_extdir_mri} ruby \ -rminitest/autorun \ -e 'Dir.glob "./test/**/test_*.rb", &method(:require)' \ -- -v ```
and
``` RUBYOPT=-I$(dirs +2)%{gem_extdir_mri} sh -x run.sh ```
* Purpose of Patch1? - Not sure about purpose of the Patch1, since if I comment it out and use the previous commands, I can execute the test suite just fine => I suggest to drop this patch altogether. - You are right that upstream is inconsistent in this regard, but I'd say that the particular require you are fixing is the way how the test suites typically works, while the proposed fix is antipattern. IOW you should propose fix fixing all the other files.
* Missing dependency on openssl-devel - Yes, you have noticed it correctly, that you need openssl-devel to enable HTTPS support, which we definitely want on Fedora.
* Shebangs in %{gem_instdir}/bin - I'd suggest against changing the shebangs. Yes, rpmlint complains about them, but these are not executed by enduser typically, so I consider them false positive and the rpmlint check too strict.
* Regenerate the parser using Ragel - The extension contains parser in C, generated using Ragel. Since current guidelines suggest to regenerate the files [1], it'd be nice if you try to regenerate them (see the Rakefile for details).
[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Use_of_pregenerated_code
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #7 from Jun Aruga jaruga@redhat.com --- Vit thanks for the view. Today I will bump the release at first.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #8 from Jun Aruga jaruga@redhat.com --- I bumped the release.
rubygem-puma-3.6.0-1.fc25 rubygem-puma-3.6.0-1.fc26
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #9 from Vít Ondruch vondruch@redhat.com --- Hi Jun,
I am not sure we understand each other. When speaking about release bump, you should do something like:
``` $ rpmdev-bumpspec rubygem-puma.spec -c "Latest change description" ```
This increases the release tag (in you case from 1 to 2) and adds new changelog entry. Increasing release without making any changes is pointless.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #10 from Jun Aruga jaruga@redhat.com --- Sorry, my mistake. I did not understand it.
Last night I just pushed and pushed the filesafter Pavel's reviewing to git repo: ssh://jaruga@pkgs.fedoraproject.org/rpms/rubygem-puma before I am going to do your mentioned items.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #11 from Jun Aruga jaruga@redhat.com --- (In reply to Vít Ondruch from comment #6)
Jun, first of all, could you please bump the release between the review iterations? Coming late to the review, it is impossible to see what did you actually changed in the .spec file.
- Mark %{gem_instdir}/Manifest.txt as a %doc
- %{gem_instdir}/Manifest.txt should be %doc, shouldn't be?
Yes, I would move to %doc.
- Move %{gem_instdir}/tools into -doc
- What is the content of %{gem_instdir}/tools directory? It does not look essential to me, hence I'd suggest to move it into -doc subpackage.
The tools includes one test and init scripts for initd and upstart. Yes, I would move to -doc subpackage.
- Remove unnecessary seds
- Please remove the seds, where you are replacing the BUILD_EXT_DIR and use following command to execute the test suite instead:
RUBYOPT=-I.:lib:$(dirs +1)%{gem_extdir_mri} ruby \ -rminitest/autorun \ -e 'Dir.glob "./test/**/test_*.rb", &method(:require)' \ -- -v
and
RUBYOPT=-I$(dirs +2)%{gem_extdir_mri} sh -x run.sh
Yes, I would change like your way.
- Purpose of Patch1?
- Not sure about purpose of the Patch1, since if I comment it out and use the previous commands, I can execute the test suite just fine => I
suggest to drop this patch altogether.
- You are right that upstream is inconsistent in this regard, but I'd say that the particular require you are fixing is the way how the test suites typically works, while the proposed fix is antipattern. IOW you should propose fix fixing all the other files.
I thinks the Patch1 is needed. Because I got an error without the Patch1. It depends on the order of test.
<mock-chroot> sh-4.3# RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0 \
ruby -r minitest/autorun -e 'Dir.glob "./test/test_http11.rb", &method(:require)' -- -v
/usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- testhelp (LoadError) from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from /builddir/build/BUILD/rubygem-puma-3.6.0/usr/share/gems/gems/puma-3.6.0/test/test_http11.rb:4:in `<top (required)>' from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from -e:1:in `glob' from -e:1:in `<main>'
When I add "-r test/testhelp", its test was passed.
<mock-chroot> sh-4.3# RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0 \
ruby -r minitest/autorun -r test/testhelp -e 'Dir.glob "./test/test_http11.rb", &method(:require)' -- -v
... 100% passed
- Missing dependency on openssl-devel
- Yes, you have noticed it correctly, that you need openssl-devel to enable HTTPS support, which we definitely want on Fedora.
Yes, I would use openssl-devel.
- Shebangs in %{gem_instdir}/bin
- I'd suggest against changing the shebangs. Yes, rpmlint complains about them, but these are not executed by enduser typically, so I consider them false positive and the rpmlint check too strict.
OK. remove its logic to change the shebangs.
- Regenerate the parser using Ragel
- The extension contains parser in C, generated using Ragel. Since current guidelines suggest to regenerate the files [1], it'd be nice if you try
to regenerate them (see the Rakefile for details).
OK, I would do it.
[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Use_of_pregenerated_code
Thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #12 from Jun Aruga jaruga@redhat.com --- Created attachment 1202550 --> https://bugzilla.redhat.com/attachment.cgi?id=1202550&action=edit Fix for Vit Review
Hi Vit, I uploaded the patch to fix for your review. Could you check this patch? Thanks.
- Add openssl-devel dependency to enable HTTPS support. - Add regenerated parser logic. - Improve Ruby load path to run test suite. - Improve files section.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #13 from Jun Aruga jaruga@redhat.com --- Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15708125
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #14 from Vít Ondruch vondruch@redhat.com --- (In reply to Jun Aruga from comment #11)
(In reply to Vít Ondruch from comment #6)
- Purpose of Patch1?
- Not sure about purpose of the Patch1, since if I comment it out and use the previous commands, I can execute the test suite just fine => I
suggest to drop this patch altogether.
- You are right that upstream is inconsistent in this regard, but I'd say that the particular require you are fixing is the way how the test suites typically works, while the proposed fix is antipattern. IOW you should propose fix fixing all the other files.
I thinks the Patch1 is needed. Because I got an error without the Patch1. It depends on the order of test.
<mock-chroot> sh-4.3# RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/ puma-3.6.0 \
ruby -r minitest/autorun -e 'Dir.glob "./test/test_http11.rb", &method(:require)' -- -v
/usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- testhelp (LoadError) from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from /builddir/build/BUILD/rubygem-puma-3.6.0/usr/share/gems/gems/puma-3.6.0/test/ test_http11.rb:4:in `<top (required)>' from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:55:in `require' from -e:1:in `glob' from -e:1:in `<main>'
When I add "-r test/testhelp", its test was passed.
<mock-chroot> sh-4.3# RUBYOPT=-I.:lib:/builddir/build/BUILD/rubygem-puma-3.6.0/usr/lib64/gems/ruby/ puma-3.6.0 \
ruby -r minitest/autorun -r test/testhelp -e 'Dir.glob "./test/test_http11.rb", &method(:require)' -- -v
... 100% passed
Of course you are right in this case, but the scope of the issue is much broader then you think. Please check these lines:
https://github.com/puma/puma/blob/master/test/testhelp.rb#L5-L8
You can see that the "." and "./test" gets on the load path at some point. So you can then require either "testhelp" or "test/testhelp". The correct fix would be to unify this, i.e. you should be able to do only "require 'testhelp'" at the end, because this is how it is typically done nowadays. The "." should not get on the load path at all.
IOW, you can keep it like this for the moment. But I'd suggest you to work with upstream to make this consistent and up to date. This should be the start of the upstream work:
``` find test -name '*.rb' -exec sed -i "s|test/testhelp|testhelp|" {} ; ```
Otherwise the changes looks reasonable. Thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Jun Aruga jaruga@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment|0 |1 #1202550 is| | obsolete| |
--- Comment #15 from Jun Aruga jaruga@redhat.com --- Created attachment 1203205 --> https://bugzilla.redhat.com/attachment.cgi?id=1203205&action=edit Fix for Vit Review #2
Hi Vit,
I updated my patch after your review.
- Delete Patch1: rubygem-puma-3.6.0-update-testhelp-path.patch - find test -name '*.rb' -exec sed -i "s|test/testhelp|testhelp|" {} ;
Koji (Scratch Build): http://koji.fedoraproject.org/koji/taskinfo?taskID=15732386
Could you check this? Thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #16 from Jun Aruga jaruga@redhat.com ---
*** build.log errors
- In %build section there is
To see why this extension failed to compile, please check the mkmf.log which can be found here: /builddir/build/BUILD/puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0/mkmf.log
But the .so is packaged. Is this false positive?
Let me check it.
Hi Pavel,
After asking to the upstream on below page, it looks false positive.
https://github.com/puma/puma/issues/1074
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #17 from Vít Ondruch vondruch@redhat.com --- (In reply to Jun Aruga from comment #15)
- Delete Patch1: rubygem-puma-3.6.0-update-testhelp-path.patch
- find test -name '*.rb' -exec sed -i "s|test/testhelp|testhelp|" {} ;
Well, I said "you can keep it like this" but anyway, since you made the changes, then IMO the least effort would be to add both "." as well as "test" on the load path and you can avoid the sed line entirely.
Thanks for opening the upstream discussion though.
Could you check this?
Otherwise the patch looks good.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #18 from Jun Aruga jaruga@redhat.com --- (In reply to Vít Ondruch from comment #17)
(In reply to Jun Aruga from comment #15)
- Delete Patch1: rubygem-puma-3.6.0-update-testhelp-path.patch
- find test -name '*.rb' -exec sed -i "s|test/testhelp|testhelp|" {} ;
Well, I said "you can keep it like this" but anyway, since you made the changes, then IMO the least effort would be to add both "." as well as "test" on the load path and you can avoid the sed line entirely.
Thanks for opening the upstream discussion though.
Could you check this?
Otherwise the patch looks good.
OK I added "." on the load path, and removed the sed line.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #19 from Vít Ondruch vondruch@redhat.com --- (In reply to Pavel Valena from comment #2)
*** build.log errors
- In %build section there is
To see why this extension failed to compile, please check the mkmf.log which can be found here: /builddir/build/BUILD/puma-3.6.0/usr/lib64/gems/ruby/puma-3.6.0/mkmf.log
But the .so is packaged. Is this false positive?
- In %install section:
cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/ http11_parser.c: Cannot stat: No such file or directory cpio: puma-3.6.0/usr/share/gems/gems/puma-3.6.0/ext/puma_http11/ext/puma_http11/ http11_parser.rl: Cannot stat: No such file or directory
Is this also false positive?
Just for the record, this is very likely RubyGems issue and it is definitely false positive. More over, the "please check the mkmf.log" is just misleading ... But I never bothered to investigate what is the real cause here.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #20 from Jun Aruga jaruga@redhat.com --- OK, thank you for the information.
By the way, I found a test that needs internet connection (test.com). The test test_timeout_in_data_phase have passed by change. But it causes tests take long time with no internet environment.
I checked it with inserting the debug log. https://kojipkgs.fedoraproject.org//work/tasks/4615/15734615/build.log
So, I would skip this test right now, asking to upstream. https://github.com/puma/puma/issues/1098
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #21 from Vít Ondruch vondruch@redhat.com --- Just comment out the specific test. Optionally, you can enable the test if some flag is specified (Bundler has a lot of tests which needs internet connectivity, so they are typically disabled, but can be enabled if the mock is executed with --with-tests flag or something similar [1]).
I would not bother upstream with that. I can imagine that is not their concern ...
[1] http://pkgs.fedoraproject.org/cgit/rpms/rubygem-bundler.git/tree/rubygem-bun...
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Jun Aruga jaruga@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED Fixed In Version| |rubygem-puma-3.6.0-3.fc26 Assignee|pvalena@redhat.com |jaruga@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #22 from Jun Aruga jaruga@redhat.com --- OK thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|jaruga@redhat.com |pvalena@redhat.com
--- Comment #23 from Vít Ondruch vondruch@redhat.com --- The reviewer should be assignee of this ticket [1]:
``` if you want to formally review the package, set the fedora-review flag to ? and assign the bug to yourself. ```
[1] https://fedoraproject.org/wiki/Package_Review_Process#Reviewer
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #24 from Jun Aruga jaruga@redhat.com --- (In reply to Vít Ondruch from comment #23)
The reviewer should be assignee of this ticket [1]:
if you want to formally review the package, set the fedora-review flag to ? and assign the bug to yourself.
[1] https://fedoraproject.org/wiki/Package_Review_Process#Reviewer
What do you mean? I have pushed it by myself.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
--- Comment #25 from Jun Aruga jaruga@redhat.com --- (In reply to Jun Aruga from comment #24)
(In reply to Vít Ondruch from comment #23)
The reviewer should be assignee of this ticket [1]:
if you want to formally review the package, set the fedora-review flag to ? and assign the bug to yourself.
[1] https://fedoraproject.org/wiki/Package_Review_Process#Reviewer
What do you mean? I have pushed it by myself.
Understand it. Please ignore me.
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #26 from Fedora Update System updates@fedoraproject.org --- rubygem-puma-3.6.0-3.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-97ac5ea7b4
https://bugzilla.redhat.com/show_bug.cgi?id=1372718
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2016-09-26 20:33:58
--- Comment #27 from Fedora Update System updates@fedoraproject.org --- rubygem-puma-3.6.0-3.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
package-review@lists.fedoraproject.org