Bug 1268910 - Review Request: h2o - HTTP server for HTTP/1.x and HTTP/2
Review Request: h2o - HTTP server for HTTP/1.x and HTTP/2
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2015-10-05 11:18 EDT by marhag87
Modified: 2015-11-14 09:40 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description marhag87 2015-10-05 11:18:21 EDT
Spec URL: https://raw.githubusercontent.com/marhag87/h2o-rpm/master/h2o.spec
SRPM URL: https://github.com/marhag87/h2o-rpm-src/raw/master/h2o-1.5.0-1.fc22.src.rpm
Description: An optimized HTTP server with support for HTTP/1.x and HTTP/2
Fedora Account System Username: marhag87

This is my first package, I need a sponsor.
Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=11336228
Comment 1 Michael Cronenworth 2015-10-05 11:50:34 EDT
Unofficial review:
Source URL guidelines: https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Tags
 Change: https://github.com/h2o/h2o/archive/v%{version}.tar.gz
     to: https://github.com/h2o/h2o/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
 and update your tarball file name.

A few nice-to-haves, but not blockers, are to use %autosetup instead of %setup to negate requiring %patch lines and removing %defattr(-,root,root).
Comment 2 Upstream Release Monitoring 2015-10-05 12:06:43 EDT
marhag87's scratch build of h2o-1.5.0-3.fc22.x86_64.rpm for f22 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11337281
Comment 3 Upstream Release Monitoring 2015-10-05 12:24:10 EDT
marhag87's scratch build of h2o-1.5.0-3.fc22.src.rpm for f22 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11337302
Comment 4 marhag87 2015-10-05 12:37:32 EDT
Thanks for the feedback! I've fixed the problems you mentioned in 1.5.0-3:

Spec URL: https://github.com/marhag87/h2o-rpm/blob/master/h2o.spec
SRPM URL: https://github.com/marhag87/h2o-rpm-src/raw/master/h2o-1.5.0-3.fc22.src.rpm
Comment 5 Michael Schwendt 2015-10-05 13:39:49 EDT
> Summary:          An optimized HTTP server with support for HTTP/1.x and HTTP/2

> %description
> H2O is a very fast HTTP server written in C.

Why is %summary longer and more detailed than %description?

Typically, the latter expands on the former:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description

Suggestion:

  Summary: HTTP server

  %description
  H2O is a very fast HTTP server written in C. It supports HTTP/1.x and HTTP/2.


> %post
> /sbin/ldconfig
> %systemd_post %{name}.service

> %postun
> /sbin/ldconfig

Running ldconfig here doesn't achieve anything related to this package, because this package doesn't store any shared lib in runtime linker's search path.


> %doc %{_datarootdir}/doc/%{name}

%_datarootdir is %_datadir, which is /usr/share, and files below /usr/share/doc are implicitly marked as %doc. See "rpm -E %__docdir_path".

> %doc %{_mandir}/man1/%{name}.1.gz

Same for %_mandir.


> %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf
> 
> %{_libexecdir}/%{name}/setuidgid

The directories %_sysconfdir/%name and %_libexecdir/%{name} are not included yet.

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories


Consider pointing the fedora-review tool at this ticket:
fedora-review -b 1268910
Comment 6 marhag87 2015-10-05 15:18:02 EDT
Thanks for the comments!

I have updated the spec file as per your suggestions. I couldn't quite get the fedora-review tool to work, but I'll give it another try with the updated spec file path from this comment (forgot to use raw in the last one).

Spec URL: https://raw.githubusercontent.com/marhag87/h2o-rpm/master/h2o.spec
SRPM URL: https://raw.githubusercontent.com/marhag87/h2o-rpm-src/master/h2o-1.5.0-4.fc22.src.rpm
Comment 7 Upstream Release Monitoring 2015-10-05 15:27:22 EDT
marhag87's scratch build of h2o-1.5.0-4.fc22.src.rpm for f22 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11339443
Comment 8 Michael Schwendt 2015-10-13 05:29:19 EDT
You may want to practise a bit more %files sections and ownership of files and directories. In particular %dir entries and inclusion of directory trees as covered here: https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> %{_sysconfdir}/%{name}
> %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf

The second line here tells that  %{_sysconfdir}/%{name}  is a directory, or else there could not be a %name.conf file be stored within it. But then the first line 

  %{_sysconfdir}/%{name}

includes the directory *and* anything in it. It other words, it includes the directory regardless of whether it's empty or whether it contains a huge tree of files and subdirs.

> %{_libexecdir}/%{name}
> %{_libexecdir}/%{name}/setuidgid

Same here.

Typically, rpmbuild warns about such cases that lead to files being listed twice in a %files section. From your scratch build:

  https://kojipkgs.fedoraproject.org//work/tasks/9447/11339447/build.log

  warning: File listed twice: /etc/h2o/h2o.conf
  warning: File listed twice: /usr/libexec/h2o/setuidgid

There is more than one solution to this problem. One involves %dir attributes to include *only* an entry for a directory but not any files within it. The files then must be listed explicitly in the %files section:

  %dir %{_sysconfdir}/%{name}
  %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf

  %dir %{_libexecdir}/%{name}
  %{_libexecdir}/%{name}/setuidgid

An alternative is to include a directory tree directly, but that would conflict with %config and other attributes on specific files. The guidelines give a few more examples.


Also in the build.log:

> No tests were found!!!

That refers to the  %check  section.


> -o h2o -rdynamic libressl-build/lib/libssl.a
> libressl-build/lib/libcrypto.a -lz -lpthread -ldl -lrt 

That seems to be related to "%cmake -DWITH_BUNDLED_SSL=on ." and ignores the openssl-devel BuildRequires, too. I hope it's just a bug in the package and not intentional:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries

As I understand it, the recent changes to the No Bundling policies are still being discussed because they are considered controversial.
Comment 9 Upstream Release Monitoring 2015-10-20 11:25:40 EDT
marhag87's scratch build of h2o-1.5.2-1.fc22.src.rpm for f22 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11516898
Comment 10 marhag87 2015-10-20 11:35:08 EDT
Thanks for the feedback!

I have updated the specfile with your suggestions, and to the latest version that is available.

In the link you gave they define libraries like this: "compiled third party source code resulting in shared or static linkable files, interpreted third party source code such as Python, PHP and others." The bundling for this program doesn't produce any new libraries. I have already removed the bundled SSL, but there are several other dependencies: "cloexec golombset klib libyrmcds mruby mruby-dir mruby-env mruby-iijson mruby-io mruby-onig-regexp mruby-pack mruby-require neverbleed picohttpparser picotest yaml yoml". Only yaml seems to have a package already. Should I try to poke upstream about decoupling them and work on making packages for all of them?

Do you have a link to the discussion about the policy discussion?

Spec URL: https://raw.githubusercontent.com/marhag87/h2o-rpm/master/h2o.spec
SRPM URL: https://raw.githubusercontent.com/marhag87/h2o-rpm-src/master/h2o-1.5.2-1.fc22.src.rpm
Comment 11 Upstream Release Monitoring 2015-11-14 09:40:51 EST
marhag87's scratch build of h2o-1.5.4-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11835100

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