Re: bzr commit into mysql-5.1 branch (v.narayanan:2678) WL#4380

[Narayanan]

Joerg Bruehe wrote:
> Hi !
>
>
> I got asked to look at this changeset,

Thank you for doing this. I really appreciate your detailed reviews and 
comments.


> and I found several items of whose necessity I do not understand.
>
> I insert most of my remarks into the changeset comments, because they 
> apply to the principle and not the details,
> but some are in the code part.

OK! My Comments are inline too.

>
>
> Narayanan V wrote:
>> #At 
>>
> file:///export/home/log/Narayanan/mysql_checkouts_bazaar/5.1_main_repository/mysql-5.1-merge/
> 
>>
>>
>> 2678 Narayanan V 2008-08-13 [merge]
>> WL#4380
>> merging mysql-5.1-sea -> mysql-5.1
>> modify the build to check for ABI/API changes in the
>> stoarge engine API layer.
>> removed:
>> include/mysql_h.ic
>
> This file was used by the existing "icheck" approach.
> 1) What are the deficiencies of this approach ?
> It seems "icheck" is not so widely available, but for an ABI check
> this should not be so critical IMO.
> 2) Even if the new approach is better (I cannot say why it should be),
> why does it *replace* the old "icheck" ?

Decided to remove icheck because

1) icheck is a C interface abi/api checking tool.
2) It is not widely available (but as said by you this is a minor issue)

copy pasted from man icheck output

"icheck(1) icheck(1)

NAME
icheck - C interface ABI/API checker"

It does not work with C++. It fails when we want to include mysql_priv.h 
in the check.
mysql_priv.h needs to go through 4394 though because it still has lots 
of parts that
need to be reorganized and will be done as part of 4394.

The old icheck rule was anyway not part of the main build and I did not 
see the logic
behind having icheck for plugin.h and mysql.h and using gcc for 
mysql_priv.h.


>
>> added:
>> include/mysql.h.pp
>> include/mysql/plugin.h.pp
>> sql/mysql_priv.h.pp
>> modified:
>> Makefile.am
>> configure.in
>> include/Makefile.am
>>
>> per-file messages:
>> Makefile.am
>> Added a rule abi_check that checks that the abi/api
>> has not changed.
>
> Why did you put that into the top-level "Makfile.am" ?

Are you saying you want one abi_check rule in include (for mysql.h and 
plugin.h) and another one in sql (for mysql_priv.h).


> IMO, the one in "include/" is the appropriate place, because we want 
> to check include files.

we are checking the storage engine API files, and mysql_priv.h is not 
part of the include directory. It made sense to have
a rule in the top level Makefile to check for all include headers that 
form part of the storage engine API. I do not want one
rule in each subdirectory that contains a header that forms part of the 
API. It would be redundant and a option that we add
to one rule, down the lane will need to be replicated in all these rules.

>
>> The following steps are followed in the do_abi_check rule
>> 1) Generate preprocessor output for the files that need to
>> be tested for abi/api changes. use -nostdinc to prevent
>> generation of preprocessor output for system headers. This
>> results in messages in stderr saying that these headers
>> were not found. Redirect the stderr output to /dev/null
>> to prevent seeing these messages.
>> 2) sed the output to 2.1) remove blank lines and lines that begin 
>> with "# "
>> 2.2) When gcc -E is run on the Mac OS and solaris sparc platforms it 
>> introduces a line of output that shows up
>> as a difference between the .pp and .out files. Remove
>> these OS specific preprocessor text inserted by the
>> preprocessor.
>> 3) diff the generated file and the canons (.pp files already in
>> the repository).
>> 4) delete the .out file that is generated.
>> If the diff fails, the generated file is not removed. This will
>> be useful for analysis of ABI differences (e.g. using a visual
>> diff tool).
>
> In your Makefile actions below,
> 1) you do not check the return code of "diff",
> or keep the output to check for non-empty,
> so your whole "if the diff fails" relies on "make" stopping when
> "diff" exits with a non-zero return code;

correct, and it does stop. I have checked that it stops! I have tested 
and encountered
this many times during my development of this patch and during 
development of WL#4454.

