maria-developers team mailing list archive

[Toby Thain <toby@xxxxxxxxxxxxxxxxxxx>]

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.