https://bugzilla.redhat.com/show_bug.cgi?id=1111691
--- Comment #6 from David Nichols <da...@qore.org> ---
(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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