> 2) you let "make" terminate on the first checked file that shows a
> difference, and do not store that difference in any place explicitly,
> so it needs human analysis of the log file to find which input file
> caused it, and then human action to run some "diff" manually.

correct! For now the idea is to stop when there is a difference and 
leave the abi_check.out file. A
person will then use a manual diff to determine the change and get this 
change approved by the
Storage engine API team.

For now it needs human action everywhere. I plan to generate lot of 
noise when an API changes
(send emails to everyone in mysql screaming about an API change), for 
now any change in the
.pp files will show it as yellow in pushbuild and you cannot compile 
your code with a API change and
the .pp files unchanged.

>
> This means a simple "make -i" will bypass the ABI check completely,
> and the rules cannot be used (if we really want to change the ABI) to 
> process all checked files in one run - even "make -k" will not help.
You see this part of the code will be in flux until 4394 happens. The 
aim of this worklog is just to extend icheck
(which was not possible) for plugin.h and mysql_priv.h. This rule in its 
present format will offer the developer conviniences
like make -i. It has not been enforced very strictly. That is because we 
still do not know how it might affect end users.
We just know what pushbuild tells us.

I believe any code in mysql is run as part of pushbuild before being 
pushed into the mainline. So pushbuild will anyway
point out a build failure. The API team or any other developer can then 
look into the API change and take necessary steps.

>
> I propose to change both these aspects.
>
>> A ABI change that causes a build to fail will always be accompanied 
>> by new canons (.out files). The .out files that are not removed will 
>> be replaced as the new .pp files.
>
> The plural is wrong, there is only one ".out" file.
> This "replacement" is a manual action, the comment should make that 
> clear.
> Each replacement can handle only one ".pp" file (see above).
I agree, I will change this comment. I noticed this earlier and saw it 
as trivia, but as you say a developer might easily be
confused.

>
>> e.g. If include/mysql/plugin.h has an ABI change then this rule would 
>> leave a <build directory>/abi_check.out file.
>
> Exactly - and all info about the header file that caused it is lost.

I do not understand this comment. gcc is being run with the -dI option. 
This leaves header information. So a
diff tool like kdiff, meld would surely show up where the differences 
are and the header information left by
-dI can be used.
>
>
>> A developer with a justified API change will then do a
>> mv <build directory>/abi_check.out include/mysql/plugin.pp to replace 
>> the old canons with the new ones.
>> configure.in
>> Enable the abi_check rule only if gcc is available in the system
>> include/Makefile.am
>> Include the .pp files in the distribution
>> Remove the references to the old icheck target
>
> See above. I propose:
>
> 1) Explain why "icheck" does not do the job, either here or in the
> worklog (it isn't there either).
> 2) Explain why the "icheck" file should be deleted.

I will do this. I agree that the changeset comments should reflect this.


> 3) Move the actual "abi_check" rule into this file.

I think the top level Makefile is the right place for this. All the 
ABI/API checking rules
should be in one place. I do not think replicating this rule in sql/ and 
include/ is right.


>
>> include/mysql.h.pp
>> add the .pp files (containing the preprocessor output)
>> for mysql.h.
>> include/mysql/plugin.h.pp
>> Add the .pp file (containing the preprocessor output)
>> for plugin.h.
>> include/mysql_h.ic
>> remove the earlier icheck canon.
>> sql/mysql_priv.h.pp
>> Add the .pp file (containing the preprocessor output) for
>> mysql_priv.h.
>> === modified file 'Makefile.am'
>> --- a/Makefile.am 2008-06-03 10:21:48 +0000
>> +++ b/Makefile.am 2008-08-08 09:00:05 +0000
>> @@ -58,6 +58,8 @@ dist-hook:
>> --datadir=$(distdir)/win/data \
>> --srcdir=$(top_srcdir)
>>
>> +all-local: @ABI_CHECK@
>> +
>
> I do not see why "all-local" is a good name for this ...
> I have to call "make all-local" to get the ABI checked,
> and this target is a no-op for non-gcc compilers ? Not helpful.
>
> In any case, it should be declared ".PHONY".

