maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Otto!

On Aug 08, Otto Kekäläinen wrote:
> 2014-07-28 0:27 GMT+03:00 Otto Kekäläinen <otto@xxxxxxxxx>:
> > Eventually. It will take weeks, so I just wanted to notify you that
> > this will be the status until then.
> 
> I've now commented all your comments and fixed most of the issues. The
> issues marked TODO I'll do later. I haven't done a new merge request
> though, I won't do that all your comments are fully addressed.
>
> The file with inline comments is attached. I'll be on IRC today if you
> want to further discuss something, e.g. cleaning up cmake arguments
> etc.
> 
> Note that there is also the option for you to add comments to my
> commits at Github
> (https://github.com/ottok/mariadb-5.5/commits/master) to avoid sending
> the same 95kb patch file back and forth.

Thanks, I'll use that.

Below I only comment on your replies, and will use github for commenting
on the patches.

Did you get any answers from the list on the questions you've sent
there?

> > === modified file 'debian/mariadb-client-5.5.menu'
> > --- debian/mariadb-client-5.5.menu	2012-01-23 11:20:16 +0000
> > +++ debian/mariadb-client-5.5.menu	2014-07-24 08:02:44 +0000
> > @@ -1,3 +1,3 @@
> >  # According to /usr/share/menu/ policy 1.4, not /usr/share/doc/debian-policy/
> > -?package(innotop):needs="text" section="Applications/Data Management"\
> > +?package(mariadb-client-5.5):needs="text" section="Applications/Data Management"\
> >    title="innotop" command="/usr/bin/innotop"
> 
> Why would there be a menu with one single entry for a third-party tool innotop?
> I'd expect a menu with most of the client tools. Or no menu at all.
> 
> Otto: menu file was introduced from upstream MariaDB, I only fixed
> format error. I think that all tools that have a GUI should have a
> menu entry, others not.

It only has text gui, as far as as can see.
So only mysql CLI should be here too, but other tools should not be.

> > === added file 'debian/mariadb-server-core-5.5.lintian-overrides'
> > --- debian/mariadb-server-core-5.5.lintian-overrides	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-server-core-5.5.lintian-overrides	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,4 @@
> > +# Embedded with same source OK
> > +mariadb-server-core-5.5: embedded-library usr/sbin/mysqld: libmysqlclient
> 
> Hmm? Why?
> 
> Otto: This the package where libmysqld.so is at the moment, issue
> https://mariadb.atlassian.net/browse/MDEV-5725 tracks possible
> re-location of it.

Okay, let's take libmysqld out of the scope of this task. No more
comments from me about libmysqld then.

> > === added file 'debian/mariadb-client-core-5.5.lintian-overrides'
> > --- debian/mariadb-client-core-5.5.lintian-overrides	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-client-core-5.5.lintian-overrides	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,5 @@
> > +# these libs are OK
> > +mariadb-client-core-5.5: embedded-library usr/bin/mysql: libmysqlclient
> > +mariadb-client-core-5.5: embedded-library usr/bin/mysqlcheck: libmysqlclient
> 
> I'd rather not embed libmysqlclient in clients instead
> 
> Otto: This is how sources upstream sources are, only adding the
> lintian override is a part of packaging. What is your suggestion to
> fix this? Is something in cmake rules generating this?

Yes. /usr/bin/mysql and /usr/bin/mysqlcheck link with libmysqlclient.a,
that's why you have this override. I think they should link with
libmysqlclient.so instead. Yes, in cmake rules, I can fix that.

> > === added file 'debian/mariadb-server-5.5.dirs'
> > --- debian/mariadb-server-5.5.dirs	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-server-5.5.dirs	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,7 @@
> > +etc/init.d
> > +etc/logrotate.d
> > +etc/mysql/conf.d
> > +usr/bin
> > +usr/share/mysql
> > +usr/share/doc/mariadb-server-5.5
> > +var/lib/mysql-upgrade
> 
> Really? Why do you need all these directories in dirs file?
> the last one is only used internally in the postinst script, it
> should not leak out. Others should be created normally because
> they contain files that this package installs.
> 
> Otto: apart from usr/share/doc/mariadb-server-5.5 these definitions
> are the same as in mysql-5.6 packaging.

Yes, but it doesn't answer the question why they're needed.
One easy way to find out would be to remove them and compare the
resulting packages. Or ask on the list.

> > === added file 'debian/mariadb-test-5.5.lintian-overrides'
> > --- debian/mariadb-test-5.5.lintian-overrides	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-test-5.5.lintian-overrides	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,9 @@
> > +# in modern Debian this xz support exists, this should not matter anymore
> > +mariadb-test-5.5: data.tar.xz-member-without-dpkg-pre-depends
> > +# Embedded from same source is OK
> > +mariadb-test-5.5: embedded-library usr/bin/mysql_client_test: libmysqlclient
> > +mariadb-test-5.5: embedded-library usr/bin/mysql_client_test_embedded: libmysqlclient
> > +mariadb-test-5.5: embedded-library usr/bin/mysqltest: libmysqlclient
> > +mariadb-test-5.5: embedded-library usr/bin/mysqltest_embedded: libmysqlclient
> > +# OK, file part of test suite
> > +mariadb-test-5.5: arch-dependent-file-in-usr-share usr/share/mysql/mysql-test/lib/My/SafeProcess/my_safe_process
> 
> And where should it be installed instead?
> 
> Otto: in /usr/bin/ but it's not relevant here in the test package. The
> override is OK. Lintian does sometimes detect false positives..

Sure, I only mean where lintian wants it to be installed, and you've
answered that. Thanks.

> > === modified file 'debian/source.lintian-overrides'
> > --- debian/source.lintian-overrides	2012-01-28 20:22:14 +0000
> > +++ debian/source.lintian-overrides	2014-07-24 08:02:44 +0000
> > @@ -1,2 +1,12 @@
> > +# Autotools is not used, these files are just old cruft from upstream
> > +outdated-autotools-helper-file extra/jemalloc/config.guess 2012-02-10
> > +outdated-autotools-helper-file extra/jemalloc/config.sub 2012-02-10
> > +outdated-autotools-helper-file storage/tokudb/ft-index/third_party/xz-4.999.9beta/build-aux/config.guess 2009-04-27
> > +outdated-autotools-helper-file storage/tokudb/ft-index/third_party/xz-4.999.9beta/build-aux/config.sub 2009-04-17
> 
> we can, technically, fix that, no need to override
> 
> Otto: yes, then please do it :) I removed the override for outdated
> autotools helper file now.

