Hi !
I got asked to look at this changeset,
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.
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" ?
> 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" ?
IMO, the one in "include/" is the appropriate place, because we want to
check include files.
>
> 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;
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.
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.
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).
>
> 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.
>
> 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.
3) Move the actual "abi_check" rule into this file.
> 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".
> 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 ?
> +
> +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").
> +
> # 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.
> # 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.
> +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 ?
>
> # 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 ?
>
> === 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.
--
Joerg Bruehe, MySQL Build Team, joerg@stripped (+49 30) 417 01 487
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Haering Muenchen: HRB161028
