https://bugzilla.redhat.com/show_bug.cgi?id=1111691 --- Comment #6 from David Nichols <david@xxxxxxxx> --- (In reply to Christopher Meng from comment #3) > More detailed initial review thought(Note you need a sponsor, I can't help, > I will address this at the end): > > 1. The packaging style looks like a decade ago. > > %define qore_ver 0.8.11 > > You should put 0.8.11 in Version tag and use %{version} instead of custom > macro, we have some fundamental macros which you should avoid using custom > macro replaced > you are right, this spec file was born a long time ago. this has been fixed > 2. I don't think you've read the guideline, for example, %define -> %global: > > https://fedoraproject.org/wiki/Packaging:Guidelines#. > 25global_preferred_over_.25define > I did not read them closely enough, you are right. this is now fixed. > 3. Remove those non-Fedora conditional bits: > sll removed > 4. # see if we can determine the distribution type > %if 0%{!?dist:1} > %define rh_dist %(if [ -f /etc/redhat-release ];then cat > /etc/redhat-release|sed "s/[^0-9.]*//"|cut -f1 -d.;fi) > %if 0%{?rh_dist} > %define dist .rhel%{rh_dist} > %else > > --------- > > Please learn how to use macro %{?el}/%{?fedora} > fixed > 4. Drop obsoleted RPM macros which are still heavily used by other distros: > > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root > > %defattr(-,root,root,-) > > %clean > > %defattr(-,root,root,-) > removed / fixed > 5. You are polluting dist tag: > > http://fedoraproject.org/wiki/Packaging:DistTag > ok done > 6. Drop BuildRequires: gcc-c++: > > https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 > dropped / done > 7. Never make BuildRequires: fdupes in any Fedora specs, we don't need it. > removed (was there for opensuse) > 8. You should avoid packaging libraries with version as its package name: > > libqore5 > > You'd better change it to libqore or qore-libs > changed to libqore for fedora / rhel > 9. I still don't understand those Provides: in libqore, can't RPM handle > this? > actually, no (at least not AFAIK). the Provides are there so that qore binary modules, which are loaded at runtime by libqore, can be matched with the module ABI of the qore library. I plan on making submission requests for qore module packages later (hopefully after I can get sponsorship to main qore for Fedora - I realize that this is not a given and anyway will take time and commitment on my part). There are quiet a few of these already. The modules then will declare Requires: for the specific module API that they are compiled against. These modules will be binary-loadable by future libqore packages that declare the old module ABI. The RPM system then will not complain when libqore is upgraded and a module compiled against a previous version of qore (and using an older, but still compatible qore module ABI) is still on the system. Otherwise without this mechanism, I would have to add an explicit dependency to libqore in the modules' spec files, which would be more restrictive than what is actually necessary, since future versions of libqore normally maintain ABI compatibility with earlier versions. It was my impression that the spec files Provides: lines for such artificial dependencies was to handle this sort of situation. > 10. %ifarch x86_64 ppc64 x390x > c64=--enable-64bit > %endif > # need to configure with /usr as prefix as this will be used to derive the > module directory > ./configure RPM_OPT_FLAGS="$RPM_OPT_FLAGS" --prefix=/usr --disable-debug > --disable-static $c64 --libdir=%{_libdir} > > a) %configure macro should be used > > b) Does qore work on ARM? We will have AArch64(ARM v8) in the future. > I have moved all the 64-bit detection stuff to configure and added support for 64-bit ARM (aarch64) > 11. /usr/bin/ -> %{_bindir} > > %{_prefix}/include/ -> %{_includedir} > > /usr/share/man/ -> %{_mandir} > > I think you don't need to care about RHEL5 nowadays. > done - removed > 12. Why not merge 2 doc packages into 1 -doc? > The reason for this is because the devel-doc package is only needed for programmers programming against the C++ API (ie for qore binary modules). This doc package is large, but most users won't need it (I expect). Most users will only need the doc package, which has the Qore documentation telling programmer's how to program in the Qore language. So From my point of view it makes sense to have two packages for the two very different kinds of documentation provided by qore. However, if this is a blocking issue for acceptance by Fedora, then let me know, and I will merge them both. > 13. mkdir -p $RPM_BUILD_ROOT/usr/bin > mkdir -p $RPM_BUILD_ROOT/%{module_dir}/%{qore_ver} > mkdir -p $RPM_BUILD_ROOT/usr/man/man1 > > I think install script should do that(create in install script or with > install -p), since you are the upstream, these could be enhanced. > you are right, this was not necesary, configure generates a Makefile that performs these actions automatically. This has been removed from the spec file. > 13. > https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > done > 14. %post -n libqore5 > ldconfig %{_libdir} > > %postun -n libqore5 > ldconfig %{_libdir} > > http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Shared_libraries > done > ------------------------------------ > Anyway, read and follow this if you want to be sponsored: > > https://fedoraproject.org/wiki/Join_the_package_collection_maintainers thanks for that! I had indeed read that and had already verified my package with a koji scratch build. I plan on following up on the other activities recommended for achieving sponsorship in the near future. I realize that this will take some time, and that I will need to demonstrate a commitment to Fedora's processes and quality standards. Thank you very much for your very detailed review and the time you took to make it. I very much appreciate it. The new SRPM and spec file have been uploaded to the URLs above (copied here) with all the changes from the comments here. - Spec URL: http://qore.org/srpms/qore.spec - SRPM URL: http://qore.org/srpms/qore-0.8.11-1.fc20.src.rpm thanks, David -- 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://admin.fedoraproject.org/mailman/listinfo/package-review