https://bugzilla.redhat.com/show_bug.cgi?id=1372718 --- Comment #11 from Jun Aruga <jaruga@xxxxxxxxxx> --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx