Spec URL: https://luhliarik.fedorapeople.org/nginx-mod-headers-more.spec SRPM URL: https://luhliarik.fedorapeople.org/nginx-mod-headers-more-0.37-1.fc42.src.rpm Description: This module allows you to add, set, or clear any output or input header that you specify. Fedora Account System Username: luhliarik
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