Page MenuHomeFreeBSD

databases/py-MySQLdb: Fix build with MariaDB 10.2.6
ClosedPublic

Authored by sirl33tname_gmail.com on Jun 5 2017, 8:14 AM.

Details

Summary

Proposed commit message

databases/py-MySQLdb: Fix build with MariaDB 10.2.7

With MariaDB 10.2.6 the Connect/C is updated to ~3.0. This means the C interface changed slightly and some functions are changed. 

 - Replace direct struct access with accessor in _mysql.c
 - Add LICENSE/LICENSE_FILE
 - Strip Binaries  
 - Remove conflict with itself

PR: 219797
Reviewed_by: sunpoet, koobs, brnrd
Submitted by: sirl33tname@gmail.com
Approved by
Differential_Revision: https://reviews.freebsd.org/D11054
Test Plan
  • portlint: OK (looks fine.)
  • testport: (poudriere, 11amd64, 10.2m) OK
  • older mariadb (5.1, 5.5, 5.6, 5.7, 8.0, 5.5m, 10.0m, 10.1m, 5.5p, 5.6p,
  • ports-build 10.3 & 11.0 i386 and amd64 with defaults (MySQL 5,6) OK

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

koobs retitled this revision from databases/py-MySQLdb update to work with MariaDB 10.2.6 to databases/py-MySQLdb: Update to work with MariaDB 10.2.6.Jun 5 2017, 8:31 AM
koobs edited the summary of this revision. (Show Details)
koobs edited the test plan for this revision. (Show Details)
koobs added a subscriber: koobs.Jun 5 2017, 8:33 AM

@sirl33tname_gmail.com Reviews should always take the form of a properly formatted (proposed) commit, and test plans (or QA steps taken) are expected, particularly where relevant QA steps are obvious and known

I've updated the review SUMMARY and TEST PLAN sections to give you an example of how they should be formatted for easy review

If you can provide the results of the QA steps i've added, that would be great.

koobs added a reviewer: Python.Jun 5 2017, 8:35 AM
koobs added a subscriber: brnrd.
sirl33tname_gmail.com edited the test plan for this revision. (Show Details)Jun 5 2017, 8:42 AM
koobs retitled this revision from databases/py-MySQLdb: Update to work with MariaDB 10.2.6 to databases/py-MySQLdb: Fix build with MariaDB 10.2.6.Jun 5 2017, 8:42 AM
koobs edited the summary of this revision. (Show Details)
koobs edited the test plan for this revision. (Show Details)

Tip: Full/verbose QA output is not required, just OK (summary result or comments if available/needed)

koobs edited the test plan for this revision. (Show Details)Jun 5 2017, 8:44 AM
koobs edited the test plan for this revision. (Show Details)

Additional items:

  • Shared library should be stripped
sirl33tname_gmail.com edited the test plan for this revision. (Show Details)Jun 5 2017, 8:49 AM
sirl33tname_gmail.com edited the summary of this revision. (Show Details)

Added the cmd to strip the binary file.

sunpoet added a subscriber: sunpoet.Jun 5 2017, 3:28 PM
sunpoet added inline comments.
Makefile
23–24

It is not mentioned in the log.

BTW, I think it is better to use ${PYTHON_PKGNAMEPREFIX} instead of py* or py??.

45

It is hard-coded to Python 2.7.
Use PYTHON_SITELIBDIR instead for future compatibility (when Python 3.x support is ready someday).

files/patch-_mysql.c
7

Is it necessary?
I guess it is not because there is MYSQL_VERSION_ID check in line 15 which means MYSQL_VERSION_ID is already defined.

sirl33tname_gmail.com marked 2 inline comments as done.Jun 5 2017, 4:27 PM
sirl33tname_gmail.com added inline comments.
files/patch-_mysql.c
7

I'm almost sure it's needed because that's the only place where MYSQL_VERSION_ID is defined.

sirl33tname_gmail.com edited the summary of this revision. (Show Details)

incorporated changes recommended by @sunpoet

sunpoet added inline comments.Jun 5 2017, 5:31 PM
files/patch-_mysql.c
7

See line 15.

+#if MYSQL_VERSION_ID >= 50500

It (MYSQL_VERSION_ID) is already there before this patch.
If it compiled without error, you do not need to add #include "mysql_version.h" again.
It might be included somewhere in the source.

files/patch-_mysql.c
7

Ah I see what you mean. But if I don't include it it fails to build because these if statement don't work. I think maybe with the interfaces changes it's not included by default.

As fare as I understand the code mysql_version.h was included in mysql.h before, but since the changed that it is including mariadb_version.h which only has MARIADB_VERSION_ID. And this would probably then not be compatible to the mysql implementation.

So in conclusion it is needed for mariadb 10.2 and should work fine on for ever other version since it even has include guards.

sirl33tname_gmail.com marked 4 inline comments as done.Jun 6 2017, 4:38 PM
sirl33tname_gmail.com added inline comments.
files/patch-_mysql.c
7

@sunpoet Yeah I guess I was right see the patch for perl: https://bz-attachments.freebsd.org/attachment.cgi?id=183259

++#include <mysql_version.h> /* MariaDB does not include this in mysql.h */

So I will mark that as solved.

sirl33tname_gmail.com marked an inline comment as done.Jun 6 2017, 4:38 PM
brnrd added a comment.Jun 10 2017, 7:25 PM

https://jira.mariadb.org/browse/MDEV-12950
https://github.com/PyMySQL/mysqlclient-python/pull/177/commits/d663649f851794dffa84010db6290e693d4baab8

mysql_options was added in MySQL 5.0 so we can safely replace the struct access with the method. According to the MariaDB devs, mysql_options is not going away (ever)... No need to use mysql_optionsv.

files/patch-_mysql.c
7

The include is fixed in mariadb102-client-10.2.6_2

update according to @brnrd recommendations

brnrd edited the summary of this revision. (Show Details)Jul 23 2017, 6:56 PM
brnrd edited the test plan for this revision. (Show Details)
brnrd edited the summary of this revision. (Show Details)Jul 23 2017, 6:59 PM
koobs accepted this revision.Jul 24 2017, 1:24 AM

Appears syntactically fine.

Someone familiar/confident with the MySQL/MariaDB/* semantics ought also accept the change prior to commit

If quarterly ports are affected, commit log message is missing MFH: 2017Q3

This revision is now accepted and ready to land.Jul 24 2017, 1:24 AM
This revision now requires review to proceed.Jul 24 2017, 1:25 AM
koobs edited the test plan for this revision. (Show Details)Jul 24 2017, 1:25 AM
brnrd accepted this revision.Jul 24 2017, 9:43 AM

All OK.
Committing!

This revision is now accepted and ready to land.Jul 24 2017, 9:43 AM
This revision was automatically updated to reflect the committed changes.