Bug 2307815 - Review Request: tinyows - WFS-T and FE implementation server
Summary: Review Request: tinyows - WFS-T and FE implementation server
Keywords:
Status: POST
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL: https://mapserver.org/tinyows/
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-08-26 00:09 UTC by Neil Hanlon
Modified: 2024-09-13 14:28 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 7941992 to 7990614 (593 bytes, patch)
2024-09-07 01:44 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 7990614 to 7990651 (293 bytes, patch)
2024-09-07 02:09 UTC, Fedora Review Service
no flags Details | Diff

Description Neil Hanlon 2024-08-26 00:09:54 UTC
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

Comment 1 Fedora Review Service 2024-08-26 00:18:32 UTC
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.

Comment 3 Fedora Review Service 2024-09-07 01:44:03 UTC
Created attachment 2045650 [details]
The .spec file difference from Copr build 7941992 to 7990614

Comment 4 Fedora Review Service 2024-09-07 01:44:05 UTC
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.

Comment 6 Fedora Review Service 2024-09-07 02:09:57 UTC
Created attachment 2045656 [details]
The .spec file difference from Copr build 7990614 to 7990651

Comment 7 Fedora Review Service 2024-09-07 02:09:58 UTC
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.

Comment 8 Zbigniew Jędrzejewski-Szmek 2024-09-12 13:59:13 UTC
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.

Comment 9 Neil Hanlon 2024-09-12 15:28:26 UTC
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

Comment 11 Zbigniew Jędrzejewski-Szmek 2024-09-12 20:05:42 UTC
%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.

Comment 12 Neil Hanlon 2024-09-13 02:43:12 UTC
(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.

Comment 14 Zbigniew Jędrzejewski-Szmek 2024-09-13 11:50:56 UTC
+ 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.

Comment 15 Zbigniew Jędrzejewski-Szmek 2024-09-13 11:58:00 UTC
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.

Comment 16 Neil Hanlon 2024-09-13 12:34:44 UTC
(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!

Comment 18 Zbigniew Jędrzejewski-Szmek 2024-09-13 14:28:37 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.