https://bugzilla.redhat.com/show_bug.cgi?id=1372718 Vít Ondruch <vondruch@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |vondruch@xxxxxxxxxx --- Comment #6 from Vít Ondruch <vondruch@xxxxxxxxxx> --- 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 -- 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 https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx