https://bugzilla.redhat.com/show_bug.cgi?id=1372718



--- Comment #11 from Jun Aruga <jar...@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.

-- 
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

Reply via email to