Bug 875150

(In reply to comment #0)
> Ok, the package builds. It's somewhat separated (not finished yet separating
> it correctly). I'd like to ask for some co-packager's help here.

Besides other things you probably meant plugins libraries still packaged into -libs, while -plugins sub-package is empty. I guess these libraries were meant to be there.

I'm not sure if mariadb upstream has already tried to create own systemd unit file, but in any case it will be necessary in Fedora to start service cleanly. If you're not familiar with creating such files, I can help with that.

From the first quick look we'll probably want to run tests during build.

You also use "%package %{name}-doc", but correctly it should only be "%package doc", since otherwise we get mariadb-mariadb-doc instead mariadb-doc.

Another set of issues:

We usually don't want any *.a files in rpms, so the following should be removed:
/usr/lib64/libmysqlclient.a
/usr/lib64/libmysqlclient_r.a
/usr/lib64/libmysqlservices.a

All config files should be marked as noreplace:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Configuration_files

Unversioned libraries (*.so) should be in -devel subpackage:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Devel_Packages

You should run ldconfig:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Shared_Libraries

If I see correctly, the packages rely on default datadir path, but we should define one explicitelly, something like /var/lib/mariadb? Then the directory should also be owned by -server subpackage:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#The_directory_is_wholly_contained_in_your_package.2C_or_involves_core_functionality_of_your_package

The rule above is valid also for log file.

I see /etc/my.cnf.d/client.cnf and /etc/my.cnf.d/mysql_clients.cnf are packaged in -server subpackage, aren't they suppose to be included in -client and -libs subpackage?

(In reply to comment #1)
> Besides other things you probably meant plugins libraries still packaged
> into -libs, while -plugins sub-package is empty. I guess these libraries
> were meant to be there.

Ok, I've moved the plugins to the plugin package. My intention is to provide individual plugin sub-packages but, for now, let's leave them there.

> I'm not sure if mariadb upstream has already tried to create own systemd
> unit file, but in any case it will be necessary in Fedora to start service
> cleanly. If you're not familiar with creating such files, I can help with
> that.

Thank you for your help. It is appreciated. It would be nice to get some collaboration ;)

> From the first quick look we'll probably want to run tests during build.

