Bug 494986 - Review Request: mysqludf_xql - MySQL UDF library for XML output
Review Request: mysqludf_xql - MySQL UDF library for XML output
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-08 17:56 EDT by Yanko Kaneti
Modified: 2009-06-04 13:14 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-04 13:14:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Yanko Kaneti 2009-04-08 17:56:47 EDT
Spec URL: http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql.spec
SRPM URL: http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-2.fc11.src.rpm
Description: 
The lib_mysqludf_xql library provides an array of functions, which can be used 
to create XML output directly from MySQL using a single SQL query.
Comment 1 Yanko Kaneti 2009-04-15 09:41:05 EDT
new version
SRPM URL:
http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-4.fc10.src.rpm
Comment 2 Yanko Kaneti 2009-04-19 03:10:04 EDT
* Sun Apr 19 2009 Yanko Kaneti <yaneti@declera.com> - 0.9.7-5
- file EOL encoding for some docs

http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql.spec
http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-5.fc10.src.rpm
Comment 3 Yanko Kaneti 2009-04-19 04:13:42 EDT
* Sun Apr 19 2009 Yanko Kaneti <yaneti@declera.com> - 0.9.7-6
- ldconfig also in postun
- remove explicit gcc-c++

http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql.spec
http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-6.fc10.src.rpm
Comment 4 arthurguru 2009-05-07 20:00:56 EDT
Yanko,

Here is my informal review of package mysqludf_xql
One issue I couldn't determine either way is whether this should be a -devel package or not.

MUST: rpmlint must be run on every package. The output should be posted in the
review.
- mysqludf_xql-0.9.7-6.fc10.src.rpm OK
- mysqludf_xql-0.9.7-6.fc10.i386.rpm OK
- mysqludf_xql-debuginfo-0.9.7-6.fc10.i386.rpm OK
- OK

MUST: The package must be named according to the Package Naming Guidelines .
- OK

MUST: The spec file name must match the base package %{name}
- OK

MUST: The package must meet the Packaging Guidelines
- OK

MUST: The package must be licensed with a Fedora approved license and meet the
Licensing Guidelines .
- OK

MUST: The License field in the package spec file must match the actual license.
- OK

MUST: 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 must be included in %doc
- OK

MUST: The spec file must be written in American English.
- OK

MUST: The spec file for the package MUST be legible.
- OK

MUST: The sources used to build the package must match the upstream source
- ac690468d8303ea5e09fcf7d29326b22 *lib_mysqludf_xql-0.9.7.tar.gz (RPM)
- ac690468d8303ea5e09fcf7d29326b22 *lib_mysqludf_xql-0.9.7.tar.gz (upstream)
- OK

MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
- OK (i386)

MUST: If the package does not successfully compile, build or work on an
architecture
- OK

MUST: All build dependencies must be listed in BuildRequires
- OK

MUST: The spec file MUST handle locales properly.
- OK, no locales available

MUST: Every binary RPM package (or subpackage) which stores shared library
files (not just symlinks) in any of the dynamic linker's default paths, must
call ldconfig in %post and %postun.
- OK

MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.
- OK, no relocatable package

MUST: A package must own all directories that it creates.
- OK

MUST: A Fedora package must not list a file more than once in the spec file's
%files listings.
- OK

MUST: Permissions on files must be set properly.
- OK

MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
- OK

MUST: Each package must consistently use macros.
- OK

MUST: The package must contain code, or permissable content.
- OK

MUST: Large documentation files must go in a -doc subpackage.
- OK, no large documentation

MUST: If a package includes something as %doc, it must not affect the runtime
of the application.
- OK

MUST: Header files must be in a -devel package.
- OK, no header files

MUST: Static libraries must be in a -static package.
- OK, no static libraries

MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for
directory ownership and usability).
- OK, no .pc files

MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
then library files that end in .so (without suffix) must go in a -devel
package.
- OK, no suffixed library files.

MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release}
- OK if not -devel package, otherwise %{name} = %{version}-%{release} is not specified.

MUST: Packages must NOT contain any .la libtool archives, these must be removed
in the spec if they are built.
- OK, no .la files

MUST: Packages containing GUI applications must include a %{name}.desktop file
- OK, no gui available

MUST: Packages must not own files or directories already owned by other
packages.
- OK

MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}
(or $RPM_BUILD_ROOT).
- OK

MUST: All filenames in rpm packages must be valid UTF-8.
- OK

SHOULD - All patches should have an upstream bug link or comment
- Not Ok, No comment provided.

