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.