https://bugzilla.redhat.com/show_bug.cgi?id=1352408 --- Comment #3 from Igor Gnatenko <ignatenko@xxxxxxxxxx> --- (In reply to Julian Sikorski from comment #2) > (In reply to Igor Gnatenko from comment #1) > Thank you for taking the review! Please find my feedback below: > > > Missing BuildRequires: gcc > > Has something changed? I thought gcc was part of minimal buildroot and does > not need to be required explicitly. In any case, something is pulling it in > as the build is working. Something changed. Since (~ f23) it's required to put all BRs like gcc. > > > > BuildRequires: intltool > > I would add also BuildRequires: gettext > > Intltool pulls gettext by requiring gettext-devel, is an explicit > requirement necessary? No, hopefully at some point upstream will stop using intltool and we will switch to gettext. > > > > Requires: pkg-config > > Drop it > > OK > > > > make %{?_smp_mflags} > > could be replaced with %make_build > > OK, it was not in the template created by rpmdev-newspec on f22. rpmdev-newspec is completely outdated. > > > > rm -rf $RPM_BUILD_ROOT > > Drop it > > Why? rpmdev-newspec still put it in as of f24 It's not needed since ~ EL6. > > > > find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';' > > find %{buildroot}%{_libdir} -type f -name '*.la' -delete -print > > Why is this needed? The version I have now is what rpmdev-newspec as of f24 > enters. Not all packages maintained well. When you use -delete, it doesn't spawn any command, it just uses unlinkat(). > > > > %find_lang %{name} --all-name > > I think --all-name is not needed, but not sure. > > I replaced it with %{name}-%{apiver} > > > > %{_datadir}/gtk-doc/html/%{name}-0.4 > > %doc %{_datadir}/gtk-doc/html/%{name}-0.4 > > OK > %dir %{_datadir}/gtk-doc > %dir %{_datadir}/gtk-doc/html you don't need to do this. > > > > $RPM_BUILD_ROOT > > %{buildroot} > > I prefer the variable format and as per current guidelines [1] both are > acceptable. hopefully it will be deprecated at some point. it's better to use %{buildroot}. but as you noted, it's up to you. > > > > > also would be great to do: %global apiver 0.4 and use it in %files. > > OK > > New releases: > Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec > SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-2.fc24.src.rpm > > [1] > https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot. > 7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS Resolution: ALMOST GOOD -- 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 https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx