Eoan FTBFS with gcc-9 and MySQL 8

[Rafael David Tinoco]

C and C++ differ in the form of types being defined. While C++ structs are defined also as new types, in C you have to explicitly typedef the struct to have a new type. From clickhouse source code, we are getting:

https://launchpadlibrarian.net/429654087/buildlog_ubuntu-eoan-amd64.clickhouse_18.16.1+ds-4build1_BUILDING.txt.gz

----

[ 6%] Building CXX object libs/libmysqlxx/CMakeFiles/mysqlxx.dir/src/Connection.cpp.o
cd /<<BUILDDIR>>/clickhouse-18.16.1+ds/obj-x86_64-linux-gnu/libs/libmysqlxx && /usr/bin/c++ -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libcommon/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/obj-x86_64-linux-gnu/libs/libcommon/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/contrib/cityhash102/include -g -O2 -fdebug-prefix-map=/<<BUILDDIR>>/clickhouse-18.16.1+ds=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -pipe -fno-omit-frame-pointer -Wall -Wnon-virtual-dtor -O2 -g -DNDEBUG -O3 -flto -fno-fat-lto-objects -fPIC -std=c++17 -o CMakeFiles/mysqlxx.dir/src/Connection.cpp.o -c /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp
In file included from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/ResultBase.h:4,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/UseQueryResult.h:3,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Query.h:6,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Connection.h:10,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp:7:
/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Types.h:7:23: error: conflicting declaration ‘using MYSQL = struct st_mysql’
 using MYSQL = st_mysql;
                       ^
In file included from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp:4:
/usr/include/mysql/mysql.h:335:3: note: previous declaration as ‘typedef struct MYSQL MYSQL’
 } MYSQL;

----

If I edit file libs/libmysqlxx/include/mysqlxx/Types.h and comment the following lines:

//struct st_mysql;
//using MYSQL = st_mysql;
//
//struct st_mysql_res;
//using MYSQL_RES = st_mysql_res;
//
//using MYSQL_ROW = char**;
//
//struct st_mysql_field;
//using MYSQL_FIELD = st_mysql_field;

and add

#include <mysql/mysql.h>

I solve the issue, as MYSQL, MYSQL_RES, MYSQL_ROW and MYSQL_FIELD types are defined by the mysql.h header file (as C++ would expect).

----

Unfortunately doing that with gcc-9 was not successful not because of the change, per se, but gcc-9 crashed during clickhouse compilation. When changing gcc-9 to gcc-8 as the default compiler (Ubuntu Eoan userland) I was able to compile clickhouse libmysqlxx library successfully.

[Rafael David Tinoco (rafaeldtinoco)]

I have opened the following issue upstream:

https://github.com/yandex/ClickHouse/issues/6552

Explaining what was discovered and asking what would be the best alternative to address the issue (particularly interested why we were not facing this before).

[Rafael David Tinoco (rafaeldtinoco)]

I opened the following upstream issue for GCC/G++ 9:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91493

and following upstream issue for ClickHouse to track the G++ issue:

https://github.com/yandex/ClickHouse/issues/6560

