Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-1.fc39.src.rpm Description: TinyOWS server implements latest WFS-T standard versions, as well as related standards such as Filter Encoding (FE). Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122486179
Copr build: https://copr.fedorainfracloud.org/coprs/build/7941992 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2307815-tinyows/fedora-rawhide-x86_64/07941992-tinyows/fedora-review/review.txt Found issues: - License file LICENSE is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/tinyows Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names - Documentation size is 79590155 bytes in 1652 files. Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-1.fc39.src.rpm - update to 1.2.2 for re-review
Created attachment 2045650 [details] The .spec file difference from Copr build 7941992 to 7990614
Copr build: https://copr.fedorainfracloud.org/coprs/build/7990614 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2307815-tinyows/fedora-rawhide-x86_64/07990614-tinyows/fedora-review/review.txt Found issues: - License file LICENSE is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/tinyows Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names - Documentation size is 78884145 bytes in 1652 files. Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-1.fc39.src.rpm - add scheme/LICENSE as %license
Created attachment 2045656 [details] The .spec file difference from Copr build 7990614 to 7990651
Copr build: https://copr.fedorainfracloud.org/coprs/build/7990651 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2307815-tinyows/fedora-rawhide-x86_64/07990651-tinyows/fedora-review/review.txt Found issues: - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/tinyows Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names - Documentation size is 78853171 bytes in 1652 files. Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
FTR, the mail thread about unretirement is here: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/NLF5LLVGGBZ4DHEMMYTBERJU5ALUF5HB/ > make %{?_smp_mflags} %make_build > cp %{_bindir}/shp2pgsql %{buildroot}%{_bindir}/shp2pgsql Hmmm? This copies /usr/bin/shp2pgsql under %buildroot! > pushd %{buildroot}%{_datadir}/%{name}/ > ln -s ../../../%{_sysconfdir}/%{name}/config.xml config.xml > popd That pushd/popd is unnecessary. Just do ln -s ../../../%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_datadir}/%{name}/config.xml or even better ln -s --relative %{buildroot}%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_datadir}/%{name}/ > %defattr(-,root,root,-) Not needed. %autorelease and %autochangelog? The spec file looks reasonable, even if a bit dated. But I'm confused about the provenance of shp2pgsql.
Thanks for the review! > > make %{?_smp_mflags} > %make_build made this change locally but forgot to push up > > cp %{_bindir}/shp2pgsql %{buildroot}%{_bindir}/shp2pgsql > Hmmm? This copies /usr/bin/shp2pgsql under %buildroot! great catch, I'm not really sure why that's there.. > > pushd %{buildroot}%{_datadir}/%{name}/ > > ln -s ../../../%{_sysconfdir}/%{name}/config.xml config.xml > > popd > > That pushd/popd is unnecessary. Just do > ln -s ../../../%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_datadir}/%{name}/config.xml > or even better > ln -s --relative %{buildroot}%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_datadir}/%{name}/ noted! thanks. > > %defattr(-,root,root,-) > Not needed. ack > %autorelease and %autochangelog? yeah, i probably should use these to make my life easier > The spec file looks reasonable, even if a bit dated. But I'm confused about the provenance of shp2pgsql. agreed, removed
Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-4.fc42.src.rpm address review comments
%changelog\n%autochangelog /usr/share/doc/tinyows is included in the main package and it's huge. I think it should be moved to -doc. In the %description, please explain what the acronyms mean. "WSF-T" is pretty cryptic.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #11) > %changelog\n%autochangelog done, thanks > > /usr/share/doc/tinyows is included in the main package and it's huge. > I think it should be moved to -doc. > Done. > In the %description, please explain what the acronyms mean. "WSF-T" is > pretty cryptic. Agreed. Expanded this.
Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-5.fc42.src.rpm - address final review comments
+ package name is OK + latest version (1.2.2) + license is acceptable for Fedora (MIT) + license is specified correctly (in spdx format) + BR/R/P look OK + builds and installs OK - %check is not present, according to the comment, a running postgresql instance would be needed to run tests + distro CLFAGS are used in compilation + large docs are split out in -doc Package is APPROVED.
Hmm, actually, the flags are not used. It seems CFLAGS are, but LDFLAGS are missing. rpmlint says: tinyows.x86_64: W: position-independent-executable-suggested /usr/bin/tinyows I see that package notes are missing from the binary too. So I think this needs another round.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #15) > Hmm, actually, the flags are not used. It seems CFLAGS are, but LDFLAGS are > missing. > > rpmlint says: > tinyows.x86_64: W: position-independent-executable-suggested /usr/bin/tinyows > > I see that package notes are missing from the binary too. > > So I think this needs another round. I will check into this and update here. Thanks!
Spec URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows.spec SRPM URL: https://neil.fedorapeople.org/reviews/tinyows/tinyows-1.2.2-6.fc42.src.rpm - respect fedora LDFLAGS
Now it looks all good. rpmlint: tinyows.x86_64: W: no-manual-page-for-binary tinyows tinyows.x86_64: W: no-documentation tinyows.x86_64: W: files-duplicate /usr/share/tinyows/schema/LICENSE /usr/share/licenses/tinyows/LICENSE tinyows-doc.noarch: E: files-duplicated-waste 16728835 tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a0a15f53f62900b330685e184da60e25e_cgraph.map /usr/share/doc/tinyows-doc/doxygen/alist_8c_a0a15f53f62900b330685e184da60e25e_cgraph.map tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a0a15f53f62900b330685e184da60e25e_cgraph.md5 /usr/share/doc/tinyows-doc/doxygen/alist_8c_a0a15f53f62900b330685e184da60e25e_cgraph.md5 tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a0a15f53f62900b330685e184da60e25e_cgraph.png /usr/share/doc/tinyows-doc/doxygen/alist_8c_a0a15f53f62900b330685e184da60e25e_cgraph.png tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.map /usr/share/doc/tinyows-doc/doxygen/alist_8c_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.map tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.md5 /usr/share/doc/tinyows-doc/doxygen/alist_8c_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.md5 tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.png /usr/share/doc/tinyows-doc/doxygen/alist_8c_a19cbf414bb13aa9fb0aac2fd26d36cfc_cgraph.png tinyows-doc.noarch: W: files-duplicate /usr/share/doc/tinyows-doc/doxygen/ows__api_8h_a815b58142ab6406ce5312cf55e44fe23_cgraph.map /usr/share/doc/tinyows-doc/doxygen/alist_8c_a815b58142ab6406ce5312cf55e44fe23_cgraph.map ... I'm not sure what is going on here. I would expect that post-build scriptlets automatically hardlink any repeated files. Ah, no the hardlinking is only done for python pyc files, and not in general. Please consider adding the following change: diff --git tinyows.spec tinyows.spec index e7d479fb24..fccbdcf4c2 100644 --- tinyows.spec +++ tinyows.spec @@ -73,7 +73,6 @@ sed -i -e 's|HTML_FOOTER|HTML_FOOTER=no_date_footer.html\n\#|g' doc/Doxyfile make doxygen %install - install -d %{buildroot}%{_bindir} install -p -m 0755 tinyows %{buildroot}%{_bindir}/ install -d %{buildroot}%{_datadir}/%{name} @@ -83,6 +82,10 @@ install -p -m 0644 ms4w/apps/tinyows-svn/config.xml %{buildroot}%{_sysconfdir}/% ln -s --relative %{buildroot}%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_datadir}/%{name}/ +mkdir -p %{buildroot}%{_pkgdocdir} +cp -a doc/doxygen %{buildroot}%{_pkgdocdir}/ +hardlink --ignore-time --reflink=never %{buildroot}%{_pkgdocdir}/ + # NOTE(neil): 2024-08-25 tests require a postgres database running; unable to run in check # https://github.com/MapServer/tinyows/blob/f1dc7bc86fc4d69faddd79ed2804d98c11802ba8/.github/workflows/linux.sh#L19-L22 %dnl %check @@ -96,7 +99,7 @@ ln -s --relative %{buildroot}%{_sysconfdir}/%{name}/config.xml %{buildroot}%{_da %files doc %doc README.md VERSION.md -%doc doc/doxygen +%doc %{_pkgdocdir}/doxygen Package is APPROVED.