jemalloc is now gone. I've reported xz issue to tokutek, hopefully
they'll fix it. If not - I do, but then we might have a merge conflict
later.

> > === modified file 'debian/mariadb-server-5.5.lintian-overrides'
> > --- debian/mariadb-server-5.5.lintian-overrides	2012-01-28 20:22:14 +0000
> > +++ debian/mariadb-server-5.5.lintian-overrides	2014-07-24 08:02:44 +0000
> > @@ -1,5 +1,10 @@
> > -mariadb-server-5.5: command-with-path-in-maintainer-script postinst
> > -mariadb-server-5.5: possible-bashism-in-maintainer-script postinst:81 'p{("a".."z","A".."Z",0..9)[int(rand(62))]}'
> > -mariadb-server-5.5: possible-bashism-in-maintainer-script preinst:33 '${cmd/ */}'
> > -mariadb-server-5.5: statically-linked-binary ./usr/bin/mysql_tzinfo_to_sql
> > -mariadb-server-5.5: statically-linked-binary ./usr/sbin/mysqld
> > +# Embedded from same source is OK
> > +mariadb-server-5.5: embedded-library usr/bin/mysqlbinlog: libmysqlclient
> > +# OK in newer Debian, includes support for xz
> > +mariadb-server-5.5: data.tar.xz-member-without-dpkg-pre-depends
> > +# ash's buildin has no "-e" so use /bin/echo
> > +mariadb-server-5.5: command-with-path-in-maintainer-script postinst:194 /bin/echo
> > +mariadb-server-5.5: command-with-path-in-maintainer-script postinst:208 /bin/echo
> > +mariadb-server-5.5: command-with-path-in-maintainer-script postinst:216 /bin/echo
> 
> I think this can be fixed too.
> 
> Otto: The comment above explains why it cannot be fixed for now (usage
> of '-e'). Unless of course the whole variable echoing thing is
> replaced with some other code.

yes, I mean, it's easy to change the script to avoid backslash escapes.
Then it could use built-in echo in ash.

Just replace \n with ; (unless \n already follows ; - in that case
simply remove \n).