Conclusion:
It's hard to say whether SQL queries are a devel activity or not, if a not a devel package then this package has passed my review.

Kind regards,
Arthur Gouros.
Comment 5 Yanko Kaneti 2009-05-07 21:13:34 EDT
(In reply to comment #4)
> One issue I couldn't determine either way is whether this should be a -devel
> package or not.

I don't think its a -devel package. Its more like a mysql plugin, which is a runtime, not a devel thing.

 
> SHOULD - All patches should have an upstream bug link or comment
> - Not Ok, No comment provided.

Redid it without patching.

* Fri May  8 2009 Yanko Kaneti <yaneti@declera.com> - 0.9.7-7
- avoid patching and reconfiguring

http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql.spec
http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-7.fc10.src.rpm 


Thanks
Comment 6 Yanko Kaneti 2009-05-08 02:37:16 EDT
It should be noted that the mysql version (5.1.x) in devel/F11 seems to require these extension libs in a specific location %{_libdir}/mysql/plugin/ and does not work if the library is only in the system linker path. This submission remains on the stable branch for now.

* Fri May  8 2009 Yanko Kaneti <yaneti@declera.com> - 0.9.7-8
- use a define for the plugin location, 
  it changes from mysql 5.0 [F10] to 5.1 [F11]

http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql.spec
http://www.declera.com/~yaneti/mysqludf_xql/mysqludf_xql-0.9.7-8.fc10.src.rpm
Comment 7 Lubomir Rintel 2009-05-08 04:21:22 EDT
Yanko: you don't need to call ldconfig; you're not installing a library, just a shared object.

rm -rf $RPM_BUILD_ROOT/%{myplugdir}/*.{a,la}
Why do you call a recursive rm here?
Isn't it possible to avoid build of the static library by passing --disable-static to configure?

Also, please do some informal reviews to prove you're familiar with the packaging guidelines so that your account can get sponsored.
Comment 8 Yanko Kaneti 2009-05-08 08:06:37 EDT
(In reply to comment #7)
> Yanko: you don't need to call ldconfig; you're not installing a library, just a
> shared object.

Empirical evdence doesn't agree, (test rpm without ldconfig):
# service mysqld start
Starting MySQL:                                            [  OK  ]
[root@f10test ~]# rpm -Uhv /home/rpmbuild/rpmbuild/RPMS/i386/mysqludf_xql-0.9.7-8.fc10.i386.rpm
Preparing...                ########################################### [100%]
   1:mysqludf_xql           ########################################### [100%]
[root@f10test ~]# mysql mysql -e 'CREATE FUNCTION lib_mysqludf_xql_info RETURNS STRING SONAME "lib_mysqludf_xql.so";'
ERROR 1126 (HY000) at line 1: Can't open shared library 'lib_mysqludf_xql.so' (errno: 22 lib_mysqludf_xql.so: cannot open shared object file: No such file or directory)
[root@f10test ~]# ldconfig 
[root@f10test ~]# mysql mysql -e 'CREATE FUNCTION lib_mysqludf_xql_info RETURNS STRING SONAME "lib_mysqludf_xql.so";'
[root@f10test ~]# 


> rm -rf $RPM_BUILD_ROOT/%{myplugdir}/*.{a,la}
> Why do you call a recursive rm here?

Just a habit. Doubt it will hurt anything, but the -r  can be removed.

> Isn't it possible to avoid build of the static library by passing
> --disable-static to configure?

not completely sufficient, thus just additional baggage.
error: Installed (but unpackaged) file(s) found:
   /usr/lib/mysql/lib_mysqludf_xql.la
Comment 9 Lubomir Rintel 2009-05-08 16:14:57 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > Yanko: you don't need to call ldconfig; you're not installing a library, just a
> > shared object.
> 
> Empirical evdence doesn't agree, (test rpm without ldconfig):

Sorry, I did not notice it uses ld.so.conf.d.

> > rm -rf $RPM_BUILD_ROOT/%{myplugdir}/*.{a,la}
> > Why do you call a recursive rm here?
> 
> Just a habit. Doubt it will hurt anything, but the -r  can be removed.
> 
> > Isn't it possible to avoid build of the static library by passing
> > --disable-static to configure?
> 
> not completely sufficient, thus just additional baggage.
> error: Installed (but unpackaged) file(s) found:
>    /usr/lib/mysql/lib_mysqludf_xql.la  

1.) .la is not a static library
2.) You seem to have removed the rm line that deletes the .la file for no reason at all
Comment 10 Yanko Kaneti 2009-05-08 16:37:03 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)

> > > Isn't it possible to avoid build of the static library by passing
> > > --disable-static to configure?
> > 
> > not completely sufficient, thus just additional baggage.
> > error: Installed (but unpackaged) file(s) found:
> >    /usr/lib/mysql/lib_mysqludf_xql.la  
> 
> 1.) .la is not a static library
> 2.) You seem to have removed the rm line that deletes the .la file for no
> reason at all  

What I meant was that between having both a --disable-static and a rm line and having just a rm line I'd prefer the latter. Seems like less clutter to me.
Of course a few cycles and disk accesses making the static lib could be spared but imho its hardly worth it. Anyway, if this ever gets to the repo I'll put in the --disable-static.
Comment 12 Lubomir Rintel 2009-05-09 05:28:57 EDT
Thanks. I like the package and am eventually going approve it. You need to get sponsored in order to import and maintain it.

In meanwhile, it would be awesome if you could do a couple of informal reviews similar to what arthur has done here. That's usually done to show that you're familiar with the packaging guidelines and are comfortable with RPM packaging. I'll sponsor you then.
Comment 13 Mamoru TASAKA 2009-05-09 09:03:37 EDT
Well, I still don't understand why ldconfig is needed
here because lib_mysqludf_xql.so does not seem to be
a library.

On F-11, without ldconfig:
------------------------------------------------------------------
[root@localhost ~]# service mysqld start
MySQL を起動中:                                            [  OK  ]
[root@localhost ~]# ls -al /usr/lib/mysql/lib_mysqludf_xql.so 
-rwxr-xr-x 1 root root 20764 2009-05-09 21:38 /usr/lib/mysql/lib_mysqludf_xql.so
[root@localhost ~]# mysql mysql -e 'CREATE FUNCTION lib_mysqludf_xql_info RETURNS STRING SONAME "lib_mysqludf_xql.so";'
ERROR 1126 (HY000) at line 1: Can't open shared library 'lib_mysqludf_xql.so' (errno: 22 /usr/lib/mysql/plugin/lib_mysqludf_xql.so: cannot open shared object file: No such file or directory)
[root@localhost ~]# mv /usr/lib/mysql/lib_mysqludf_xql.so /usr/lib/mysql/plugin/
[root@localhost ~]# mysql mysql -e 'CREATE FUNCTION lib_mysqludf_xql_info RETURNS STRING SONAME "lib_mysqludf_xql.so";'
[root@localhost ~]#
------------------------------------------------------------------

So on F-11, installing lib_mysqludf_xql.so under %_libdir/mysql/plugin
work without ldconfig. Are there any possibility that installing
lib_mysqludf_xql.so should be installed under somewhere else
even on F-10?
Comment 14 Yanko Kaneti 2009-05-09 10:49:49 EDT
(In reply to comment #13)
> Well, I still don't understand why ldconfig is needed

> So on F-11, installing lib_mysqludf_xql.so under %_libdir/mysql/plugin
> work without ldconfig. Are there any possibility that installing
> lib_mysqludf_xql.so should be installed under somewhere else
> even on F-10?  

That is
http://dev.mysql.com/doc/refman/5.0/en/udf-compiling.html
vs
http://dev.mysql.com/doc/refman/5.1/en/udf-compiling.html

I consider the submission fine as is. For F11 myplugdir can become %{_libdir}/mysql/plugin/ and the post and postun can be removed.
Comment 15 arthurguru 2009-05-15 02:46:53 EDT
Hi Yanko,

Just did another informal review of mysqludf_xql.

All went well.

Best regards,
Arthur Gouros.
Comment 16 Mamoru TASAKA 2009-06-03 14:14:44 EDT
Lubomir, if you think there is no blocker for this
review request, please feel free to approve this ticket.

Removing NEEDSPONSOR as I am now sponsoring the submitter.
Comment 17 Lubomir Rintel 2009-06-04 04:16:07 EDT
APPROVED
Comment 18 Yanko Kaneti 2009-06-04 04:37:56 EDT
New Package CVS Request
=======================
Package Name: mysqludf_xql
Short Description: MySQL UDF library for XML output
Owners: yaneti
Branches: F-10 F-11
InitialCC:
Comment 19 Jason Tibbitts 2009-06-04 11:36:25 EDT
CVS done.
Comment 20 Yanko Kaneti 2009-06-04 13:14:50 EDT
Thanks everyone for the review.

Note You need to log in before you can comment on or make changes to this bug.