Right now, the last test (Test #45) which, I presume, checks for debug info of some kind, it's failing. I need to figure out why and, maybe, disable that test for now. 

> You also use "%package %{name}-doc", but correctly it should only be
> "%package doc", since otherwise we get mariadb-mariadb-doc instead
> mariadb-doc.

You're right. I corrected this already. I will be posting the SRPM after I check your other comments.

Thank you, very much, for your interest.

(In reply to comment #2)
> Another set of issues:
> 
> We usually don't want any *.a files in rpms, so the following should be
> removed:
> /usr/lib64/libmysqlclient.a
> /usr/lib64/libmysqlclient_r.a
> /usr/lib64/libmysqlservices.a

ok, removed them using find

> All config files should be marked as noreplace:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Configuration_files

Done, I, also, moved the my.cnf to common and separated the client and server configs into their corresponding sub-packages

> Unversioned libraries (*.so) should be in -devel subpackage:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Devel_Packages

Ok, done.

> You should run ldconfig:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Shared_Libraries

Done. What should be the correct location of the post and postun in the kickstart? After %install?

> If I see correctly, the packages rely on default datadir path, but we should
> define one explicitelly, something like /var/lib/mariadb? Then the directory
> should also be owned by -server subpackage:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#The_directory_is_wholly_contained_in_your_package.
> 2C_or_involves_core_functionality_of_your_package
> 
> The rule above is valid also for log file.

As up to now, these guys consider themselves a MySQL replacement. Most lib dirs and tools (/usr/bin) are named mysql*.

The big issue with this is that the libclient files replace the mysql's client files. Also, libs is going to have issues with this. 

What I am saying is that it's either MySQL or MariaDB so far. The later works fine with the former's lib /var/lib dir. 

What to do?

> I see /etc/my.cnf.d/client.cnf and /etc/my.cnf.d/mysql_clients.cnf are
> packaged in -server subpackage, aren't they suppose to be included in
> -client and -libs subpackage?

Yeah, I put mysql_clients and client.cnf in the clients subpackage. Why do you mention libs?

errr.. it seems I am having some problems when separating *.so in /var/lib64 from everything... It appears that MariaDB needs those libs there...

SRPM: http://renich.fedorapeople.org/SRPMS/mariadb-5.5.28-2.fc17.src.rpm
SPEC: http://renich.fedorapeople.org/SPECS/mariadb.spec

(In reply to comment #4)
> Done. What should be the correct location of the post and postun in the
> kickstart? After %install?

You mean in a spec file? I believe it doesn't matter, but they are usually located between %install/%check and %files sections.

> > If I see correctly, the packages rely on default datadir path, but we should
> > define one explicitelly, something like /var/lib/mariadb? Then the directory
> > should also be owned by -server subpackage:
> > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> > Guidelines#The_directory_is_wholly_contained_in_your_package.
> > 2C_or_involves_core_functionality_of_your_package
> > 
> > The rule above is valid also for log file.
> 
> As up to now, these guys consider themselves a MySQL replacement. Most lib
> dirs and tools (/usr/bin) are named mysql*.

As I understand the "replacement" statement, we can safely use maria instead of mysql from the view of application. But I wouldn't rely on any promise to use exactly the same file formats in datadir and I'm not sure if there even is such promise.

> The big issue with this is that the libclient files replace the mysql's
> client files. Also, libs is going to have issues with this.

If we'll ever replace mysql by maria, I'd suggest to do it together with new major/minor version rebase (probably to 5.6). Then soname versions would bump and all clients would have to be rebuilt anyway.

> What I am saying is that it's either MySQL or MariaDB so far. The later
> works fine with the former's lib /var/lib dir. 
> 
> What to do?

I'd suggest not to use former mysql's directory silently. Even if it works in most cases, it can do bad things if it doesn't work. On the other hand, all users would have to do some manual work after installing maria, which is annoying. At any case, I think this can be consulted on fedora-list to find the least problematic solution.

> Yeah, I put mysql_clients and client.cnf in the clients subpackage. Why do
> you mention libs?

Actually I'm not sure what is meant by mysql_clients. I thought client.cnf is a config file for mysql utility and mysql_clients.cnf for libmysqlclient libraries. That's why I thought it should be in the same package as that library. But maybe I didn't get their purpose correctly.

(In reply to comment #5)
> errr.. it seems I am having some problems when separating *.so in /var/lib64
> from everything... It appears that MariaDB needs those libs there...

I'm not sure if we understand each other here. By separating *.so into devel I meant only packaging them into different sub-package, not moving them in the tree, just keep them in /usr/lib64.

You can do the separation as follows:

%files libs
%{_libdir}/libmysqlclient*.so.*
...

%files devel
%{_libdir}/libmysqlclient*.so
...

(In reply to comment #7)
> As I understand the "replacement" statement, we can safely use maria instead
> of mysql from the view of application. But I wouldn't rely on any promise to
> use exactly the same file formats in datadir and I'm not sure if there even
> is such promise.

I understand what you mean. You're right. I will move the lib into it's own dir.

> > The big issue with this is that the libclient files replace the mysql's
> > client files. Also, libs is going to have issues with this.
> 
> If we'll ever replace mysql by maria, I'd suggest to do it together with new
> major/minor version rebase (probably to 5.6). Then soname versions would
> bump and all clients would have to be rebuilt anyway.

Ok, so, I will leave it at that for now. Let's see what comes in the future and what guidance I can get in the devel list. I will contact upstream as well.

> I'd suggest not to use former mysql's directory silently. Even if it works
> in most cases, it can do bad things if it doesn't work. On the other hand,
> all users would have to do some manual work after installing maria, which is
> annoying. At any case, I think this can be consulted on fedora-list to find
> the least problematic solution.

Ok, so, I will consult. ;)
 
> > Yeah, I put mysql_clients and client.cnf in the clients subpackage. Why do
> > you mention libs?
> 
> Actually I'm not sure what is meant by mysql_clients. I thought client.cnf
> is a config file for mysql utility and mysql_clients.cnf for libmysqlclient
> libraries. That's why I thought it should be in the same package as that
> library. But maybe I didn't get their purpose correctly.

Honestly, I don't know either. Another thing to consult upstream.

> (In reply to comment #5)
> > errr.. it seems I am having some problems when separating *.so in /var/lib64
> > from everything... It appears that MariaDB needs those libs there...
> 
> I'm not sure if we understand each other here. By separating *.so into devel
> I meant only packaging them into different sub-package, not moving them in
> the tree, just keep them in /usr/lib64.
> 
> You can do the separation as follows:
> 
> %files libs
> %{_libdir}/libmysqlclient*.so.*
> ...
> 
> %files devel
> %{_libdir}/libmysqlclient*.so
> ...

Ok, I think I didn't explain myself correctly. 

I am not moving the file's location. I am packaging *.so in devel, as you asked and, when devel not installed, this brings problems.

If I put the .so files into devel, you install everything but devel, then, you get a broken symlink. Look at this dir structure:

$ ls -lS /usr/lib64/libmysql*
-rwxr-xr-x. 1 root root 66511620 oct 19 10:36 /usr/lib64/libmysqld.so.18
-rwxr-xr-x. 1 root root  4937388 oct 19 10:12 /usr/lib64/libmysqlclient.so.18.0.0
-rwxr-xr-x. 1 root root  2222056 oct 19 10:38 /usr/lib64/libmysqlclient_r.so.16.0.0
-rwxr-xr-x. 1 root root  2212424 oct 19 10:38 /usr/lib64/libmysqlclient.so.16.0.0
-rwxr-xr-x. 1 root root  2052744 oct 19 10:38 /usr/lib64/libmysqlclient_r.so.15.0.0
-rwxr-xr-x. 1 root root  2043496 oct 19 10:38 /usr/lib64/libmysqlclient.so.15.0.0
lrwxrwxrwx. 1 root root       26 nov  8 02:03 /usr/lib64/libmysqlclient_r.so.15 -> libmysqlclient_r.so.15.0.0
lrwxrwxrwx. 1 root root       26 nov  8 02:03 /usr/lib64/libmysqlclient_r.so.16 -> libmysqlclient_r.so.16.0.0
lrwxrwxrwx. 1 root root       24 nov  8 02:03 /usr/lib64/libmysqlclient.so.15 -> libmysqlclient.so.15.0.0
lrwxrwxrwx. 1 root root       24 nov  8 02:03 /usr/lib64/libmysqlclient.so.16 -> libmysqlclient.so.16.0.0
lrwxrwxrwx. 1 root root       24 nov  8 02:03 /usr/lib64/libmysqlclient.so.18 -> libmysqlclient.so.18.0.0
lrwxrwxrwx. 1 root root       20 nov  8 02:03 /usr/lib64/libmysqlclient.so -> libmysqlclient.so.18
lrwxrwxrwx. 1 root root       17 nov  8 02:03 /usr/lib64/libmysqlclient_r.so -> libmysqlclient.so
lrwxrwxrwx. 1 root root       17 nov  8 02:03 /usr/lib64/libmysqlclient_r.so.18 -> libmysqlclient.so
lrwxrwxrwx. 1 root root       17 nov  8 02:03 /usr/lib64/libmysqlclient_r.so.18.0.0 -> libmysqlclient.so
lrwxrwxrwx. 1 root root       15 nov  8 02:03 /usr/lib64/libmysqld.so -> libmysqld.so.18


As you can see, some symlinks point to lbmysqlclient.so; including: libmysqlclient_r.so.18, libmysqlclient_r.so.18.0.0.

That said, you will notice that libmysqlclient.so is a symlink as well that points to libmysqlclient.so.18 so... should I still put it, and all that points to it, into devel?

(In reply to comment #8)
> > If we'll ever replace mysql by maria, I'd suggest to do it together with new
> > major/minor version rebase (probably to 5.6). Then soname versions would
> > bump and all clients would have to be rebuilt anyway.
> 
> Ok, so, I will leave it at that for now. Let's see what comes in the future
> and what guidance I can get in the devel list. I will contact upstream as
> well.

I think we can still keep going to work on this review to have it prepared for the future.

> That said, you will notice that libmysqlclient.so is a symlink as well that
> points to libmysqlclient.so.18 so... should I still put it, and all that
> points to it, into devel?

Ah, I see the problem now. The same issue has mysql as well and it is solved there by removing all libmysqlclient_r.so.18.* links and keeping just libmysqlclient_r.so for backward compatibility. However, since
  $ repoquery --whatrequires '*libmysqlclient_r.so*'
returns no results, what about to remove all libmysqlclient_r.so* links for now and see if anybody needs it at all?

(In reply to comment #9)
> I think we can still keep going to work on this review to have it prepared
> for the future.

I apologize for my English; it seems I keep saying things the wrong way. 

I will, gladly, keep packaging MariaDB actively.
 
> Ah, I see the problem now. The same issue has mysql as well and it is solved
> there by removing all libmysqlclient_r.so.18.* links and keeping just
> libmysqlclient_r.so for backward compatibility. However, since
>   $ repoquery --whatrequires '*libmysqlclient_r.so*'
> returns no results, what about to remove all libmysqlclient_r.so* links for
> now and see if anybody needs it at all?

Ok, so I will remove the libmysqlclient_r.so* for now as you suggest. I'll have the SRPM revision 3  up in a while.

Sorry for quite long delay, but I've finally found time to get back to this. However, after consulting with another packagers it seems much better to not use completely new SPEC file and packaging mariadb from scratch, but we should rather work on existing mysql.spec instead. There are several reasons:

1. there are many things we really need in mariadb.spec and that are already in mysql.spec (like test suite run, proper build requires, ...)
2. admins will probably better accept a new package if it's as much similar as the previous one
3. patches - most of them are still valid even for mariadb source (but I got rid of some)

So I've done changes in mysql.spec and converted it to mariadb.spec, which is available here together with SRPM:

Spec URL: http://hhorak.fedorapeople.org/mariadb-review/mariadb.spec
SRPM URL: http://hhorak.fedorapeople.org/mariadb-review/mariadb-5.5.28a-4.fc18.src.rpm

No all tests pass during the build currently, so the test build doesn't break if test suite fails. What is important, the tests suite works fine after install, so the failing tests are only build-time problem.

Anyway, I'm currently assigned as a reviewer, but I'd rather take over the packaging work, if you don't mind, Renich. Then I'd need someone else to do the package review actually, since it wouldn't be good to do the review on own work.

So we can simply switch the roles (which means you would be the reviewer) or I can ask someone else to do that, in case you're not feeling comfortable with that. What do you think?

(In reply to comment #11)
> Sorry for quite long delay, but I've finally found time to get back to this.
> However, after consulting with another packagers it seems much better to not
> use completely new SPEC file and packaging mariadb from scratch, but we
> should rather work on existing mysql.spec instead. There are several reasons:
> 
> 1. there are many things we really need in mariadb.spec and that are already
> in mysql.spec (like test suite run, proper build requires, ...)
> 2. admins will probably better accept a new package if it's as much similar
> as the previous one
> 3. patches - most of them are still valid even for mariadb source (but I got
> rid of some)
> 
> So I've done changes in mysql.spec and converted it to mariadb.spec, which
> is available here together with SRPM:
> 
> Spec URL: http://hhorak.fedorapeople.org/mariadb-review/mariadb.spec
> SRPM URL:
> http://hhorak.fedorapeople.org/mariadb-review/mariadb-5.5.28a-4.fc18.src.rpm
> 
> No all tests pass during the build currently, so the test build doesn't
> break if test suite fails. What is important, the tests suite works fine
> after install, so the failing tests are only build-time problem.

I understand. If I recall correctly, I suggested this approach in the beginning. Anyway, it seems like a good idea.

> Anyway, I'm currently assigned as a reviewer, but I'd rather take over the
> packaging work, if you don't mind, Renich. Then I'd need someone else to do
> the package review actually, since it wouldn't be good to do the review on
> own work.
> 
> So we can simply switch the roles (which means you would be the reviewer) or
> I can ask someone else to do that, in case you're not feeling comfortable
> with that. What do you think?

Well, I am glad to let you be the packager. The goal is to have MariaDB in Fedora. Let me know if I can assist you.

On the reviewer offer, It'd be my first package as a reviewer and I don't feel it's a good one to start with; I rather start with a simpler one.

I will kindly refuse. Please, find another reviewer.

Thank you for your hard work and effort.

I've done the review as discussed with Honza.

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[N/A] = Not Applicable

Issues:
=======
[!]: Changelog in prescribed format.
5.5.28-4 -> 5.5.28a-4

[!]: License field in the package spec file matches the actual license.
Looking at the README and the result of fedora-review's licensecheck
(will attach) it seems there could be also LGPLv2 and/or BSD licensed files.
Could you check it ?

[!]: Final provides and requires are sane.
I think you should remove all occurrences of %{epoch}, because it's undefined:
# rpm -qp --provides mariadb-5.5.28a-4.fc18.x86_64.rpm 
mysql = %{epoch}:5.5.28a-4.fc18

[!]: %check is present and all tests pass.
Shouldn't the tests be run in %check instead of %build ?

===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
[!]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Each %files section contains %defattr if rpm < 4.4
[N/A]: Macros in Summary, %description expandable at SRPM build time.
[N/A]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[!]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macros.
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: No %config files under /usr.
[x]: Package does not generate any conflict.
     Note: Conflicts: tags contain justification.
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: Rpmlint is run on all rpms the build produces.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[N/A]: Large documentation must go in a -doc subpackage.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[N/A]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[!]: Final provides and requires are sane.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: %check is present and all tests pass.
[N/A]: Packages should try to preserve timestamps of original installed files.
[x]: Files in /run, var/run and /var/lock uses tmpfiles.d when appropriate
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Spec file according to URL is the same as in SRPM.
[x]: Rpmlint is run on all installed packages.
rpmlint output is quite big, but there are either false-positives or
upstream problems (please look at them and report as much as possible upstream)
or problems that I already mentioned elsewhere.

Created attachment 674968 [details]
fedora-review's licensecheck output

Some suggestions for spec file clean-up:

1) 
- %post libs
- /sbin/ldconfig
+ %post libs -p /sbin/ldconfig

- %postun libs
- if [ $1 = 0 ] ; then
-     /sbin/ldconfig
- fi
+ %postun libs -p /sbin/ldconfig

and there's no need to 'Requires: /sbin/ldconfig' in '%package libs'

2) Given that mariadb won't be available for F17 you don't need to check whether macroized systemd scriptlets exist (they do in F18+)

- %if 0%{?systemd_post:1}
- %systemd_post mysqld.service
- %else
- if [ $1 = 1 ]; then
-    # Initial installation
-    /bin/systemctl daemon-reload >/dev/null 2>&1 || :
- fi
- %endif
+ %systemd_post mysqld.service

3) could this be removed (there's been rpm 4.10 in F18) ? 
# When rpm 4.9 is universal, this could be cleaned up:
%global __perl_requires %{SOURCE999}
%global __perllib_requires %{SOURCE999}

(In reply to comment #13)
> [!]: Changelog in prescribed format.
> 5.5.28-4 -> 5.5.28a-4
> 
> [!]: License field in the package spec file matches the actual license.
> Looking at the README and the result of fedora-review's licensecheck
> (will attach) it seems there could be also LGPLv2 and/or BSD licensed files.
> Could you check it ?
> 
> [!]: Final provides and requires are sane.
> I think you should remove all occurrences of %{epoch}, because it's
> undefined:
> # rpm -qp --provides mariadb-5.5.28a-4.fc18.x86_64.rpm 
> mysql = %{epoch}:5.5.28a-4.fc18

These are mistakes -- I'll fix them in the next round.

> [!]: %check is present and all tests pass.
> Shouldn't the tests be run in %check instead of %build ?

Tom, do you remember if there is a reason to run the tests in %build section?

(In reply to comment #16)
> Tom, do you remember if there is a reason to run the tests in %build section?

The reason I've historically run mysql's regression tests (and also postgresql's) in the %build part is that %check is misdesigned: it runs the checks only after the %install section, so that a lot of work is wasted if the regression test fails.

I might be willing to tolerate that and use %check if it actually did anything useful, like say if rpmbuild had an option to control whether to run the %check part or not.  Since it doesn't, and we have to roll our own support for that anyhow (cf %runselftest in these specfiles), I find %check to be completely useless and best ignored.

YMMV of course.

(In reply to comment #17)
> (In reply to comment #16)
> > Tom, do you remember if there is a reason to run the tests in %build section?
> 
> The reason I've historically run mysql's regression tests (and also
> postgresql's) in the %build part is that %check is misdesigned: it runs the
> checks only after the %install section, so that a lot of work is wasted if
> the regression test fails.

Well, since test-suite is not interrupted in case some tests fail and comparing duration of testing to duration of %install section, I don't see the wasting time too much important here.

> I might be willing to tolerate that and use %check if it actually did
> anything useful, like say if rpmbuild had an option to control whether to
> run the %check part or not. Since it doesn't, and we have to roll our own
> support for that anyhow (cf %runselftest in these specfiles), I find %check
> to be completely useless and best ignored.

Actually, rpmbuild allows to skip %check section with --nocheck now, but I'm not sure if it can be used in wrappers like fedpkg, if people don't use rpmbuild directly.

Anyway, I can imagine someone can benefit from moving the tests to %check section, so I'd vote for the change.

(In reply to comment #18)
> (In reply to comment #17)
> > I might be willing to tolerate that and use %check if it actually did
> > anything useful, like say if rpmbuild had an option to control whether to
> > run the %check part or not.

> Actually, rpmbuild allows to skip %check section with --nocheck now,

[ checks an F18 machine... ]  Huh, they finally got around to adding that.  Okay, given that I'm fine with using %check instead of a hand-rolled define.  The only real downside is that we couldn't make the can't-be-root test at the very start, but that seems like a minor issue.

(In reply to comment #19)
> [ checks an F18 machine... ]  Huh, they finally got around to adding that. 
> Okay, given that I'm fine with using %check instead of a hand-rolled define.
> The only real downside is that we couldn't make the can't-be-root test at
> the very start, but that seems like a minor issue.

I'd rather keep the runselftest define, it'll still be handy for faster building in mock or scratch building in koji -- I haven't found a way how to skip %check in these tools.

Created attachment 676314 [details]
Changes made in spec file

(In reply to comment #13)
> [!]: License field in the package spec file matches the actual license.
> Looking at the README and the result of fedora-review's licensecheck
> (will attach) it seems there could be also LGPLv2 and/or BSD licensed files.
> Could you check it ?

Some of the files indeed are under LGPLv2, respectively BSD license. I've included also corresponding license files, but I'm not sure what to do with two identical license files (COPYING.Percona and COPYING.Google) -- both BSD license, but with different copyright authority. I've included both, but I'm not sure if this is a correct solution. If anybody knows better how to handle it, just let me know.

(In reply to comment #15)
> 2) Given that mariadb won't be available for F17 you don't need to check
> whether macroized systemd scriptlets exist (they do in F18+)
> 
> - %if 0%{?systemd_post:1}
> - %systemd_post mysqld.service
> - %else
> - if [ $1 = 1 ]; then
> -    # Initial installation
> -    /bin/systemctl daemon-reload >/dev/null 2>&1 || :
> - fi
> - %endif
> + %systemd_post mysqld.service

Even if mariadb won't be officially in F17-, some users may want to build it there on their own, so I'd better keep the backward compatible macro there for the time F17 is supported. I've added a comment about the reason into the spec.

The rest issues should be fixed, I'm attaching a diff for easier check.

Also the links are updated:
Spec URL: http://hhorak.fedorapeople.org/mariadb-review/mariadb.spec
SRPM URL: http://hhorak.fedorapeople.org/mariadb-review/mariadb-5.5.28a-5.fc18.src.rpm

(In reply to comment #21)
> ... but I'm not sure what to do with
> two identical license files (COPYING.Percona and COPYING.Google) -- both BSD
> license, but with different copyright authority. I've included both ...

I'd do the same.

> Even if mariadb won't be officially in F17-, some users may want to build it
> there on their own, so I'd better keep the backward compatible macro there

Fair enough.

I haven't spotted any more problems so I consider this package APPROVED.

New Package SCM Request
=======================
Package Name: mariadb
Short Description: A community developed branch of MySQL database
Owners: hhorak tgl
Branches: f19
InitialCC:

Summary and request name mismatch, please correct.  Also, no need to request
f19, devel branch is automatic.

New Package SCM Request
=======================
Package Name: mariadb
Short Description: A community developed branch of MySQL database
Owners: hhorak tgl
Branches: 
InitialCC:

Git done (by process-git-requests).

Package Change Request
======================
Package Name: mariadb
New Branches: f18 f17
Owners: hhorak tgl
InitialCC: 

In order to make testing of mariadb more easy for larger group of people, we'd like to offer F17 and F18 builds for testing as well.

Git done (by process-git-requests).

MariaDB packages are built in koji.

Package Change Request
======================
Package Name: mariadb
New Branches: el5 el6
Owners: hhorak tgl
InitialCC: 

I've heard some requests for MariaDB to be in EPEL as well, since it would be handy for testing 5.1 -> 5.5 migration. Keeping the same content as in Fedora shouldn't bring too much extra work.

Git done (by process-git-requests).