> > === added file 'debian/mariadb-client-5.5.manpages'
> > --- debian/mariadb-client-5.5.manpages	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-client-5.5.manpages	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,16 @@
> > +debian/additions/innotop/innotop.1
> > +debian/tmp/usr/share/man/man1/mysqlaccess.1
> > +debian/tmp/usr/share/man/man1/mysqladmin.1
> > +debian/tmp/usr/share/man/man1/mysqlbug.1
> > +debian/tmp/usr/share/man/man1/mysqldump.1
> > +debian/tmp/usr/share/man/man1/mysqldumpslow.1
> > +debian/tmp/usr/share/man/man1/mysql_find_rows.1
> > +debian/tmp/usr/share/man/man1/mysql_fix_extensions.1
> > +debian/tmp/usr/share/man/man1/mysqlimport.1
> > +debian/tmp/usr/share/man/man1/mysqlman.1
> > +debian/additions/mysqlreport.1
> > +debian/tmp/usr/share/man/man1/mysqlshow.1
> > +debian/tmp/usr/share/man/man1/mysqlslap.1
> > +debian/additions/mysql_tableinfo.1
> 
> should be removed (and other dead stuff)
> 
> Otto: How do I know what is dead stuff?

I've fixed all that some time ago on your request. All dead manual pages
are now gone. And you can simply remove lines for files that don't exist
in 5.5. mysqlman.1 and mysql_tableinfo.1 are surely dead. Others seem to
be present.

> > +debian/tmp/usr/share/man/man1/mysql_waitpid.1
> > +debian/tmp/usr/share/man/man8/mysqlmanager.8

Yes, and mysqlmanager.8 is dead too.

> > === modified file 'debian/mariadb-server-5.5.preinst'
> > --- debian/mariadb-server-5.5.preinst	2012-01-31 07:57:59 +0000
> > +++ debian/mariadb-server-5.5.preinst	2014-07-24 08:02:44 +0000
> > @@ -64,22 +78,22 @@
> >  show_downgrade_warning=0
> >  for i in `ls $DATADIR/debian-*.flag 2>/dev/null`; do
> >    found_version=`echo $i | sed 's/.*debian-\([0-9\.]\+\).flag/\1/'`
> > -  if dpkg --compare-versions "$this_version" '<<' "$found_version"; then
> > +  if dpkg --compare-versions "5.5" '<<' "$found_version"; then
> 
> why did you remove $this_version ?
> 
> Otto: I'd rather be specific and verbose. $this_version can be hard-coded because it goes hand-in-hand with the source changes anyway.

Right, it is hard-coded. Few lines above there is

  this_version=5.5

But it's hard-coded in one place, so in the next version only that one
line needs to be changed. I like it that way, while after your patch one
needs to search-and-replace all "5.5" strings. It's error-prone.

> > === added file 'debian/mariadb-server-5.5.install'
> > --- debian/mariadb-server-5.5.install	1970-01-01 00:00:00 +0000
> > +++ debian/mariadb-server-5.5.install	2014-07-24 08:02:44 +0000
> > @@ -0,0 +1,72 @@
> > +usr/lib/mysql/plugin/sphinx.so
> > +usr/lib/mysql/plugin/sql_errlog.so
> > +usr/share/doc/mariadb-server-5.5/INFO_BIN
> > +usr/share/doc/mariadb-server-5.5/INFO_SRC
> > +usr/share/doc/mariadb-server-5.5/mysqld.sym.gz
> 
> eh? you delete mysqld.sym.gz in the clean file, don't you?
> 
> Otto: The end result is still that mysqld.sym.gz ends up in the mariadb-server-5.5 package.
> 
> What are these files anyway? They are not defined in the mysql-5.5|5.6 Debian packages
> usr/share/doc/mariadb-server-5.5/INFO_BIN
> usr/share/doc/mariadb-server-5.5/INFO_SRC
> usr/share/doc/mariadb-server-5.5/mysqld.sym.gz

You can drop INFO_SRC and INFO_BIN (I've recently removed them from our
current packages). As for mysqld.sym.gz... debian builds strip binaries,
don't they? mysqld.sym.gz was supposed to have all symbols to allow
resolving stack traces in stripped binaries. But it is 15-year-old 3.23.x technology,
I don't know whether it still works in 5.5.

> > +ifneq (,$(filter $(ARCH), i386 kfreebsd-i386 hurd-i386))
> > +    TAOCRYPT_OPT="-DTAOCRYPT_DISABLE_X86ASM"
> > +endif
> 
> This belongs in CMakeLists.txt (or configure.cmake)
> or in extra/yassl/taocrypt/include/misc.hpp
> 
> Otto: This is directly inherited from rules in mysql-5.5|5.6. I assume
> the reason is that ARCH is available only in the rules file. CMake
> does not know what the target ARCH is in cross-compilation situations
> (which happen all the time on Debian infrastructure).

CMake must know about the target architecture. If it does not, it is
either a bug in our cmakefiles or you're using it incorrectly.

How do you do cross-compilation with CMake?

> If you want, this can be skipped from upstream rules file. I assume
> all upstream builds are always in KVM virtual machines (and no
> cross-compiling is done).

