Code review comment for lp:~qu1j0t3/maria/solaris10-port

Revision history for this message
Toby Thain (qu1j0t3) wrote :

On 3-Jun-09, at 12:48 PM, Kristian Nielsen wrote:

> Toby Thain <email address hidden> writes:
>
>> Toby Thain has proposed merging lp:~qu1j0t3/maria/solaris10-port
>> into lp:maria.
>
>> Added build scripts for 32 bit x86 architecture on Solaris.
>> Renamed some scripts for consistency. Changed to dynamic linking
>> of libgcc.
>> --
>> https://code.launchpad.net/~qu1j0t3/maria/solaris10-port/+merge/6999
>> You are requested to review the proposed merge of lp:~qu1j0t3/
>> maria/solaris10-port into lp:maria.
>>
>> === modified file 'BUILD/compile-solaris-amd64'
>> --- BUILD/compile-solaris-amd64 2009-05-09 04:01:53 +0000
>> +++ BUILD/compile-solaris-amd64 2009-06-02 22:10:57 +0000
>> @@ -26,7 +26,7 @@
>> extra_flags="$amd64_cflags -D__sun -m64 -mtune=athlon64"
>> extra_configs="$amd64_configs $max_configs --with-libevent"
>>
>> -LDFLAGS="-lmtmalloc -static-libgcc"
>> +LDFLAGS="-lmtmalloc -R/usr/sfw/lib/64"
>> export LDFLAGS
>>
>> . "$path/FINISH.sh"
>>
>
> I'm basically ok with these changes.
>
> However, I would like you to add some explaining comments in the
> commit
> message about why the changes are done, especially the above regarding
> -static-libgcc and -R/usr/sfw/lib. Why are the changes needed, and
> what do
> they do?

The -static-libgcc was a legacy of the original build scripts. -R
(analogous to -L link time search path) is a Solaris mechanism to
ensure a needed lib directory is searched at runtime.

Background: Historically gcc has been available on Solaris through
3rd-party ports. In Solaris 10, gcc comes bundled, under /usr/sfw. If
you use a 3rd party gcc then you cannot, of course, rely on the
runtime library being available on all systems where the binaries
might be running; which is perhaps why the old scripts linked libgcc
in this way. But this doesn't apply if you are using the Solaris gcc,
which is what I would recommend as a default procedure. (If a
particular site wants to use source builds and 3rd party gcc then
presumably the necessary libraries have been made available.)

Personally I prefer to see this system library dynamically linked.
One reason is that the application benefits from ordinary system
patch maintenance.

>
> (generally it is more important in comments to explain _why_ than
> to explain
> _what_; the code already shows what happens, but not why.)
>
> Eg. if I had to merge these changes against conflicting changes
> from MySQL
> upstream, I would have no clue about what to do to resolve the
> conflict.
>
> You might also want to add some comments in the new script files
> for the Forte
> C options if any of them are of special importance, it is up to you
> really,
> you are the one who knows what they mean.

These are cribbed from other Solaris builders who have benchmarked
some benefit. I don't have my own benchmarks to justify them, but
this should be done at some point (any recommended procedures?
mysqlslap?)

I guess we have 3 "less arbitrary" choices:
1. follow what Sun/MySQL use as best practice
2. build generically - without any platform performance options, until:
3. recommended options can be developed for Maria based on benchmarking

--Toby

>
> - Kristian.
> --
> https://code.launchpad.net/~qu1j0t3/maria/solaris10-port/+merge/6999
> Your team Maria developers is subscribed to branch lp:maria.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~maria-developers
> More help : https://help.launchpad.net/ListHelp

« Back to merge proposal