I must confess I am not an expert on autoconf and automake. But from 
what I have read
I know that including it as part of all-local target does include it in 
the main build.

"Note that Automake does not make any difference between rules with 
commands and rules that only specify dependencies. So it is not possible 
to append new dependencies to an automake-defined target without 
redefining the entire rule.

However, various useful targets have a '-local' version you can specify 
in your Makefile.am. Automake will supplement the standard target with 
these user-supplied targets."

you do not have to call make all-local. It is run as part of the main build.


>> tags:
>> support-files/build-tags
>>
>> @@ -231,5 +233,81 @@ test-full-qa:
>> test-binlog-statement test-ext test-fast-view \
>> test-fast-cursor test-unit
>>
>> +#
>> +# Headers which need to be checked for abi/api compatibility.
>> +# API_PREPROCESSOR_HEADER will be used until mysql_priv.h stablizes
>> +# after which TEST_PREPROCESSOR_HEADER will be used.
>> +#
>> +
>> +API_PREPROCESSOR_HEADER = $(top_srcdir)/include/mysql/plugin.h \
>> + $(top_srcdir)/include/mysql.h
>> +
>> +TEST_PREPROCESSOR_HEADER = $(top_srcdir)/include/mysql/plugin.h \
>> + $(top_srcdir)/sql/mysql_priv.h \
>> + $(top_srcdir)/include/mysql.h
>> +
>> +#
>> +# Rules for checking that the abi/api has not changed.
>> +#
>> +# The following steps are followed in the do_abi_check rule below
>> +#
>> +# 1) Generate preprocessor output for the files that need to
>> +# be tested for abi/api changes. use -nostdinc to prevent
>> +# generation of preprocessor output for system headers. This
>> +# results in messages in stderr saying that these headers
>> +# were not found. Redirect the stderr output to /dev/null
>> +# to prevent seeing these messages.
>> +# 2) sed the output to +# 2.1) remove blank lines and lines that 
>> begin with "# "
>> +# 2.2) When gcc -E is run on the Mac OS and solaris sparc platforms it
>> +# introduces a line of output that shows up as a difference between
>> +# the .pp and .out files. Remove these OS specific preprocessor text
>> +# inserted by the preprocessor.
>> +# 3) diff the generated file and the canons (.pp files already in
>> +# the repository).
>> +# 4) delete the .out file that is generated.
>> +#
>> +# If the diff fails, the generated file is not removed. This will
>> +# be useful for analysis of ABI differences (e.g. using a visual
>> +# diff tool).
>> +#
>> +# A ABI change that causes a build to fail will always be accompanied
>> +# by new canons (.out files). The .out files that are not removed will
>> +# be replaced as the new .pp files.
>> +#
>> +# e.g. If include/mysql/plugin.h has an ABI change then this rule would
>> +# leave a <build directory>/abi_check.out file.
>> +#
>> +# A developer with a justified API change will then do a
>> +# mv <build directory>/abi_check.out include/mysql/plugin.pp +# to 
>> replace the old canons with the new ones.
>> +#
>> +
>> +abi_check: $(API_PREPROCESSOR_HEADER)
>> + $(MAKE) abi_headers="$^" do_abi_check
>> +
>> +abi_check_all: $(TEST_PREPROCESSOR_HEADER)
>> + $(MAKE) abi_headers="$^" do_abi_check
>
> IMO, this approach is wrong.
> You use "configure" setting the "@ABI_CHECK@" variable to empty or the 
> above target depending on whether "gcc" is used or not,
> but you have no handling of a non-gcc compiler in the real targets.
> Is it intentional that I can call "make abi_check" with any compiler ?

If you have a compiler that supports -nostdinc and -dI why would I stop 
the user from using this? I know icc and cc do not
support this. I do not know about other compilers.

Maybe the user wants to compile his code with other compilers, but wants 
to check abi/api change with gcc. I do not see
what I would disable this target for such users.


