Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1372718 - Review Request: rubygem-puma - A simple, fast, threaded, and highly concurrent HTTP 1.1 server
Summary: Review Request: rubygem-puma - A simple, fast, threaded, and highly concurren...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Pavel Valena
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-09-02 13:12 UTC by Jun Aruga
Modified: 2016-09-27 00:33 UTC (History)
3 users (show)

Fixed In Version: rubygem-puma-3.6.0-3.fc26
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-27 00:33:58 UTC
Type: Bug
Embargoed:
pvalena: fedora-review+


Attachments (Terms of Use)
Fix for Vit Review (deleted)
2016-09-19 16:33 UTC, Jun Aruga
no flags Details | Diff
Fix for Vit Review #2 (deleted)
2016-09-21 10:09 UTC, Jun Aruga
no flags Details | Diff

Description Jun Aruga 2016-09-02 13:12:41 UTC
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/

Comment 1 Pavel Valena 2016-09-02 13:50:00 UTC
Hi,

I will do the review.

Comment 2 Pavel Valena 2016-09-02 15:53:00 UTC
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

Comment 3 Jun Aruga 2016-09-02 16:08:44 UTC
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

Comment 4 Gwyn Ciesla 2016-09-02 17:58:35 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-puma

Comment 5 Jun Aruga 2016-09-02 18:08:20 UTC
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

Comment 6 Vít Ondruch 2016-09-05 12:12:14 UTC
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

Comment 7 Jun Aruga 2016-09-13 07:10:13 UTC
Vit thanks for the view.
Today I will bump the release at first.

Comment 8 Jun Aruga 2016-09-13 21:28:44 UTC
I bumped the release.

rubygem-puma-3.6.0-1.fc25
rubygem-puma-3.6.0-1.fc26

Comment 9 Vít Ondruch 2016-09-14 10:25:36 UTC
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.

Comment 10 Jun Aruga 2016-09-14 11:08:36 UTC
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.

Comment 11 Jun Aruga 2016-09-19 13:57:45 UTC
(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.

Comment 12 Jun Aruga 2016-09-19 16:33:33 UTC
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.

Comment 13 Jun Aruga 2016-09-19 16:35:33 UTC
Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15708125

Comment 14 Vít Ondruch 2016-09-20 11:44:15 UTC
(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.

Comment 15 Jun Aruga 2016-09-21 10:09:53 UTC
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.

Comment 16 Jun Aruga 2016-09-21 10:14:39 UTC
> > *** 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

Comment 17 Vít Ondruch 2016-09-21 10:42:32 UTC
(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.

Comment 18 Jun Aruga 2016-09-21 10:56:52 UTC
(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.

Comment 19 Vít Ondruch 2016-09-21 11:00:01 UTC
(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.

Comment 20 Jun Aruga 2016-09-21 14:13:59 UTC
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

Comment 21 Vít Ondruch 2016-09-21 14:23:53 UTC
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

Comment 22 Jun Aruga 2016-09-21 14:45:48 UTC
OK thanks.

Comment 23 Vít Ondruch 2016-09-21 14:49:05 UTC
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

Comment 24 Jun Aruga 2016-09-21 15:02:32 UTC
(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.

Comment 25 Jun Aruga 2016-09-21 15:16:31 UTC
(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.

Comment 26 Fedora Update System 2016-09-22 09:55:07 UTC
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

Comment 27 Fedora Update System 2016-09-27 00:33:58 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.