I already have patches to either fix (issue #1) or mitigate (issue #2) and will propose upstream soon (testing local builds to make sure everything is right).

[Rafael David Tinoco (rafaeldtinoco)]

@gianfranco:

https://github.com/yandex/ClickHouse/issues/6552

Based on the upstream discussion, I think upstream change should be:

----

From cc17b8f0687d0b290e7a543032a87ca289290563 Mon Sep 17 00:00:00 2001
From: Rafael David Tinoco <email address hidden>
Date: Tue, 20 Aug 2019 02:38:47 +0000
Subject: [PATCH 2/2] MySQL 8 integration requires previous declaration checks

C and C++ differ in the form of types being defined. While C++ structs
are defined also as new types, in C you have to explicitly typedef the
struct to have a new type.

Fir this case, it was enough to check if MySQL header was already
defined in order not to re-declare MYSQL, MYSQL_RES, MYSQL_ROW and
MYSQL_FIELD.

Signed-off-by: Rafael David Tinoco <email address hidden>
---
 libs/libmysqlxx/include/mysqlxx/Types.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libs/libmysqlxx/include/mysqlxx/Types.h b/libs/libmysqlxx/include/mysqlxx/Types.h
index 30abdeb..b5ed709 100644
--- a/libs/libmysqlxx/include/mysqlxx/Types.h
+++ b/libs/libmysqlxx/include/mysqlxx/Types.h
@@ -3,6 +3,8 @@
 #include <cstdint>
 #include <string>

+#ifndef _mysql_h
+
 struct st_mysql;
 using MYSQL = st_mysql;

@@ -14,7 +16,7 @@ using MYSQL_ROW = char**;
 struct st_mysql_field;
 using MYSQL_FIELD = st_mysql_field;

-
+#endif

 namespace mysqlxx
 {
--
2.20.1

----

instead. Im proposing that to upstream, just so you are aware for the next merge!

[Rafael David Tinoco (rafaeldtinoco)]

Im attaching 2 patches to the case:

- Fixing MySQL building issue (proposed upstream)
- Mitigating GCC 9 issue (reported upstream)

Package maintainer already mitigated both with different patches.

clickhouse (18.16.1+ds-4ubuntu1) eoan; urgency=medium

  * Use gcc-8 to build: see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91500
  * Patch to make it build with mysql

 -- Gianfranco Costamagna <email address hidden> Tue, 20 Aug 2019 13:39:54 +0200

Forcing GCC 8 instead and including mysql.h to a C++ header. These 2 patches could be considered now or in a near future merge, when upstream issues are addressed.

[Rafael David Tinoco (rafaeldtinoco)]

Migration failed due to autopkgtests:

https://launchpadlibrarian.net/438156898/buildlog_ubuntu-eoan-amd64.clickhouse_18.16.1+ds-4ubuntu1_BUILDING.txt.gz

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "0001-G-9-crashes-returning-exception-using-true-false-syn.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[Rafael David Tinoco (rafaeldtinoco)]

Just for the record, it wasn't the migration, it was the build tests at the end. And it looks like the deviation from expected results was regarding number formatting other than different results:

00534_exp10: FAIL - result differs with reference:

--- /<<BUILDDIR>>/clickhouse-18.16.1+ds/dbms/tests/queries/0_stateless/00534_exp10.reference 2018-12-20 16:38:50.000000000 +0000
+++ /tmp/clickhouse.test..aS0HS/tmp/0_stateless/00534_exp10.stdout 2019-08-20 12:50:21.402976662 +0000
@@ -21,7 +21,7 @@
 20 100000000000000000000
 21 1e21
 22 1e22
-23 1e23
+23 1.0000000000000001e23
 24 1e24
 25 1e25
 26 1e26
@@ -208,7 +208,7 @@
 207 1e207
 208 1e208
 209 1e209
-210 1e210
+210 1.0000000000000001e210
 211 1e211
 212 1e212
 213 1e213

I'm testing this localy AND the upstream change:

https://github.com/yandex/ClickHouse/pull/6569

Failed in a test that is being skipped by our builds (00704_drop_truncate_memory_table).

[Rafael David Tinoco (rafaeldtinoco)]

gcc-9 issue likely related to:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91493
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91500

[Rafael David Tinoco (rafaeldtinoco)]

Fix for ": ?" errors for gcc-9:

https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=274550
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=274597

[Rafael David Tinoco (rafaeldtinoco)]

Alright, the compilation error in comment #9 is related to:

libs/libcommon/include/common/preciseExp10.h:

/** exp10 from GNU libm fails to give precise result for integer arguments.
  * For example, exp10(3) gives 1000.0000000000001
  * despite the fact that 1000 is exactly representable in double and float.
  * Better to always use implementation from MUSL.
  *
  * Note: the function names are different to avoid confusion with symbols from the system libm.
  */

They prefer using libm from MUSL, and they even have a check to check exp() precision. I guess I'll have to blacklist that test for our builds.

[Rafael David Tinoco (rafaeldtinoco)]

@Gian,

As clickhouse is blocking the full MySQL 8 update, and we have only 1 day until the freeze, I'll update clickhouse with the accepted upstream patch:

https://github.com/yandex/ClickHouse/issues/6552
https://github.com/yandex/ClickHouse/pull/6569

instead of yours mysql.h patch, AND we *have* to use gcc-9, so I'll remove your change to defaulting to g++8 and apply the mitigation for the g++-9 bug (I'll maintain this as delta, from Debian, until, and if, you decide to have such change there).

For the failing build (from your previous upload):
https://launchpadlibrarian.net/438156898/buildlog_ubuntu-eoan-amd64.clickhouse_18.16.1+ds-4ubuntu1_BUILDING.txt.gz

I'll try to disable that test, as we are NOT using MUSL, and they already know the issue regarding glibc libm precision for exp().

Let me know if you have any comments or concerns, please.

Best,

Rafael

[Rafael David Tinoco (rafaeldtinoco)]

Looks like that patch:

0008-Replace-cpuid-with-cpuinfo.patch

Introduced a stderr in ctests:

11:
11: Running stateless tests.
11:
11: 00802_daylight_saving_time_shift_backwards_at_midnight: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:
11: 00802_system_parts_with_datetime_partition: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:
11: 00801_daylight_saving_time_hour_underflow: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:

And tests start to fail in:

test 11
      Start 11: with_server

[Rafael David Tinoco (rafaeldtinoco)]

From cpuinfo library:

#if defined(__ANDROID__) && !defined(CPU_SETSIZE)
    /*
     * Android NDK headers before platform 21 do not define CPU_SETSIZE,
     * so we hard-code its value, as defined in platform 21 headers
     */
    #if defined(__LP64__)
        static const uint32_t default_max_processors_count = 1024;
    #else
        static const uint32_t default_max_processors_count = 32;
    #endif
#else
    static const uint32_t default_max_processors_count = CPU_SETSIZE;
#endif

AND

#if !defined(__ANDROID__)
    /*
     * sched.h is only used for CPU_SETSIZE constant.
     * Android NDK headers before platform 21 do have this constant in sched.h
     */
    #include <sched.h>
#endif

and resolving includes, it gets CPU_SETSIZE (_CPU_SETSIZE) from:

x86_64-linux-gnu/bits/cpu-set.h:

/* Size definition for CPU sets. */
#define __CPU_SETSIZE 1024
#define __NCPUBITS (8 * sizeof (__cpu_mask))

And that leads to:

uint32_t cpuinfo_linux_get_max_processors_count(void) {
...
    cpuinfo_log_warning("using platform-default max processors count = %"PRIu32, default_max_processors_count);
...
}

[Rafael David Tinoco (rafaeldtinoco)]

I have opened the following BUG:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1840847

To address this issue, for now.. I might have to drop the specific patch:

----
From: Alexander GQ Gerasiov <email address hidden>
Date: Fri, 4 Jan 2019 03:04:07 +0300
Subject: Replace cpuid with cpuinfo.

cpuinfo is already shipped in Debian and provides more suitable API.
This commit is not enough for inclusion into upstream codebase. One also
needs to replace cpuid with cpuinfo in the "contrib" directory.

Signed-off-by: Alexander GQ Gerasiov <email address hidden>
----

That introduced this issue.

[Rafael David Tinoco (rafaeldtinoco)]

Im currently building the to-be-suggested source package locally so I can push it to a PPA for the debdiff reviewers before final upload.

[Rafael David Tinoco (rafaeldtinoco)]

Instead of reverting the libcpuid change (done in Debian and not upstream) I will make libcpuid not to stderr for warnings for now:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1840847

And this might fix comment #14 issue.

[Rafael David Tinoco (rafaeldtinoco)]

Fixing libcpuid I was able to run all tests and, apart from those already marked as buggy and/or flaky, in debian/rules, I'm adding those extra for now:

#
# LP: #1840511 cleanup
#

# libm precision on exp() related functions
SKIP_TESTS+=00536 00534

# long_http_bufferization ?
SKIP_TESTS+=00429

# system processes port ? (bind: error already in use)
SKIP_TESTS+=00379

# clickhouse-clang not found
SKIP_TESTS+=00281

# end of LP: #1840511 cleanup

They don't seem to be indicating real issues with this build of clickhouse.

[Rafael David Tinoco (rafaeldtinoco)]

I guess if tests make the build hard to happen we can just enable:

 #export DEB_BUILD_OPTIONS+=nocheck

in debian/rules but, for now, I tried to avoid that.

[Rafael David Tinoco (rafaeldtinoco)]

PPA containing clickhouse build and cpuinfo, with needed fix from (LP: #1840847):

https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1840511

Debdiff with proposed changes are attached to this bug.

[Rafael David Tinoco (rafaeldtinoco)]

Well it looks like clickhouse is an amd64 only package, I have added more architectures in the PPA, please ignore those building errors.

[Gianfranco Costamagna (costamagnagianfranco)]

I uploaded after changing some little bit to make the changes upstreamable to Debian