Thanks once again for the detailed comments.
I really appreciate your attention to detail in the reviews.
Joerg Bruehe wrote:
> 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 ?
All I know is that ha_innodb.cc which includes mysql_priv.h seems pure C
to me. I just
grep'ed for class and template and this file seemed to be free of them.
> 2) If it should include C++ parts now, will it stay that way ?
If it includes C parts will it stay that way, considering it includes
mysql_priv.h?
> 3) What happens if you give a C++ header file to "icheck" ?
Here is a output of trying to pass mysql_priv.h to icheck
---------------------------------------------------------------------------------------------------------
icheck --canonify -o ../../test_projects/mysql_priv.ic -I./include
-I./sql/backup -I./sql ./sql/mysql_priv.h
./include/my_bitmap.h:123:2: syntax error while trying to parse
compound_statement
{
((bit < (map)->n_bits) ? (void) (0) : __assert_fail ("bit <
(map)->n_bits", "./include/my_bitmap.h", 122, __PRETTY_FUNCTION__));
(((uchar*)(map)->bitmap)[(bit) / 8] |= (1 << ((bit) & 7)));
^-- Here
}
static inline void
Failed to parse file ./sql/mysql_priv.h
------------------------------------------------------------------------------------------------------------
Another output of trying to parse my_bitmap.h
icheck --canonify -o ../../test_projects/my_bitmap.ic -I./include
-I./sql/backup -I./sql ./include/my_bitmap.h
./include/m_string.h:168:0: syntax error
extern char *strsuff(const char *src,const char *suffix);
extern char *strcont(const char *src,const char *set);
extern char *strxcat _VARARGS((char *dst,const char *src, ...));
^-- Here
extern char *strxmov _VARARGS((char *dst,const char *src, ...));
extern char *strxcpy _VARARGS((char *dst,const char *src, ...));
Failed to parse file ./include/my_bitmap.h
-------------------------------------------------------------------------------------------------------------
>
> 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.
I am surprised that you are willing to put your bet on icheck a tool
whose only reference of existence is
http://packages.debian.org/unstable/devel/icheck, which does not support
C++ and using which mysql_priv.h in its current form
cannot be parsed.
icheck target as far as I know has not been maintained since 5.0. When I
tried to run a diff between 5.0 and 5.1 it failed.
I know that mysql_priv.h is part of the storage engine API and any
change to it whether to the C/C++ parts should be caught. Whether the
re-organization will remove all the C++ code away from it and whether
then only icheck will suffice, I am not sure about. In its
present form mysql_priv.h cannot be used with icheck.
We should build our API/ABI checks for our API independent of the
storage engines using it.
So if mysql_priv.h is going to remain this way, icheck cannot be used.
>
> 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.
agreed!
>
>>
>> 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.
Actually I think if you are right about the running time of this rule
(which I am sure you are ;) ) it
does make sense to have it as a separate target.
But my reservations to doing this are outlined below.
I do not know to gauge the impact of this rule on the builds. All I know
is that it does not take much time in my system. But you
are the expert here.
I will change the rule to not run as part of the main build, basically
remove it from all-am. As long as it detects the changes
and generates the requisite noise, I am as happy as can be.
But then why did this never happen with icheck?
icheck was a seperate target right?
Changes in mysql.h were allowed to seep in, inspite of this header being
included as part of the icheck target?
As long as it runs as part of pushbuild and someone helps me with doing
this, I am OK.
>
>>
>>
>>>
>>>> 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.
what are the disadvantages of having it in the top level make file? We
are doing a abi/api check for the
storage engine API of MySQL and not for a set of files in include/.
>
> 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.)
I am not sure all the files will be in the include directory.
Are you proposing moving sql/table.h and sql/handler.h?
I am not sure the above headers will be moved so easily?
We might restructure mysql_priv.h but not the headers it depends on.
>
>>
>>>
>>>> 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.
icheck does not do that. icheck generates one monolith of a .ic file and
finally compares the .ic files. You are a fan of icheck
but you do not like gcc doing the same things ;) ?
I do not really like the idea of having one file for noting down all the
changes,
1) It is difficult to spot difference in a monolith
2) If a declaration is moved from 1.h to 2.h it is not one sin, it is two
sin 1: removing it from 1.h
sin 2: moving it into 2.h
Your contention is that sin 1 and sin 2 should be seen together, as
much as this would be good, it is a minor convenience
compared to the convenience offered by having individual diffs for
various files.
For example consider changing the signature of functions in plugin.h
then you would really not need the monolith
would you? The individual diffs would then offer a compact and clean
record of the differences and comparing the
abi_check.out file with the header would reveal the differences
immediately.
>
>>
>> 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.
agreed with my doubts outlined above.
>
>
>>
>>>
>>> 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.
agreed with my doubts outlined above.
>
>>
>>>
>>> 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.
I have tried to address this concern above. Also this is the way icheck
works, one .ic file per header.
>
>
>>
>>>
>>>> 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.
I do not agree on this one. I have explained above.
>
>>>
>>>
>>>> [[...]]
>>>
>>> 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.
agreed on this one.
>
>>
>>
>>> 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.
done my best already :)
>
>>
>>
>>>
>>>> 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").
If it has to be run as a separate target, it does not need to be
included as part of .PHONY also.
>
> 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.
I have my reservations about a summary file. I have outlined my
reservations above. I still think individual diffs and failure on the
individual
headers is easier to handle. The incremental approach seems cleaner to me.
I can work on this. But I really do not see anything wrong with
incremental failure reporting. As far as I know
make itself works that way no?
Tells you 1.cc has a mistake. You fix it and then it takes you to 2.cc.
Why can't this rule be the same way?
I am not saying I will not work on this. All I am saying is that
currently pushbuild does the same thing, reports
failures incrementally.
>
>>
>>
>>>> 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 ?
sorry got confused with -B not -b. -B is not available on sol10-sparc-a
https://intranet.mysql.com/secure/pushbuild/getlog.pl?dir=bzr_mysql-5.1-sea&entry=Narayanan_V__v_narayanan-20080722123700-1&name=build&plat=sol10-sparc-a
I used -w because -b is not enough for sol10-sparc-a. It still shows up
differences in that platform.
https://intranet.mysql.com/secure/pushbuild/getlog.pl?dir=bzr_mysql-5.1-sea&entry=Narayanan_V__v_narayanan-20080721104200-1&name=build&plat=sol10-sparc-a
Maybe I can change it because it becomes moot after disabling CC, but I
am not sure.
>
>>
>>>
>>>> +
>>>> # 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.
Will do!
>
>
>>
>>>
>>>> # 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).
Will work on b)
>
>>
>> [[...]]
>
>
> Jörg
>
