Bug 1268910

Summary: Review Request: h2o - HTTP server for HTTP/1.x and HTTP/2
Product: [Fedora] Fedora Reporter: marhag87
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: carl, package-review, vedran
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-07-23 09:54:11 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 177841    

Description marhag87 2015-10-05 15:18:21 UTC
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 15:50:34 UTC
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 16:06:43 UTC
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 16:24:10 UTC
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 16:37:32 UTC
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 17:39:49 UTC
> 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 19:18:02 UTC
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 19:27:22 UTC
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 09:29:19 UTC
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 15:25:40 UTC
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 15:35:08 UTC
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 14:40:51 UTC
marhag87's scratch build of h2o-1.5.4-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11835100

Comment 12 Carl George 2019-02-25 16:45:33 UTC
marhag87, here are the latest docs regarding bundled libraries.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

yaml is already packaged as libyaml, so at a minimum that must be unbundled.  mruby was submitted in bug 818414, but has since been closed.  You should consider reviving that review and then unbundling mruby if possible.

Comment 13 Package Review 2020-07-10 00:53:49 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.