[Maria-developers] MariaDB Debian packaging: pending merge to upstream
Hello! Now when the rehearsal is done using simple spelling error fix patches, I now posted the first big merge request about my 5.5/debian upon the upstream 5.5 branch. Issue at https://mariadb.atlassian.net/browse/MDEV-6284 Merge request at https://code.launchpad.net/~otto/maria/maria-fix-debpkg-5.5/+merge/221576 I also have these old merge request still open: https://code.launchpad.net/~otto/maria/maria-fix-debian-copyright-trunk/+mer... https://code.launchpad.net/~otto/maria/maria-fix-debian-copyright-5.5/+merge... The sooner somebody has can merge these the less likely it is that there will be merge conflicts.. Thanks! -- Check out our blog at http://seravo.fi/blog and follow @ottokekalainen
Hi, Otto! On May 30, Otto Kekäläinen wrote:
Now when the rehearsal is done using simple spelling error fix patches, I now posted the first big merge request about my 5.5/debian upon the upstream 5.5 branch.
Issue at https://mariadb.atlassian.net/browse/MDEV-6284 Merge request at https://code.launchpad.net/~otto/maria/maria-fix-debpkg-5.5/+merge/221576
Here's a review, lots of questions sorry. It might be faster if you ping me on irc and we'll go through my questions, instead of doing it by email. Regards, Sergei
2014-07-25 15:02 GMT+03:00 Sergei Golubchik <serg@mariadb.org>:
Here's a review, lots of questions sorry.
It might be faster if you ping me on irc and we'll go through my questions, instead of doing it by email.
Thanks for your list of issues. Some of them are my fault or other things I can address now. Some I need to explain or maybe re-think along the ways of your suggestion. Lots of the issues are inherited from mysql-5.6 packaging, e.g. the rules and scripts parts. There are also some things (e.g. the man pages) that have been fixed in between sending my bzr merge request and you reviewing it. I think I'll pull back the merge request, fix most of the issues into my own Debian git repo, and then send back a new bzr pull request. Eventually. It will take weeks, so I just wanted to notify you that this will be the status until then. Thanks for your code review (of almost everything I've done in MariaDB packaging in the last year or so)!
Hi, Otto! On Jul 28, Otto Kekäläinen wrote:
2014-07-25 15:02 GMT+03:00 Sergei Golubchik <serg@mariadb.org>:
Here's a review, lots of questions sorry.
It might be faster if you ping me on irc and we'll go through my questions, instead of doing it by email.
I think I'll pull back the merge request, fix most of the issues into my own Debian git repo, and then send back a new bzr pull request. Eventually. It will take weeks, so I just wanted to notify you that this will be the status until then.
Just something to clarify - I understand that MySQL maintainers in Debian try to limit their changes to debian/ directory. I would probably do the same, were I a maintainer. But as a vendor I don't do that - on the opposite, I prefer to fix issues in the code, not work around them in packaging (e.g. by lintian overrides or patches). You'll see that some of my review comments mean exactly that, they don't say that the packaging is wrong, they say that the workaround should be removed and the real issue should be fixed in the code (or CMakeLists.txt, whatever) instead. Regards, Sergei
Hello Sergei! 2014-07-28 0:27 GMT+03:00 Otto Kekäläinen <otto@seravo.fi>:
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.
Hi, Otto! On Aug 08, Otto Kekäläinen wrote:
2014-07-28 0:27 GMT+03:00 Otto Kekäläinen <otto@seravo.fi>:
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/ff3f1b0116be219c95471eb82cee84ea.... Related is also https://github.com/ottok/mariadb-5.5/commit/21c251a852b2bcbafa8b9e0049be0471...
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_MA... " 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
Hello! 2014-10-01 22:14 GMT+03:00 Sergei Golubchik <serg@mariadb.org>: [..]
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?
I don't remember. It is kind of hard to keep this discussion in my head as it spans over several months. At the moment I've prioritized issues that Debian people point because I am interested in getting MariaDB into Debian in a state that Debian values as much as possible. Some of the comments you sent in July haven't yet been addressed but they are in the list as TODO items. Note that current state-of-the art packaging is in the MariaDB-10.0 repo I've been working on in September, and those changes haven't been reviewed/discussed with you. This review-everything-in-one-go is a bit too big task, so I've thought about sending you these packaging improvements as multiple small patches instead, which is easiest using the MariaDB 10.1 branch as it is in git. Also I think there are benefits in introducing better packaging in 10.1 this fall/year when it has never been released yet. So you can wait for me to send patches against debian/ files in the 10.1 tree in coming months.
=== 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.
Current state is http://anonscm.debian.org/cgit/pkg-mysql/mariadb-10.0.git/tree/debian/mariad... and it could in theory be expanded to include even more cli tools, but very low priority.
=== 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.
OK
=== 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.
OK
=== 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.
Still here http://anonscm.debian.org/cgit/pkg-mysql/mariadb-10.0.git/tree/debian/mariad... TODO item for me
=== 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.
I think this is fine, no need to relocate the file.
=== 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.
OK
=== 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).
It could also be replaced with printf, that would be the POSIX way of doing it. But still, having lots of big echos is stupid, better to have separate files that are piped around. The install/remove scripts have a lot of legacy and I've asked the Debian mysql-main team to help review and modernize the install/remove scripts across all mysql variants, but I guess that is an effort that will happen some time later as it requires a lot of planning.
=== 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.
Yes, these are all fixed by now.
=== 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.
OK
=== 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.
No idea either. If you say this I think it is pretty safe to drop them.
+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?
With this script: http://labs.seravo.fi/~otto/mariadb-repo/build.sh Inside the rules file the line: ARCH := $(shell dpkg-architecture -qDEB_BUILD_ARCH) ..will yield correct cross-compilation target arch. I don't recall the details, but it seems to be passed on to cmake and this has worked fine so far.
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.
Sure, just tell me if you have better ideas that will work with the Debian infrastructure as nicely as current solution does.
# 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.
OK, I'll remove this section.
- 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/ff3f1b0116be219c95471eb82cee84ea.... Related is also https://github.com/ottok/mariadb-5.5/commit/21c251a852b2bcbafa8b9e0049be0471...
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_MA... " 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.
In 10.0 rules file I have: # Add support for verbose builds ifneq (,$(filter verbose,$(DEB_BUILD_OPTIONS))) MAKEFLAGS += VERBOSE=1 endif Seems to work 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)
OK. I'll rewrite the parallel check section.
+ # 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?
It is better to modify debian/* stuff with scripts inside debian/ due to how Debian packaging and maintenance works. Much easier to track what affects what. Or at least from a Debian packager point of view and everybody else who need to deal with the package (e.g. a security team making new releases without the original author).
+ # 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.
this is old legacy, I have no idea why it does this, but the same part is in the mysql packages too. This is part of the install/remove modernization I've tried to start discussion and planning about, but it hasn't progressed yet. For interoperability reasons inside the Debian mysql-maint team ti would be nice that all packages and packagers agree on what the install/remove scripts do. Thanks Sergei for your comments! Let's continue this merge process once I get around to sending pull requests against the 10.1 git tree. In the mean time it is very kind of you that you have helped me with build issues etc so that the Debian packages are now in a really good shape and most likely will flow frictionlessly through the Debian QA processes. For example if we can get this all green it would be cool: https://buildd.debian.org/status/package.php?p=mariadb-10.0&suite=experimental http://buildd.debian-ports.org/status/package.php?p=mariadb-10.0&suite=experimental We are very close already :) - Otto
participants (2)
-
Otto Kekäläinen -
Sergei Golubchik