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.src.rpm 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/
Hi, I will do the review.
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#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. 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
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
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-puma
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.src.rpm http://people.redhat.com/jaruga/git/rubygem-puma/README.Fedora
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
Vit thanks for the view. Today I will bump the release at first.
I bumped the release. rubygem-puma-3.6.0-1.fc25 rubygem-puma-3.6.0-1.fc26
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.
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.org/rpms/rubygem-puma before I am going to do your mentioned items.
(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.
Created attachment 1202550 [details] 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.
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15708125
(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.
Created attachment 1203205 [details] 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.
> > *** 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
(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.
(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.
(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.
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
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-bundler.spec#n3
OK thanks.
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
(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.
(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.
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
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.