Hi !
Again, comments are inline.
Narayanan wrote:
> Joerg Bruehe wrote:
>[[...]]
>>
>>
>> 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.
>> [[...]]
>
> 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.
Well, I will not comment on the structure of "mysql_priv.h" in detail
here (other than stating that the common part for server and client
*definitely* belongs into a separate file, included into both sides).
But what I would like to know:
1) Does the storage engine API/ABI, as used by InnoDB and in general,
include C++ parts or is it pure C ?
2) If it should include C++ parts now, will it stay that way ?
3) What happens if you give a C++ header file to "icheck" ?
As you see, I am very reluctant to lose our current "icheck" information
unless you can really convince me it will not do the job in the long run.
In the text of WL#4380, you removed all references to "icheck" (both in
title and text) without explaining. IMO, the reasoning needs to be
documented so that it is available for examination.
>
> 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.
Agreed that we need not use two similar mechanisms in parallel.
However, I do *not* see a need to run the abi_check with every build (on
the target platforms !) where it is possible.
All this does is
1) raise the build turnaround time (we do build up to 6 different
configurations, so the check would be done 6 times),
2) introduce the risk to have a false positive on the target platform
("diff" is quite different across our hosts) and so make a release
build fail.
I will resume this aspect further down.
>
>
>>
>>> 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).
Not at all - see below.
>
>
>> 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.
I fully agree to have one rule only, I'm just not convinced of having it
in the top Makefile.
I feel this is about header files, so "include/" might be the better
place, even if it has to reference files from other directories (for now).
I still have hope this might be restructured, so that all affected files
are in one directory only.
(As this is about the storage engine ABI, "storage/" would be a
candidate as well, but this seems not to fit the current file locations.)
>
>>
>>> The following steps are followed in the do_abi_check rule
>>> [[...]]
>>
>> 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.
I still consider it risky to rely on that "make" behavior, but that
becomes moot - see next comment.
>
>> 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.
That approach isn't good:
Assume some declaration is moved from one include file to another, both
being subject to your check (or an include file is split, or ...).
To check (by a human) that this is a valid change (or to find it is
invalid), the full set of differences must be available, and of course
it must be generated by the Makefile action.
So you must ensure that *all* files are compared, and the rule finally
fails if *any* file showed a difference.
>
> 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 is simply not fit for a rule which is to be executed with every
release build.
Imagine some user takes the source and does modify the ABI - would you
really want him to automatically send mail ?
This ABI check must be done independent of a normal build, as a separate
action.
>
>>
>> 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.
You don't really intend pushbuild to test that on every target platform
(which has gcc), I hope. Already now, pushbuild runs take too long.
As this is a static test based on source code text, one such check for
every push is fully sufficient.
Change it so that it is a rule which can be called on its own,
and pushbuild needs to call this rule once per push.
>
>>
>> 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.
Thinking more about this, I can just refer to my above remark:
The structure of the rule needs to be changed so that it handles all
files in one run, and then fails at the end if any file changed.
>
>>
>>> 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.
See above - one run of the rule needs to check all controlled files.
>>
>>
>>> [[...]]
>>
>> 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.
Ok, this demand is already mentioned close to the start of this mail.
>
>
>> 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.
Agreed on having it only once, but this does not determine the placement.
If you really feel putting it into a subdirectory (not the top) has
disadvantages, I will agree to the top - but you need to convince me.
>
>
>>
>>> 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.
I have explained above why it must not be part of the main build action.
As regards .PHONY:
A Makefile target should be declared .PHONY if it doesn't correspond to
a file, that's what I took from "info make" (node "Phony targets").
OTOH, that may become moot when you change the rule to handle all files,
depending on whether you create a summary file (the target) or not.
IMO, it might be easiest to loop over all to-be-checked header files,
processing them and running "diff" (keeping the result files), appending
result statistics ("ls -l" and diff's return code, for example) to a
status file, and then checking that status: The rule passed if the
status file contains success for all checked headers.
Of course, you may have better ideas how to loop over several files and
keep track of individual differences.
>
>
>>> tags:
>>> support-files/build-tags
>>>
>>> @@ -231,5 +233,81 @@ test-full-qa:
>>> [[...]]
>>> +
>>> +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.
You are right, I agree tho that.
>
>
>>
>>> +
>>> +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.
Option "-b" is much more widespread than "-w", it already appears in the
1988 edition of the X/Open Portability Guide.
Which pushbuild aspect did you follow ?
>
>>
>>> +
>>> # 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.
They have something called "diff", but I would not rely on it supporting
"-w" (above):
1) Copy the (g)tar rule, so that we can provide a "gdiff" if needed.
2) Put it into the section where you checked for "gcc", there is no need
to look for "diff" if we don't run the abi_check on that platform.
>
>>
>>> # 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)
>>> [[...]]
>>> +# 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.
No, I don't insist on that.
>
>>
>>> +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.
Now that I know about you relying on "all-local", this is irrelevant.
IMO, there are two choices:
a) Have "@CC@ -E -nostdinc -dI" hard-coded, like it is now (which means
we can only do it on gcc platforms), or
b) if it is "gcc", set some variable ("@ABI_CHECK@" ?) to the gcc line,
otherwise set it to "no" or some other string.
With approach a), it is the user's responsibility not to call "make
abi_check" with a compiler that doesn't accept these options,
with b), we (or some other user) could expand "configure.in" by adding
settings for other compilers.
As you can tell, I would prefer b), but I won't reject a).
>
>[[...]]
Jörg
--
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