>
>> +
>> +do_abi_check:
>> + set -ex; \
>> + for file in $(abi_headers); do \
>> + @CC@ -E -nostdinc -dI \
>> + -I$(top_srcdir)/include \
>> + -I$(top_srcdir)/include/mysql \
>> + -I$(top_srcdir)/sql \
>> + -I$(top_builddir)/include \
>> + -I$(top_builddir)/include/mysql \
>> + -I$(top_builddir)/sql \
>> + $$file 2>/dev/null | \
>> + @SED@ -e '/^# /d' \
>> + -e '/^[ ]*$$/d' \
>> + -e '/^#pragma GCC set_debug_pwd/d' \
>> + -e '/^#ident/d' > \
>> + $(top_builddir)/abi_check.out; \
>> + @DIFF@ -w $$file.pp $(top_builddir)/abi_check.out; \
>> + @RM@ $(top_builddir)/abi_check.out; \
>> + done
>
> See my remark in the general section about these actions.
>
> Also, I am not sure the "-w" option for "diff" exists everywhere.
> It is not specified in the 2001 edition of IEEE 1003.1 ("POSIX").

I followed pushbuild here. I know -B and -b is not available in all 
platforms. But -w
is available in all platforms (pushbuild runs) in which gcc is available 
and this target is
run on.

>
>> +
>> # Don't update the files from bitkeeper
>> %::SCCS/s.%
>>
>> === modified file 'configure.in'
>> --- a/configure.in 2008-08-13 10:45:56 +0000
>> +++ b/configure.in 2008-08-13 14:32:27 +0000
>> @@ -419,6 +419,7 @@ AC_PATH_PROG(SED, sed, sed)
>> AC_PATH_PROG(CMP, cmp, cmp)
>> AC_PATH_PROG(CHMOD, chmod, chmod)
>> AC_PATH_PROG(HOSTNAME, hostname, hostname)
>> +AC_PATH_PROG(DIFF, diff, diff)
>
> Doesn't that introduce e dependency on "diff" for the builds ?
> A program with this name should exist everywhere, but still ...
>
> Considering your requirement of "-w", you should check for GNU diff.
>
> I propose to move this line (or a better version) down to the 
> ABI_CHECK handling.

I agree why stop a user from building if he does not have diff. But I 
really thought
all platforms will have a diff utility.

>
>> # Check for a GNU tar named 'gtar', or 'gnutar' (MacOS X) and
>> # fall back to 'tar' otherwise and hope that it's a GNU tar as well
>> AC_CHECK_PROGS(TAR, gnutar gtar tar)
>> @@ -443,25 +444,16 @@ AC_SUBST(HOSTNAME)
>> AC_SUBST(PERL)
>> AC_SUBST(PERL5)
>>
>> -# icheck, used for ABI check
>> -AC_PATH_PROG(ICHECK, icheck, no)
>> -# "icheck" is also the name of a file system check program on Tru64.
>> -# Verify the program found is really the interface checker.
>> -if test "x$ICHECK" != "xno"
>> -then
>> - AC_MSG_CHECKING(if $ICHECK works as expected)
>> - echo "int foo;" > conftest.h
>> - $ICHECK --canonify -o conftest.ic conftest.h 2>/dev/null
>> - if test -f "conftest.ic"
>> - then
>> - AC_MSG_RESULT(yes)
>> - else
>> - AC_MSG_RESULT(no)
>> - ICHECK=no
>> - fi
>> - rm -f conftest.ic conftest.h
>> +# Enable the abi_check rule only if gcc is available
>> +
>> +if expr "$CC" : ".*gcc.*"
>
> We currently use at least 3 (three) different ways to check for "gcc":
> 1) This "expr" construct,
> 2) a shell variable "ac_cv_prog_gcc",
> 3) a shell variable "GCC".
> IMO, approaches 2 and 3 are better than 1, and we might unify.

I saw all the three. I liked 1), checks for all versions of gcc. I can 
change if you insist
but I think 1) is OK and will work without glitches.

