https://bugzilla.redhat.com/show_bug.cgi?id=1979403 Jaroslav Škarvada <jskarvad@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(jskarvad@redhat.c | |om) | --- Comment #2 from Jaroslav Škarvada <jskarvad@xxxxxxxxxx> --- (In reply to Ben Beasley from comment #1) Thanks for the review. > You accidentally listed %{_includedir}/wdsp.h twice. Please remove it from > the base package’s %files and leave it in the -devel package’s %files only. > Fixed. > - The Fedora compiler flags are respected, in that they are added to the > command line and are after those from the project, so they should prevail. > It > would probably be better to stop the build system from adding -O3 > altogether, > though. See > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags. > Something like this should work (passing the default OPTIONS from the > Makefile with -O3 removed): > > %make_build OPTIONS='-g -fPIC -D _GNU_SOURCE' \ > CFLAGS="%{build_cflags}" \ > LDFLAGS="%{build_ldflags}" \ > GTK_INCLUDE=GTK > Fixed. > - I see there is a comment in the spec file that you encouraged upstream to > start versioning the shared library. Was this in a public venue that you > can > link to? If so, could you add the link? If not, could you say so in the > comment (e.g. “via private email”)? > It's in the mentioned PR, duplicated the link. > - Since the Makefile uses pkg-config/pkgconf to find GTK, this > > BuildRequires: gtk3-devel > > would be better written as: > > BuildRequires: pkgconfig(gtk+-3.0) > > See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > PkgConfigBuildRequires/. > Fixed. > - Please add a comment to the spec file where the %check section would be, > stating that there are no tests. > I cannot see the check/test target in the Makefile. I.e. there is no reason for the comment. From where this requirement is coming from? > - For downstream so-versioning, you should start with “0.1” or some other > “0.n” > to help avoid future conflicts in case upstream starts versioning with “0”. > Currently, you are using “0”. Please see > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_downstream_so_name_versioning > for details. > Fixed. Personally I think 0.0.0 is much better version than the 0.1 from the guidelines. Upstream usually starts with the major 0 and minor 1 and boom conflict, but I changed it. Spec URL: https://jskarvad.fedorapeople.org/wdsp/wdsp.spec SRPM URL: https://jskarvad.fedorapeople.org/wdsp/wdsp-0-0.2.20210705gitc55342c5.fc33.src.rpm -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
