Bug 2341923

Created attachment 2073693 [details]
review.txt$

Minor problem

There is no LICENSE file upstream
please file a bug asking for it (and add link in the spec)

The license text is in the README file
perhaps this one should be tag with %license

No blocker

==== APPROVED ====

Absence of any %license marked file seems to me like a blocker, not a minor issue. %doc marked documents are allowed to be omitted when installing the package. I think packaging guidelines requires %license marked text to be always installed, which is not the case now.

More on: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text

I propose to either do not mark README.markdown as %doc or mark it directly %license. I would propose requesting separate file with license text only added upstream. But until separate %license file is done, I think the package has to ensure license text is present among its files when the package is installed. That forbids %doc having that text IMO.

It might be possible to "extract" license text from README at build time, then using it as separate marked file.

sed --silent -e '/^Copyright & License/,/\[Back to TOC\]/ p' README.markdown > LICENSE.markdown
touch -r README.markdown LICENSE.markdown

Technically upstream includes license, just not in separate file. That could be separated in package spec until upstream moves it to separate file. But I do not think %doc can be used for it.

Additional problem seems unowned parent directories.

%{nginx_moddir}
%{nginx_modconfdir}

These directories need to exist, but the package does not have any direct Requires: ensuring package owning them must be present. I think potential shared library dependencies should not be relied on in this case, but not only BuildRequires should be used. Add Requires: nginx-core for correct depend owning those.

[!]: Package requires other packages for directories it uses.

Hello Remi and Peter,

Thanks a lot for your review.

(In reply to Petr Menšík from comment #3)
> Absence of any %license marked file seems to me like a blocker, not a minor
> issue. %doc marked documents are allowed to be omitted when installing the
> package. I think packaging guidelines requires %license marked text to be
> always installed, which is not the case now.
> 
> More on:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> LicensingGuidelines/#_license_text
> 
> I propose to either do not mark README.markdown as %doc or mark it directly
> %license. I would propose requesting separate file with license text only
> added upstream. But until separate %license file is done, I think the
> package has to ensure license text is present among its files when the
> package is installed. That forbids %doc having that text IMO.
> 
> It might be possible to "extract" license text from README at build time,
> then using it as separate marked file.
> 
> sed --silent -e '/^Copyright & License/,/\[Back to TOC\]/ p' README.markdown
> > LICENSE.markdown
> touch -r README.markdown LICENSE.markdown
> 
> Technically upstream includes license, just not in separate file. That could
> be separated in package spec until upstream moves it to separate file. But I
> do not think %doc can be used for it.

I agree that separate LICENSE SHOULD be included and therefor I've already 
created a upstream request to include license text in a separate file [0].

According to fedora packaging guidelines [1] it is not not mandatory it is 
only recommended (SHOULD instead of MUST), so I think it should not be a 
showstopper and we don't need to wait until upstream resolves this issue. 

> 
> Additional problem seems unowned parent directories.
> 
> %{nginx_moddir}
> %{nginx_modconfdir}
> 
> These directories need to exist, but the package does not have any direct
> Requires: ensuring package owning them must be present. I think potential
> shared library dependencies should not be relied on in this case, but not
> only BuildRequires should be used. Add Requires: nginx-core for correct
> depend owning those.
> 
> [!]: Package requires other packages for directories it uses.

Every module package requires nginx(abi) = %{_nginx_abiversion} based on 
the ABI version of nginx which was used to build this module e.g.

$ rpm -qp ./nginx-mod-headers-more-0.37-1.fc41.x86_64.rpm --requires |grep nginx
nginx(abi) = 1.26.2

[0] https://github.com/openresty/headers-more-nginx-module/pull/168
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/

> According to fedora packaging guidelines [1] it is not not mandatory it is 
> only recommended (SHOULD instead of MUST), so I think it should not be a 
> showstopper and we don't need to wait until upstream resolves this issue. 

Guidelines are unclear (this was clearer in the past)

Fedora Guidelines say SHOULD, but BSD License says MUST ;)

The reason why I said this is not a blocker (BSD doesn't care about "how" it is provided, so %doc is OK)

Upstream luckily accepted my PR[0] in few hours, so the %license issue is already solved. 

[0] https://github.com/openresty/headers-more-nginx-module/releases/tag/v0.38

The Pagure repository was created at https://src.fedoraproject.org/rpms/nginx-mod-headers-more

Package is now in repositories, closing review.