>
>> +then
>> + ABI_CHECK="abi_check"
>> +else
>> + ABI_CHECK=""
>> fi
>> -AC_SUBST(ICHECK)
>> +
>> +AC_SUBST(ABI_CHECK)
>
> I do not like that replacement mechanism, it is too cryptic.
> Why don't you set an "ABI_CHECK" variable to "yes" or "no" and then 
> use it explicitly in the Makefile, like was done for "ICHECK" which is 
> also missing on many platforms ?
Again, this is a result of my lack of expertise in autoconf best 
practices. I found this approach OK. But I can change it if you
think otherwise.

>
>>
>> # Lock for PS
>> AC_PATH_PROG(PS, ps, ps)
>>
>> === modified file 'include/Makefile.am'
>> --- a/include/Makefile.am 2008-06-18 16:17:15 +0000
>> +++ b/include/Makefile.am 2008-07-18 13:51:54 +0000
>> @@ -38,10 +38,11 @@ noinst_HEADERS = config-win.h config-net
>> atomic/rwlock.h atomic/x86-gcc.h atomic/x86-msvc.h \
>> atomic/gcc_builtins.h my_libwrap.h my_stacktrace.h
>>
>> +EXTRA_DIST = mysql.h.pp mysql/plugin.h.pp
>> +
>> # Remove built files and the symlinked directories
>> CLEANFILES = $(BUILT_SOURCES) readline openssl
>>
>> -EXTRA_DIST = mysql_h.ic
>>
>> # Some include files that may be moved and patched by configure
>> DISTCLEANFILES = sched.h $(CLEANFILES)
>> @@ -63,18 +64,5 @@ my_config.h: config.h
>> dist-hook:
>> $(RM) -f $(distdir)/mysql_version.h $(distdir)/my_config.h
>>
>> -#
>> -# Rules for checking that ABI has not changed
>> -#
>> -
>> -# Create a icheck file and compare it to the reference
>> -abi_check: $(HEADERS_ABI) mysql_version.h mysql_h.ic
>> - @set -ex; \
>> - if [ @ICHECK@ != no ] ; then \
>> - @ICHECK@ --canonify --skip-from-re /usr/ -o $@ $(HEADERS_ABI); \
>> - @ICHECK@ --compare mysql_h.ic $@; \
>> - fi; \
>> - touch abi_check;
>> -
>> # Don't update the files from bitkeeper
>> %::SCCS/s.%
>>
>> === added file 'include/mysql.h.pp'
> > --- a/include/mysql.h.pp 1970-01-01 00:00:00 +0000
> > +++ b/include/mysql.h.pp 2008-08-08 09:00:05 +0000
> > @@ -0,0 +1,674 @@
>
> I didn't check the file contents, as it is generated stuff anyway.
>
>>
>> === added file 'include/mysql/plugin.h.pp'
> > --- a/include/mysql/plugin.h.pp 1970-01-01 00:00:00 +0000
> > +++ b/include/mysql/plugin.h.pp 2008-06-17 12:27:04 +0000
> > @@ -0,0 +1,139 @@
>
> Unchecked as well.
>
>>
>> === removed file 'include/mysql_h.ic'
>> --- a/include/mysql_h.ic 2008-02-28 17:55:46 +0000
>> +++ b/include/mysql_h.ic 1970-01-01 00:00:00 +0000
>
> Why do you remove this file and all icheck information ?
> Is it totally useless, obsolete (unmaintained), or what ?
1) It is obsolete
2) It is not maintained
3) It cannot handle C++, which renders it useless for mysql_priv.h

>
>>
>> === added file 'sql/mysql_priv.h.pp'
>> --- a/sql/mysql_priv.h.pp 1970-01-01 00:00:00 +0000
>> +++ b/sql/mysql_priv.h.pp 2008-06-17 12:27:04 +0000
>> @@ -0,0 +1,10978 @@
>
> Again, I didn't check the file contents.
>
> But do you really think checking the "diff" of a 10,978 line file 
> against a previous version,
> and trying to judge whether the differences are ok or not,
> is a good way to ensure ABI stability ?
>
> I still think the special-purpose tool "icheck" would do this in a 
> better, more reliable way.

No. I believe more sophistication is underway in 4394. This rules as I 
told you was to extend icheck or
search for a suitable replacement. Which it does!

icheck does not support C++, It is a special purpose tool for C API/ABI 
checking.
>
>