I'd prefer to fix our cmake files to work properly when cross-compiling.

> >  # This causes seg11 crashes if LDAP is used for groups in /etc/nsswitch.conf
> >  # so it is disabled by default although, according to MySQL, it brings >10%
> >  # performance gain if enabled. See #299382.
> >  ifeq ($(STATIC_MYSQLD), 1)
> > -    USE_STATIC_MYSQLD=--with-mysqld-ldflags=-all-static
> > +    USE_STATIC_MYSQLD:=--with-mysqld-ldflags=-all-static
> >  endif
> 
> Did you actually try it? If enabled, it'd pass
> --with-mysqld-ldflags=-all-static
> to cmake, but it's not a valid cmake argument, it'd fail immediately.
> This can never work.
> 
> Otto: This is directly inherited from rules in mysql-5.5|5.6. How do
> you know it will fail? Issue
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=299382 repeats the
> same syntax.

That debian issue is for mysql-4.1. Before MySQL-5.5 we have used
autotools. And our ./configure script indeed supported
--with-mysqld-ldflags=-all-static option.

But since 5.5 we use CMake. It only accepts flags in the form of -DXXX=YYY.
So if you ever define STATIC_MYSQLD as above, MariaDB will simply fail
to compile. CMake will error out at once.

MySQL debian rules only work as long as nobody cares to define
STATIC_MYSQLD.

> > -	cd $(builddir) && $(MAKE) $(MAKE_J) $(AM_EXTRA_MAKEFLAGS)
> > +	cd $(builddir) && $(MAKE) $(AM_EXTRA_MAKEFLAGS)
> > +	touch $@
> 
> I don't see any point in overwriting dh_auto_build, you don't add anything
> valueable to make command line. You've removed -j, and did not
> add support for DEB_BUILD_OPTIONS=parallel. AM_EXTRA_MAKEFLAGS
> isn't needed here, it can be passed to cmake or set in MAKEFLAGS.
> 
> So, I think that
> * dh_auto_build override shold be removed
> * support for DEB_BUILD_OPTIONS=parallel should be added (via MAKEFLAGS)
> * AM_EXTRA_MAKEFLAGS should go away, instead CMAKE_VERBOSE_MAKEFILE should
> be set to 1 if DH_VERBOSE is set (or, may be, if DEB_BUILD_OPTIONS=verbose)
> 
> Otto: OK, done. Just in case, please review commit
> https://github.com/ottok/mariadb-5.5/commit/ff3f1b0116be219c95471eb82cee84eaf3ec6c17.
> Related is also
> https://github.com/ottok/mariadb-5.5/commit/21c251a852b2bcbafa8b9e0049be0471f5bdca3e
> 
> Googling for CMAKE_VERBOSE_MAKEFILE documentation didn't show much but
> said to use option VERBOSE=1

Here:
http://www.cmake.org/cmake/help/v2.8.12/cmake.html#variable:CMAKE_VERBOSE_MAKEFILE
"
  This variable defaults to false. You can set this variable to true to
  make CMake produce verbose makefiles that show each command line as it
  is used.
"

You can pass VERBOSE=1 to make or -DCMAKE_VERBOSE_MAKEFILE=1 to cmake.
Either way is ok.

> The end result in logs with parallel building specified (DEB_BUILD_OPTIONS="parallel=4") looks like this:
> 
> 	sh -c  'PATH=${MYSQL_BUILD_PATH:-"/usr/local/bin:/usr/bin:/bin"} \
> 	    	CC=${MYSQL_BUILD_CC:-gcc} \
> 		CFLAGS=${MYSQL_BUILD_CFLAGS:-"-O2 -DBIG_JOINS=1 -fno-strict-aliasing "} \
> 	    	CXX=${MYSQL_BUILD_CXX:-g++} \
> 	    	CXXFLAGS=${MYSQL_BUILD_CXXFLAGS:-"-O3 -DBIG_JOINS=1 -felide-constructors -fno-exceptions -fno-rtti -fno-strict-aliasing "} \
> 	    cmake -DCMAKE_INSTALL_PREFIX=/usr \
> 	     \
> 	    -w --jobserver-fds=3,4 -j \
> 	    -DWITHOUT_TOKUDB=true \
> 	    -DCOMPILATION_COMMENT="(Ubuntu)" \
> 	    -DMYSQL_SERVER_SUFFIX="-2" \
> 	    -DSYSTEM_TYPE="debian-linux-gnu" \
> 	    -DBUILD_CONFIG=mysql_release \
> 	    -DINSTALL_LIBDIR=lib/x86_64-linux-gnu \
> 	    -DINSTALL_PLUGINDIR=lib/mysql/plugin \
> 		-DDEB=1 ..'
> 
> As -j is empty and now equal to processor count, I assume something is
> still bad. Also note that with MAKEFLAGS there is some extra payload
> '-w --jobserver-fds=3,4'.

