[Bug 658754] Review Request: cubrid - a very fast and reliable open source SQL database server

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=658754

--- Comment #33 from Michael Schwendt <bugs.michael@xxxxxxx> ---
It would be an idea to follow the "Join the packagers" procedure and request
fedorapeople web space for a much more convenient way to offer the src.rpm and
spec file.


A couple of concerns, because the spec file is large:

* There are questionable macro definitions at the top. For example:

>  %global file_version   1

is used only once. In the Source0 URL. What's the benefit?

For an update of the software, you would need to touch three macros to
construct a Source0 URL that needs to be parsed by a tool (with e.g. spectool
-g) to get a download location you could use in a browser or with wget/curl.
The Source0 tag currently includes a hardcoded "9.1.0", however, so the usage
of macros here appears to be half-hearted.

> Version:       %{cubrid_version}.%{build_version}

This implicitly defines %{version} to get the same value. So, wherever you
currently use %{cubrid_version}.%{build_version}, you could use %{version}
instead. Unless the versioning scheme is very complicated, and then macros
become a burden.

> %global cubrid_vendor  Search Solution Corporation

Not used anywhere.

> %global debug_package %{nil}

Why that?


> Requires:      expect
> Requires:      ncurses
> Requires:      csh
> Requires:      gc
> Requires:      lzo
> Requires:      pcre

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> BuildRequires: glibc-devel

Harmless, but glibc-devel is supposed to be available in the minimum build
environment as a dependency of gcc/gcc-c++.
https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2


> %package devel 
> Requires:   %{name} = %{cubrid_version}.%{build_version}

That isn't strict enough for how Fedora does it:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Imagine you release a fix where you want the -devel package (or other
subpackages) to be in sync with the base package.


> %package demodb 
> Group:      Development/Libraries

An unusual group for files stored below /usr/share. It might be an idea to not
define those Group tags anymore (they are optional nowadays).

> Requires:   %{name} = %{cubrid_version}.%{build_version}

Same as with the -devel package.


> %build
> %ifarch x86_64
>     CFLAGS=" -m64 " 
>     CUBRID_COMMON_CONFIGURE="${CUBRID_COMMON_CONFIGURE} --enable-64bit"
> %endif

What about other 64-bit targets?


> ./autogen.sh
> ./configure ${CUBRID_COMMON_CONFIGURE}

autogen.sh but no %configure macro usage? See "rpm --eval %configure". If
%configure cannot be used with this configure script, please add a comment in
the spec file.


> %install

It's odd that this section is so long. Why is it necessary to install/adjust so
many things here _after_ "make install"? Why doesn't "make install" do all
that?


> find %{buildroot} -size 0 -delete

Hopefully you'll never need to include an empty file or touch one to include it
as %ghost.


> %post devel -p /sbin/ldconfig

Not true. The package includes only headers. Even if it contained *.so symlinks
for libs, running ldconfig would not be needed. It's a tool for the run-time
linker.

> %postun devel -p /sbin/ldconfig

Same here.

> %post demodb 
> /sbin/ldconfig

It contains only files below /usr/share.

> %postun demodb 
> /sbin/ldconfig

Same here.

> %preun devel

An empty scriptlet?


The %preun section in the spec file is after "%post demodb". Sorting the
scriptlets would increase readability.


The %files sections list many files with very generic file names (even headers
such as %{_includedir}/dbi.h). It'll be a lot of fun to avoid conflicts with
other packages.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=eKK1UiWdTM&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]