Bug 1440780

Summary: Review Request: mod_http2 - module implementing HTTP/2 for Apache 2
Product: [Fedora] Fedora Reporter: Luboš Uhliarik <luhliari>
Component: Package ReviewAssignee: Remi Collet <fedora>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora, jorton, package-review
Target Milestone: ---Flags: fedora: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-06-09 18:58:29 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:
Attachments:
Description Flags
review.txt none

Description Luboš Uhliarik 2017-04-10 13:08:14 UTC
Spec URL: https://luhliarik.fedorapeople.org/mod_http2.spec
SRPM URL: https://luhliarik.fedorapeople.org/mod_http2-1.10.0-1.fc23.src.rpm
Description: The mod_h2 Apache httpd module implements the HTTP2 protocol (h2+h2c) on top of libnghttp2 for httpd 2.4 servers.
Fedora Account System Username: luhliarik

Comment 1 Remi Collet 2017-04-11 08:15:44 UTC
Quick notes.

Error: Transaction check error:
  file /usr/lib64/httpd/modules/mod_http2.so from install of mod_http2-1.10.0-1.fc25.remi.x86_64 conflicts with file from package httpd-2.4.25-1.fc25.x86_64

What is the need of this package if already provided by httpd ?


- compatibility macro for htpd 2.2 are obviously unneeded (IIRC only _httpd_mmn is need at buildtime)

- latest version is 1.10.1

- LICENSE file must be packaged

- why is there versioned .so, obviously not wanted

- why no LoadModule directive for mod_proxy_httpd2 ?

- %changelog need to be fixed (with your name/address)

Comment 2 Joe Orton 2017-04-11 09:53:39 UTC
Because upstream mod_http2 moves a bit faster than httpd releases, we're planning to package it separately and drop it from httpd.

Comment 3 Luboš Uhliarik 2017-04-11 10:02:31 UTC
Thank you Remi, for your review.

(In reply to Remi Collet from comment #1)
> Quick notes.
> 
> Error: Transaction check error:
>   file /usr/lib64/httpd/modules/mod_http2.so from install of
> mod_http2-1.10.0-1.fc25.remi.x86_64 conflicts with file from package
> httpd-2.4.25-1.fc25.x86_64
> 
> What is the need of this package if already provided by httpd ?

Joe decided, that it would be better to have mod_http2 as separate module, since upstream is producing releases more often, than httpd releases. I knew about this problem, so I will will add Requires tag, which will require httpd >= than version, which will not contain mod_http2 (I have to remove it from httpd).

> 
> - compatibility macro for htpd 2.2 are obviously unneeded (IIRC only
> _httpd_mmn is need at buildtime)
- you are right, I forgot to remove them from SPEC file. Fixed.

> 
> - latest version is 1.10.1
- they released this version yesterday, after I made this package... Fixed.

> 
> - LICENSE file must be packaged
- they are missing LICENCE file in release tar.gz. Reported to upstream. https://github.com/icing/mod_h2/issues/136

> 
> - why is there versioned .so, obviously not wanted
- fixed

> 
> - why no LoadModule directive for mod_proxy_httpd2 ?
- fixed

> 
> - %changelog need to be fixed (with your name/address)
- fixed


You can download updated SPEC and SRPM here:
Spec URL: https://luhliarik.fedorapeople.org/mod_http2.spec
SRPM URL: https://luhliarik.fedorapeople.org/mod_http2-1.10.1-1.fc23.src.rpm

Comment 4 Remi Collet 2017-04-11 10:28:32 UTC
(In reply to Joe Orton from comment #2)
> Because upstream mod_http2 moves a bit faster than httpd releases, we're
> planning to package it separately and drop it from httpd.

Thanks for clarification.


> echo "LoadModule http2_module modules/mod_http2.so" > %{buildroot}%{_sysconfdir}/httpd/conf.d/10-h2.conf
> echo "LoadModule proxy_http2_module modules/mod_proxy_http2.so" > %{buildroot}%{_sysconfdir}/httpd/conf.d/10-proxy_h2.conf

Shouldn't this be in conf.modules.d ?
So using %_httpd_modconfdir

Comment 5 Remi Collet 2017-04-11 10:35:33 UTC
Please also use %_httpd_moddir instead of harcoded path

Comment 6 Luboš Uhliarik 2017-04-11 11:16:59 UTC
Hardcoded paths have been substituted with macros (%_httpd_moddir and %_httpd_moddir)

You can download updated SPEC and SRPM here:
Spec URL: https://luhliarik.fedorapeople.org/mod_http2.spec
SRPM URL: https://luhliarik.fedorapeople.org/mod_http2-1.10.1-1.fc23.src.rpm

Comment 7 Remi Collet 2017-04-11 11:35:41 UTC
Created attachment 1270745 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1440780
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++

Comment 8 Remi Collet 2017-04-11 11:36:08 UTC
Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines

[x]: Package does not generate any conflict.
	Expected, as this module is going to be removed from httpd
	(package split)
	=> if possible add Conflicts: httpd < ...

[!]: %build honors applicable compiler flags or justifies otherwise.
	Please use 
		%configure
		make %{?_smp_mflags} V=1

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required

[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required

Comment 9 Remi Collet 2017-04-11 13:13:11 UTC
+Conflicts:      httpd < 2.4.25-8

[x]: Package does not generate any conflict.

-%configure
+%configure \
+  --libdir=%{_libdir} \
+  --prefix=%{_prefix}

[x]: %build honors applicable compiler flags or justifies otherwise.

-rm -rf %{buildroot}

[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the

-%clean
-rm -rf %{buildroot}
-

[x]: Package has no %clean section with rm -rf %{buildroot} (or


No more blocker, so approved.

Comment 10 Remi Collet 2017-04-11 13:14:20 UTC
BTW, --libdir and --prefix unneeded (already part of %configure)

Comment 11 Gwyn Ciesla 2017-05-15 14:10:57 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/mod_http2

Comment 12 Fedora Update System 2017-05-15 20:52:47 UTC
mod_http2-1.10.1-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-8f2df0e3f2

Comment 13 Fedora Update System 2017-05-16 06:10:09 UTC
mod_http2-1.10.1-1.fc26 has been pushed to the Fedora 26 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-2017-8f2df0e3f2

Comment 14 Fedora Update System 2017-05-16 09:54:39 UTC
mod_http2-1.10.5-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-8f2df0e3f2

Comment 15 Fedora Update System 2017-05-16 17:44:04 UTC
mod_http2-1.10.5-1.fc26 has been pushed to the Fedora 26 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-2017-8f2df0e3f2

Comment 16 Fedora Update System 2017-06-09 18:58:29 UTC
mod_http2-1.10.5-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.