Yes, -j is for make, not cmake. cmake does not understand -j.
And I don't know what --jobserver is, neither make nor cmake understand
it.

Besides, I'd avoid using "make -j" where -j does not have a number.
In that case make will start as many build jobs as it wants, with no
limit. On my laptop it immediately overloads the CPU, the system is
practically brought to a halt, and in 10-20 seconds the OOM killer
starts killing around, in another minute my ^C comes through and stops
the madness (yeah, a couple of times when I was typing "make -j5" I
pressed <Enter> too early)

> > +	# If TokuDB plugin was built
> > +	# add it to the server install list.
> > +	[ ! -f $(BUILDDIR)/usr/lib/mysql/plugin/ha_tokudb.so ] || echo 'usr/lib/mysql/plugin/ha_tokudb.so\netc/mysql/conf.d/tokudb.cnf\nusr/bin/tokuftdump\nusr/share/doc/mariadb-server-5.5/README-TOKUDB\nusr/share/doc/mariadb-server-5.5/README.md' >> debian/mariadb-server-5.5.install
> 
> Bad idea, you should never modify version controlled files from scripts.
> Instead, there should be debian/mariadb-server-5.5.install.in file
> and you copy it to debian/mariadb-server-5.5.install, appending whatever you
> want. That would be a hack that works. A proper solution would be to let
> cmake to create debian/mariadb-server-5.5.install from the *.in file
> because cmake knows exactly whether tokudb is built and what files
> needs to be installed. That's what the old code did.
> 
> Otto: The old code didn't work on Debian/Ubuntu build systems with
> chroots etc. Also from Debian point of view maintaining an install.in
> file that is modified on runtime by external tool (cmake) was
> difficult, as all tools expect to find a consistent .install (or
> .install.in) file.

But you're doing exactly that - modifying the install file.
Why is it better to modify it with echo than with cmake?

> > +    # update privilege tables
> > +    password_column_fix_query=`/bin/echo -e \
> > +        "USE mysql\n" \
> > +        "ALTER TABLE user CHANGE Password Password char(41) character set latin1 collate latin1_bin DEFAULT '' NOT NULL"`;
> > +    replace_query=`/bin/echo -e \
> > +        "USE mysql\n" \
> > +        "SET sql_mode='';\n" \
> > +        "REPLACE INTO user SET " \
> > +        "  host='localhost', user='debian-sys-maint', password=password('$pass'), " \
> > +        "  Select_priv='Y', Insert_priv='Y', Update_priv='Y', Delete_priv='Y', " \
> > +        "  Create_priv='Y', Drop_priv='Y', Reload_priv='Y', Shutdown_priv='Y', " \
> > +        "  Process_priv='Y',  File_priv='Y', Grant_priv='Y', References_priv='Y', " \
> > +        "  Index_priv='Y', Alter_priv='Y', Super_priv='Y', Show_db_priv='Y', "\
> > +        "  Create_tmp_table_priv='Y', Lock_tables_priv='Y', Execute_priv='Y', "\
> > +        "  Repl_slave_priv='Y', Repl_client_priv='Y', Create_view_priv='Y', "\
> > +        "  Show_view_priv='Y', Create_routine_priv='Y', Alter_routine_priv='Y', "\
> > +        "  Create_user_priv='Y', Event_priv='Y', Trigger_priv='Y',"\
> > +        "  ssl_cipher='', x509_issuer='', x509_subject='';"`;
> > +    # Engines supported by etch should be installed per default. The query sequence is supposed
> > +    # to be aborted if the CREATE TABLE fails due to an already existent table in which case the
> > +    # admin might already have chosen to remove one or more plugins. Newlines are necessary.
> > +    install_plugins=`/bin/echo -e \
> > +        "USE mysql;\n" \
> > +        "CREATE TABLE IF NOT EXISTS plugin (name char(64) COLLATE utf8_bin NOT NULL DEFAULT '', " \
> > +        "  dl char(128) COLLATE utf8_bin NOT NULL DEFAULT '', " \
> > +        "  PRIMARY KEY (name)) ENGINE=MyISAM DEFAULT CHARSET=utf8 COLLATE=utf8_bin COMMENT='MySQL plugins';" `
> 
> Eh, there's mysql_upgrade and mysql_fix_privilege_tables.sql, etc
> Why part of it has to be hard-coded in the debian scripts?

Otto, you must've missed this question.

Regards,
Sergei