[Maria-developers] bzr commit into MariaDB 5.1, with Maria 1.5:maria branch (knielsen:2698)
#At lp:maria 2698 knielsen@knielsen-hq.org 2009-05-05 [merge] test merge. modified: BUILD/compile-solaris-amd64 BUILD/compile-solaris-amd64-debug* BUILD/compile-solaris-amd64-forte BUILD/compile-solaris-amd64-forte-debug* storage/innobase/include/univ.i storage/pbxt/src/Makefile.am storage/pbxt/src/lock_xt.h storage/pbxt/src/pbms.h === modified file 'BUILD/compile-solaris-amd64' --- a/BUILD/compile-solaris-amd64 2007-04-12 11:20:38 +0000 +++ b/BUILD/compile-solaris-amd64 2009-05-04 14:36:55 +0000 @@ -1,55 +1,34 @@ -#!/usr/bin/bash +#!/bin/sh -function _find_mysql_root () ( - while [ "x$PWD" != "x/" ]; do - # Check if some directories are present - if [ -d BUILD -a -d sql -a -d mysys ]; then - echo "$PWD" - return 0 - fi - cd .. - done - return 1 -) +# Pre-requisites for Solaris 10: +# +# setup: +# ln -s `which perl` /usr/local/bin +# your $PATH needs to include (in this order): +# /usr/local/bin:/usr/sfw/bin:/usr/ccs/bin +# +# packages from http://sunfreeware.com/indexintel10.html : +# automake-1.10.2-sol10-x86-local.gz +# autoconf-2.63-sol10-x86-local.gz +# m4-1.4.12-sol10-x86-local.gz +# libsigsegv-2.6-sol10-x86-local.gz +# libtool-1.5.24-sol10-x86-local.gz +# ( how to install these packages: + # wget ftp://ftp.sunfreeware.com/pub/freeware/intel/10/automake-1.10.2-sol10-x86-local.gz + # gunzip automake-1.10.2-sol10-x86-local.gz + # pkgadd -d automake-1.10.2-sol10-x86-local +# ) + +# +# if using --with-libevent, install that library from +# http://www.monkey.org/~provos/libevent/ -make -k clean || true -/bin/rm -f */.deps/*.P config.cache - path=`dirname $0` -. "$path/autorun.sh" - -warning_flags="-Wimplicit -Wreturn-type -Wswitch -Wtrigraphs -Wcomment -W -Wchar-subscripts -Wformat -Wparentheses -Wsign-compare -Wwrite-strings -Wunused" -compiler_flags="-g -O3 -fno-omit-frame-pointer" +. "$path/SETUP.sh" +extra_flags="$amd64_cflags -D__sun -m64 -mtune=athlon64" +extra_configs="$amd64_configs $max_configs --with-libevent" -export CC CXX CFLAGS CXXFLAGS LDFLAGS LIBS -CC="gcc" -CXX="gcc" -CFLAGS="$warning_flags $compiler_flags" -CXXFLAGS="" -LDFLAGS="-O3 -g -static-libgcc" -LIBS=-lmtmalloc -root=$(_find_mysql_root) +LDFLAGS="-lmtmalloc -static-libgcc" +export LDFLAGS -$root/configure \ - --prefix=/usr/local/mysql \ - --localstatedir=/usr/local/mysql/data \ - --libexecdir=/usr/local/mysql/bin \ - --with-extra-charsets=complex \ - --enable-thread-safe-client \ - --enable-local-infile \ - --with-zlib-dir=bundled \ - --with-big-tables \ - --with-readline \ - --with-archive-storage-engine \ - --with-named-curses=-lcurses \ - --with-big-tables \ - --with-innodb \ - --with-berkeley-db \ - --with-example-storage-engine \ - --with-blackhole-storage-engine \ - --with-ndbcluster \ - --with-federated-storage-engine \ - --with-csv-storage-engine \ - --with-ssl \ - --with-embedded-server \ - --disable-shared +. "$path/FINISH.sh" === modified file 'BUILD/compile-solaris-amd64-debug' (properties changed: -x to +x) --- a/BUILD/compile-solaris-amd64-debug 2007-11-10 10:03:07 +0000 +++ b/BUILD/compile-solaris-amd64-debug 2009-05-04 14:36:55 +0000 @@ -1,10 +1,11 @@ -#! /bin/sh +#!/bin/sh + path=`dirname $0` . "$path/SETUP.sh" -amd64_cflags="-m64 -mtune=athlon64" -extra_flags="$amd64_cflags $debug_cflags $max_cflags" -c_warnings="$c_warnings $debug_extra_warnings" -cxx_warnings="$cxx_warnings $debug_extra_warnings" -extra_configs="$amd64_configs $debug_configs $max_configs --enable-thread-safe-client" +extra_flags="$amd64_cflags -D__sun -m64 $debug_cflags" +extra_configs="$amd64_configs $debug_configs $max_configs --with-libevent" + +LDFLAGS="-lmtmalloc -static-libgcc" +export LDFLAGS . "$path/FINISH.sh" === modified file 'BUILD/compile-solaris-amd64-forte' --- a/BUILD/compile-solaris-amd64-forte 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte 2009-05-04 15:48:27 +0000 @@ -1,53 +1,35 @@ -#! /bin/sh +#!/bin/sh -gmake -k clean || true -/bin/rm -f */.deps/*.P config.cache - -path=`dirname $0` -. "$path/autorun.sh" +# See file compile-solaris-amd64 for basic pre-requisites. + +# This build uses the Sun Studio compilers (cc, CC). +# http://developers.sun.com/sunstudio/downloads/index.jsp +# Note that you may want to apply current patches, as the downloaded version +# is typically out of date. Download the PKG version if you intend to patch! + +# After installing, add /opt/SUNWspro/bin to your $PATH -# For "optimal" code for this computer add -fast to EXTRA -# To compile 64 bit, add -m64 to EXTRA_64_BIT -EXTRA_64_BIT="-m64" -EXTRA="-fast" +path=`dirname $0` +. "$path/SETUP.sh" + +extra_flags="-m64 -mt -D_FORTEC_ -xbuiltin=%all -xlibmil -xlibmopt -fns=no -xprefetch=auto -xprefetch_level=3" +extra_configs="$max_configs --with-libevent" -# -# The following should not need to be touched -# - -export CC CXX CFLAGS CXXFLAGS LIBS -STD="-g -mt -D_FORTEC_ $EXTRA $EXTRA_64_BIT" -ASFLAGS="$EXTRA_64_BIT" -CC=cc-5.0 -CFLAGS="-Xa -xstrconst $STD" +# I want this for my buildbot, which has limited zone resources +#export AM_MAKEFLAGS +#AM_MAKEFLAGS="" + +export warnings c_warnings cxx_warnings base_cxxflags +warnings="" +c_warnings="" +cxx_warnings="" +base_cxxflags="-noex" + +export CC CXX CFLAGS CXXFLAGS LDFLAGS +CC=cc +CFLAGS="-xstrconst" CXX=CC -CXXFLAGS="-noex $STD" -LIBS=-lmtmalloc -./configure \ - --prefix=/usr/local/mysql \ - --localstatedir=/usr/local/mysql/data \ - --libexecdir=/usr/local/mysql/bin \ - --with-extra-charsets=complex \ - --enable-thread-safe-client \ - --enable-local-infile \ - --with-zlib-dir=bundled \ - --with-big-tables \ - --with-readline \ - --with-archive-storage-engine \ - --with-named-curses=-lcurses \ - --with-big-tables \ - --with-innodb \ - --with-example-storage-engine \ - --with-blackhole-storage-engine \ - --with-federated-storage-engine \ - --with-csv-storage-engine \ - --with-ssl \ - --enable-assembler - -# Not including: -# --with-ndbcluster -# --with-berkeley-db +LDFLAGS="-lmtmalloc" -gmake -j4 -test $? = 0 && make test +. "$path/FINISH.sh" === modified file 'BUILD/compile-solaris-amd64-forte-debug' (properties changed: -x to +x) --- a/BUILD/compile-solaris-amd64-forte-debug 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte-debug 2009-05-04 15:48:27 +0000 @@ -1,54 +1,28 @@ -#! /bin/sh +#!/bin/sh -gmake -k clean || true -/bin/rm -f */.deps/*.P config.cache - path=`dirname $0` -. "$path/autorun.sh" +. "$path/SETUP.sh" -# To compile 64 bit, add -m64 to EXTRA_64_BIT -EXTRA_64_BIT="-m64" - -# For "optimal" code for this computer add -fast to EXTRA. Note that -# this causes problem with debugging the program since -fast implies -# -xO5. -EXTRA="" - -# -# The following should not need to be touched -# - -export CC CXX CFLAGS CXXFLAGS -STD="-g -mt -D_FORTEC_ $EXTRA $EXTRA_64_BIT $debug_cflags" -ASFLAGS="$EXTRA_64_BIT" -CC=cc-5.0 -CFLAGS="-Xa -xstrconst $STD" +# take only #define options - the others are gcc specific +DEFS="" +for F in $debug_cflags ; do + expr "$F" : "^-D" && DEFS="$DEFS $F" +done +debug_cflags="-O0 -g $DEFS" + +extra_flags="-m64 -mt -D_FORTEC_ -xlibmopt -fns=no $debug_cflags" +extra_configs="$max_configs --with-libevent $debug_configs" + +export warnings c_warnings cxx_warnings base_cxxflags debug_cflags +warnings="" +c_warnings="" +cxx_warnings="" +base_cxxflags="-noex" + +export CC CXX CFLAGS CXXFLAGS LDFLAGS +CC=cc +CFLAGS="-xstrconst" CXX=CC -CXXFLAGS="-noex $STD" -./configure \ - --prefix=/usr/local/mysql \ - --localstatedir=/usr/local/mysql/data \ - --libexecdir=/usr/local/mysql/bin \ - --with-extra-charsets=complex \ - --enable-thread-safe-client \ - --enable-local-infile \ - --with-zlib-dir=bundled \ - --with-big-tables \ - --with-readline \ - --with-archive-storage-engine \ - --with-named-curses=-lcurses \ - --with-big-tables \ - --with-innodb \ - --with-example-storage-engine \ - --with-blackhole-storage-engine \ - --with-federated-storage-engine \ - --with-csv-storage-engine \ - --with-ssl \ - --with-debug \ - --enable-assembler - -# Not including: -# --with-ndbcluster -# --with-berkeley-db +LDFLAGS="-lmtmalloc" -gmake -j4 +. "$path/FINISH.sh" === modified file 'storage/innobase/include/univ.i' --- a/storage/innobase/include/univ.i 2008-03-27 01:40:45 +0000 +++ b/storage/innobase/include/univ.i 2009-05-04 14:54:10 +0000 @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope /* We only try to do explicit inlining of functions with gcc and Microsoft Visual C++ */ -# if !defined(__GNUC__) +# if !defined(__GNUC__) && !defined(__SUNPRO_C) # undef UNIV_MUST_NOT_INLINE /* Remove compiler warning */ # define UNIV_MUST_NOT_INLINE # endif === modified file 'storage/pbxt/src/Makefile.am' --- a/storage/pbxt/src/Makefile.am 2009-04-01 06:40:11 +0000 +++ b/storage/pbxt/src/Makefile.am 2009-05-04 14:36:55 +0000 @@ -46,7 +46,7 @@ libpbxt_la_CFLAGS = $(AM_CFLAGS) -DMYSQ EXTRA_LIBRARIES = libpbxt.a noinst_LIBRARIES = libpbxt.a libpbxt_a_SOURCES = $(libpbxt_la_SOURCES) -libpbxt_a_CXXFLAGS = $(AM_CXXFLAGS) -Wno-long-long +libpbxt_a_CXXFLAGS = $(AM_CXXFLAGS) libpbxt_a_CFLAGS = $(AM_CFLAGS) -std=c99 EXTRA_DIST = CMakeLists.txt === modified file 'storage/pbxt/src/lock_xt.h' --- a/storage/pbxt/src/lock_xt.h 2009-04-02 10:03:14 +0000 +++ b/storage/pbxt/src/lock_xt.h 2009-05-04 14:36:55 +0000 @@ -208,9 +208,9 @@ inline void xt_atomic_dec2(volatile xtWo #elif defined(__GNUC__) __sync_fetch_and_sub(mptr, 1); #elif defined(XT_SPL_SOLARIS_LIB) - val1 = atomic_dec_16_nv(mptr); + atomic_dec_16_nv(mptr); #else - val1 = --(*mptr); + --(*mptr); #endif } === modified file 'storage/pbxt/src/pbms.h' --- a/storage/pbxt/src/pbms.h 2009-03-26 12:18:01 +0000 +++ b/storage/pbxt/src/pbms.h 2009-05-03 16:31:02 +0000 @@ -782,21 +782,28 @@ private: void deleteTempFiles() { - struct dirent entry; + struct dirent *entry; struct dirent *result; DIR *odir; int err; + size_t sz; char temp_file[100]; +#ifdef XT_SOLARIS + sz = sizeof(struct dirent) + pathconf("/tmp/", _PC_NAME_MAX); // Solaris, see readdir(3C) +#else + sz = sizeof(struct dirent); +#endif + entry = (struct dirent*)malloc(sz); if (!(odir = opendir("/tmp/"))) return; - err = readdir_r(odir, &entry, &result); + err = readdir_r(odir, entry, &result); while (!err && result) { const char **prefix = temp_prefix; while (*prefix) { - if (startsWith(entry.d_name, *prefix)) { - int pid = atoi(entry.d_name + strlen(*prefix)); + if (startsWith(entry->d_name, *prefix)) { + int pid = atoi(entry->d_name + strlen(*prefix)); /* If the process does not exist: */ if (kill(pid, 0) == -1 && errno == ESRCH) { @@ -807,9 +814,10 @@ private: prefix++; } - err = readdir_r(odir, &entry, &result); + err = readdir_r(odir, entry, &result); } closedir(odir); + free(entry); } }; #endif // PBMS_API
Hi Toby, Nice (and much needed) cleanup of the Solaris BUILD/* scripts, thanks a lot! And thanks for the PBXT portability fixes also. I have a few smaller comments given inline below. The patch should be ok to be pushed to main after these are resolved. I suggest you re-commit all of the changes as a single commit (this is generally preferred in Maria development), or at least add a good commit message (eg. mention fixing the BUILD/ scripts). Also, you should add per-file commit messages; one way to do this I know of is using the `bzr gcommit` plugin (I personally do not like per-file commit messages much, but in Maria development they are generally preferred).
=== modified file 'BUILD/compile-solaris-amd64' --- a/BUILD/compile-solaris-amd64 2007-04-12 11:20:38 +0000 +++ b/BUILD/compile-solaris-amd64 2009-05-04 14:36:55 +0000
+# Pre-requisites for Solaris 10: +# +# setup: +# ln -s `which perl` /usr/local/bin
Just out of curiosity, what is the reason for this requirement? Preferably, we should fix the source so that things like this are not necessary. (But ok to leave as is in patch).
+# if using --with-libevent, install that library from +# http://www.monkey.org/~provos/libevent/
Is it really necessary to install libevent separately when using --with-libevent? The idea is that ./configure should use the bundled libevent (in extra/libevent/) if none is installed. If really necessary, please explain (on the mailing list) why this is so, so that we can fix it seperately. If not really necessary, please fix the comment.
=== modified file 'BUILD/compile-solaris-amd64-forte' --- a/BUILD/compile-solaris-amd64-forte 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte 2009-05-04 15:48:27 +0000
+# I want this for my buildbot, which has limited zone resources +#export AM_MAKEFLAGS +#AM_MAKEFLAGS=""
Please remove this (one should generally not leave commented-out code in the main tree). However, I guess the problem is really that you don't want to run with `make -j 6` on the low-resource host, right? We should make a proper fix for this. How about fixing SETUP.sh like this? === modified file 'BUILD/SETUP.sh' --- BUILD/SETUP.sh 2009-03-22 12:16:09 +0000 +++ BUILD/SETUP.sh 2009-05-05 13:02:33 +0000 @@ -80,7 +80,10 @@ path=`dirname $0` . "$path/check-cpu" export AM_MAKEFLAGS -AM_MAKEFLAGS="-j 6" +if test -z "$AM_MAKEFLAGS" +then + AM_MAKEFLAGS="-j 6" +fi # SSL library to use.--with-ssl will select our bundled yaSSL # implementation of SSL. To use openSSl you will nee too point out Then you can set AM_MAKEFLAGS for the buildbot account, or I can set it in the master config, and that value will override the default -j 6 in SETUP.sh. If you agree, please include this change in fixed changeset.
+export warnings c_warnings cxx_warnings base_cxxflags
Hm, these actually don't have to be exported, do they? Seems they are mostly for use in SETUP.sh? If they need not be exported, please remove the export to avoid confusion (if it is needed, leave it as it is not really a problem).
=== modified file 'BUILD/compile-solaris-amd64-forte-debug' (properties changed: -x to +x) --- a/BUILD/compile-solaris-amd64-forte-debug 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte-debug 2009-05-04 15:48:27 +0000
+# take only #define options - the others are gcc specific +DEFS="" +for F in $debug_cflags ; do + expr "$F" : "^-D" && DEFS="$DEFS $F" +done +debug_cflags="-O0 -g $DEFS"
Hm. You should not really have to do this ;-). But I agree with this solution for now.
+export warnings c_warnings cxx_warnings base_cxxflags debug_cflags
Again, check if export is needed, and remove if not.
=== modified file 'storage/innobase/include/univ.i' --- a/storage/innobase/include/univ.i 2008-03-27 01:40:45 +0000 +++ b/storage/innobase/include/univ.i 2009-05-04 14:54:10 +0000 @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope /* We only try to do explicit inlining of functions with gcc and Microsoft Visual C++ */
-# if !defined(__GNUC__) +# if !defined(__GNUC__) && !defined(__SUNPRO_C) # undef UNIV_MUST_NOT_INLINE /* Remove compiler warning */ # define UNIV_MUST_NOT_INLINE # endif
What is the purpose of this change? I am wondering if this change is unintentional (if so it should be removed)? Or if this is really what you intended, please explain in the commit message what it does / why it is done. And also update the comment above the changed line. Thanks for the effort! And looking forward to seeing a Solaris buildbot host :-) - Kristian.
On 5-May-09, at 9:24 AM, Kristian Nielsen wrote:
Hi Toby,
Nice (and much needed) cleanup of the Solaris BUILD/* scripts, thanks a lot! And thanks for the PBXT portability fixes also.
I have a few smaller comments given inline below. The patch should be ok to be pushed to main after these are resolved.
I suggest you re-commit all of the changes as a single commit (this is generally preferred in Maria development), or at least add a good commit message (eg. mention fixing the BUILD/ scripts). Also, you should add per-file commit messages; one way to do this I know of is using the `bzr gcommit` plugin (I personally do not like per-file commit messages much, but in Maria development they are generally preferred).
Thanks for the tips. I'm new to Bzr. (Plenty of Subversion.)
=== modified file 'BUILD/compile-solaris-amd64' --- a/BUILD/compile-solaris-amd64 2007-04-12 11:20:38 +0000 +++ b/BUILD/compile-solaris-amd64 2009-05-04 14:36:55 +0000
+# Pre-requisites for Solaris 10: +# +# setup: +# ln -s `which perl` /usr/local/bin
Just out of curiosity, what is the reason for this requirement?
Path assumptions in the autotools. $ head `which aclocal` #!/usr/local/bin/perl -w ...leading to... + aclocal BUILD/compile-solaris-amd64: aclocal: not found + die Can't execute aclocal + echo Can't execute aclocal Can't execute aclocal + exit 1 aclocal is part of the 3rd party (Sun Freeware) prerequisite packages mentioned in compile-solaris-amd64, which use /usr/local as their prefix. As a consequence, their scripts are built to assume Perl in / usr/local/bin as well. The Solaris bundled Perl is not, hence the symlink (I can add a more detailed comment).
Preferably, we should fix the source so that things like this are not necessary. (But ok to leave as is in patch).
+# if using --with-libevent, install that library from +# http://www.monkey.org/~provos/libevent/
Is it really necessary to install libevent separately when using --with-libevent? The idea is that ./configure should use the bundled libevent (in extra/libevent/) if none is installed.
I missed that it was bundled, so I can remove that comment completely.
If really necessary, please explain (on the mailing list) why this is so, so that we can fix it seperately. If not really necessary, please fix the comment.
=== modified file 'BUILD/compile-solaris-amd64-forte' --- a/BUILD/compile-solaris-amd64-forte 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte 2009-05-04 15:48:27 +0000
+# I want this for my buildbot, which has limited zone resources +#export AM_MAKEFLAGS +#AM_MAKEFLAGS=""
Please remove this (one should generally not leave commented-out code in the main tree).
However, I guess the problem is really that you don't want to run with `make -j 6` on the low-resource host, right? We should make a proper fix for this.
I definitely don't want -jN for the bot, and I wasn't sure how best to handle that. I was going to ask you, actually, this comment was really a reminder. :)
How about fixing SETUP.sh like this?
=== modified file 'BUILD/SETUP.sh' --- BUILD/SETUP.sh 2009-03-22 12:16:09 +0000 +++ BUILD/SETUP.sh 2009-05-05 13:02:33 +0000 @@ -80,7 +80,10 @@ path=`dirname $0` . "$path/check-cpu"
export AM_MAKEFLAGS -AM_MAKEFLAGS="-j 6" +if test -z "$AM_MAKEFLAGS" +then + AM_MAKEFLAGS="-j 6" +fi
# SSL library to use.--with-ssl will select our bundled yaSSL # implementation of SSL. To use openSSl you will nee too point out
Then you can set AM_MAKEFLAGS for the buildbot account, or I can set it in the master config, and that value will override the default -j 6 in SETUP.sh.
If you agree, please include this change in fixed changeset.
This is a great suggestion. Not knowing buildbot, I wasn't sure how to best do it. I can just set it locally, I think.
+export warnings c_warnings cxx_warnings base_cxxflags
Hm, these actually don't have to be exported, do they? Seems they are mostly for use in SETUP.sh?
If they need not be exported, please remove the export to avoid confusion (if it is needed, leave it as it is not really a problem).
They do need to be exported for FINISH, as we must remove the gcc- only options produced by SETUP which are incompatible with Sun cc on Solaris. (I'm also trying to remain compatible with Solaris sh.)
=== modified file 'BUILD/compile-solaris-amd64-forte- debug' (properties changed: -x to +x) --- a/BUILD/compile-solaris-amd64-forte-debug 2008-09-30 20:57:48 +0000 +++ b/BUILD/compile-solaris-amd64-forte-debug 2009-05-04 15:48:27 +0000
+# take only #define options - the others are gcc specific +DEFS="" +for F in $debug_cflags ; do + expr "$F" : "^-D" && DEFS="$DEFS $F" +done +debug_cflags="-O0 -g $DEFS"
Hm. You should not really have to do this ;-). But I agree with this solution for now.
Sure, it's ugly. I wish I could think of a better way myself.
+export warnings c_warnings cxx_warnings base_cxxflags debug_cflags
Again, check if export is needed, and remove if not.
=== modified file 'storage/innobase/include/univ.i' --- a/storage/innobase/include/univ.i 2008-03-27 01:40:45 +0000 +++ b/storage/innobase/include/univ.i 2009-05-04 14:54:10 +0000 @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope /* We only try to do explicit inlining of functions with gcc and Microsoft Visual C++ */
-# if !defined(__GNUC__) +# if !defined(__GNUC__) && !defined(__SUNPRO_C) # undef UNIV_MUST_NOT_INLINE /* Remove compiler warning */ # define UNIV_MUST_NOT_INLINE # endif
What is the purpose of this change?
I am wondering if this change is unintentional (if so it should be removed)?
Or if this is really what you intended,
This (along with some of the compiler options) are performance enhancements which have been benchmarked by other builders on Solaris. It is not strictly required for portability, so it could be removed, but you are very likely to want it for InnoDB. It merely enables inlining in Sun CC, prevented by the vanilla sources, but normally done by GCC on other platforms. There is some disagreement on optimal compiler flags especially for Sun cc so I have tried to be conservative. These are performance related: -xbuiltin=%all -xlibmil -xlibmopt -fns=no -xprefetch=auto - xprefetch_level=3 (more intrinsics, inline some math lib, performance-optimised math lib, IEEE strictness, and prefetching)
please explain in the commit message what it does / why it is done. And also update the comment above the changed line.
Perhaps all performance tuning should be removed now, and added in a separate performance patch, with supporting numbers? What are you most comfortable with?
Thanks for the effort! And looking forward to seeing a Solaris buildbot host :-)
Thanks for the feedback, I'll try to turn this around quickly. --Toby
- Kristian.
Toby Thain <toby@telegraphics.com.au> writes:
Just out of curiosity, what is the reason for this requirement?
Path assumptions in the autotools.
$ head `which aclocal` #!/usr/local/bin/perl -w
Ah, I see, thanks for the info.
+export warnings c_warnings cxx_warnings base_cxxflags
Hm, these actually don't have to be exported, do they? Seems they are mostly for use in SETUP.sh?
If they need not be exported, please remove the export to avoid confusion (if it is needed, leave it as it is not really a problem).
They do need to be exported for FINISH, as we must remove the gcc- only options produced by SETUP which are incompatible with Sun cc on Solaris. (I'm also trying to remain compatible with Solaris sh.)
Ok, thanks for information. (My impression was that if set in the script, they would also be available in other scripts sourced from this first script. You can leave it, it's not that important).
+# take only #define options - the others are gcc specific +DEFS="" +for F in $debug_cflags ; do + expr "$F" : "^-D" && DEFS="$DEFS $F" +done +debug_cflags="-O0 -g $DEFS"
Hm. You should not really have to do this ;-). But I agree with this solution for now.
Sure, it's ugly. I wish I could think of a better way myself.
Well, the solution would be that SETUP.sh should not put gcc-specific flags in $debug_cflags! But fixing that should not be a blocker for your patch, I think.
=== modified file 'storage/innobase/include/univ.i' --- a/storage/innobase/include/univ.i 2008-03-27 01:40:45 +0000 +++ b/storage/innobase/include/univ.i 2009-05-04 14:54:10 +0000 @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope /* We only try to do explicit inlining of functions with gcc and Microsoft Visual C++ */
-# if !defined(__GNUC__) +# if !defined(__GNUC__) && !defined(__SUNPRO_C) # undef UNIV_MUST_NOT_INLINE /* Remove compiler warning */ # define UNIV_MUST_NOT_INLINE # endif
What is the purpose of this change?
I am wondering if this change is unintentional (if so it should be removed)?
Or if this is really what you intended,
This (along with some of the compiler options) are performance enhancements which have been benchmarked by other builders on Solaris. It is not strictly required for portability, so it could be removed, but you are very likely to want it for InnoDB. It merely enables inlining in Sun CC, prevented by the vanilla sources, but normally done by GCC on other platforms.
Ok, thanks for information. I think this sounds good. So I suggest to keep it (since you tested that it works, it seems a good change). Just mention it in the commit comment, and fix the comment above the change (which is apparently already wrong since it mentions Visual C++). I suggest this: /* Enable explicit inlining of functions only for compilers known to support it. */ - Kristian.
On 5-May-09, at 2:39 PM, Kristian Nielsen wrote:
Toby Thain <toby@telegraphics.com.au> writes: ...
+# take only #define options - the others are gcc specific +DEFS="" +for F in $debug_cflags ; do + expr "$F" : "^-D" && DEFS="$DEFS $F" +done +debug_cflags="-O0 -g $DEFS"
Hm. You should not really have to do this ;-). But I agree with this solution for now.
Sure, it's ugly. I wish I could think of a better way myself.
Well, the solution would be that SETUP.sh should not put gcc- specific flags in $debug_cflags! But fixing that should not be a blocker for your patch, I think.
Maybe separation of variables for the truly platform-independent flags (like -g, -O, ...) and the non-portable ones that currently assume gcc (-Wxxx) is indicated. Do you have a Windows build system yet?
=== modified file 'storage/innobase/include/univ.i' --- a/storage/innobase/include/univ.i 2008-03-27 01:40:45 +0000 +++ b/storage/innobase/include/univ.i 2009-05-04 14:54:10 +0000 @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope /* We only try to do explicit inlining of functions with gcc and Microsoft Visual C++ */
-# if !defined(__GNUC__) +# if !defined(__GNUC__) && !defined(__SUNPRO_C) # undef UNIV_MUST_NOT_INLINE /* Remove compiler warning */ # define UNIV_MUST_NOT_INLINE # endif
What is the purpose of this change?
I am wondering if this change is unintentional (if so it should be removed)?
Or if this is really what you intended,
This (along with some of the compiler options) are performance enhancements ... It merely enables inlining in Sun CC, prevented by the vanilla sources, but normally done by GCC on other platforms.
Ok, thanks for information.
I think this sounds good. So I suggest to keep it (since you tested that it works, it seems a good change). Just mention it in the commit comment, and fix the comment above the change (which is apparently already wrong since it mentions Visual C++). I suggest this:
/* Enable explicit inlining of functions only for compilers known to support it. */
Will do. Thanks again! --Toby
- Kristian.
Toby Thain <toby@telegraphics.com.au> writes:
Maybe separation of variables for the truly platform-independent flags (like -g, -O, ...) and the non-portable ones that currently assume gcc (-Wxxx) is indicated. Do you have a Windows build system yet?
Yes, that would be the correct way, I think. And no, we don't have a Windows build system yet. - Kristian.
On 5-May-09, at 2:39 PM, Kristian Nielsen wrote:
...
+export warnings c_warnings cxx_warnings base_cxxflags
Hm, these actually don't have to be exported, do they? Seems they are mostly for use in SETUP.sh?
If they need not be exported, please remove the export to avoid confusion (if it is needed, leave it as it is not really a problem).
They do need to be exported for FINISH, ...
Ok, thanks for information.
(My impression was that if set in the script, they would also be available in other scripts sourced from this first script. You can leave it, it's not that important).
You are correct, the exports are not needed & will be removed; I didn't notice at first that these scripts are .'d and not run in subshells. --Toby
Kristian Nielsen <knielsen@knielsen-hq.org> writes:
I suggest you re-commit all of the changes as a single commit (this is generally preferred in Maria development), or at least add a good commit message (eg. mention fixing the BUILD/ scripts). Also, you should add per-file commit messages; one way to do this I know of is using the `bzr gcommit` plugin (I personally do not like per-file commit messages much, but in Maria development they are generally preferred).
You asked on IRC what I meant with 're-commit'. There are at least two ways: 1. Make a diff of your old tree, then clone a fresh branch of mariadb, apply your diff to that tree with `patch`, and commit the result: cd my-original-tree bzr diff -r2697 > ../mypatch cd .. bzr branch lp:maria my-new-tree cd my-new-tree patch -p0 -s < ../mypatch # You can do any additional changes you want here bzr gcommit 2. Use `bzr uncommit` to remove the previous commits (but not the related changes) in your tree, then do a new commit: cd my-original-tree bzr uncommit -r2697 # You can do any additional changes you want here bzr gcommit (Git has a `git-rebase` command to automate this, and I think there was a similar rebase plugin for bzr, haven't tried it though). The main point in either case is that you get to revise the commit message. A smaller point is that you get things collected in a single commit. Having fewer changesets is the generally prefered way for maria development (for other development I personally prefer each independent change in its own changeset, like the Linux kernel people do with their quilt patches, but it is best to follow a common style for maria). Hope this helps, else ask again. It is also ok to just commit your additional changes and push them to your original tree if you are not comfortable with re-committing, and I will handle it when I merge it into the main tree. - Kristian.
On 8-May-09, at 1:45 AM, Kristian Nielsen wrote:
Kristian Nielsen <knielsen@knielsen-hq.org> writes:
I suggest you re-commit all of the changes as a single commit (this is generally preferred in Maria development), or at least add a good commit message (eg. mention fixing the BUILD/ scripts). Also, you should add per-file commit messages; one way to do this I know of is using the `bzr gcommit` plugin (I personally do not like per-file commit messages much, but in Maria development they are generally preferred).
You asked on IRC what I meant with 're-commit'.
There are at least two ways: ...
2. Use `bzr uncommit` to remove the previous commits (but not the related changes) in your tree, then do a new commit:
cd my-original-tree bzr uncommit -r2697 # You can do any additional changes you want here
I have uncommit'd the last 4 of my local commits, but unfortunately gcommit is not available to me as I am doing this work on a headless server. I am not aware of any way this can be done on the command line (I did check #bzr and Google). Do you mind if I omit per-file remarks in this push? --Toby
bzr gcommit ...
- Kristian.
Toby Thain <toby@telegraphics.com.au> writes:
I have uncommit'd the last 4 of my local commits, but unfortunately gcommit is not available to me as I am doing this work on a headless server. I am not aware of any way this can be done on the command line (I did check #bzr and Google). Do you mind if I omit per-file remarks in this push?
No, that's fine with me, I also don't know of any other way to do per-file commit messages. - Kristian.
Hi!
"Toby" == Toby Thain <toby@telegraphics.com.au> writes:
Toby> On 8-May-09, at 1:45 AM, Kristian Nielsen wrote:
Kristian Nielsen <knielsen@knielsen-hq.org> writes:
I suggest you re-commit all of the changes as a single commit (this is generally preferred in Maria development), or at least add a good commit message (eg. mention fixing the BUILD/ scripts). Also, you should add per-file commit messages; one way to do this I know of is using the `bzr gcommit` plugin (I personally do not like per-file commit messages much, but in Maria development they are generally preferred).
You asked on IRC what I meant with 're-commit'.
There are at least two ways: ...
2. Use `bzr uncommit` to remove the previous commits (but not the related changes) in your tree, then do a new commit:
cd my-original-tree bzr uncommit -r2697 # You can do any additional changes you want here
Toby> I have uncommit'd the last 4 of my local commits, but unfortunately Toby> gcommit is not available to me as I am doing this work on a headless Toby> server. I am not aware of any way this can be done on the command Toby> line (I did check #bzr and Google). Do you mind if I omit per-file Toby> remarks in this push? In this case it's ok to only have a good global commit message. Thanks a lot for the help and for the information regarding the changes! Regards, Monty
participants (4)
-
knielsen＠knielsen-hq.org -
Kristian Nielsen
-
Michael Widenius -
Toby Thain