Hi Narayanan, Sergei, all !
I cc: Sergei, as he showed a distinct interest.
He has already commented on some parts, and I have replied to that
separately.
Narayanan wrote:
> Thanks once again for the detailed comments.
>
> I really appreciate your attention to detail in the reviews.
Pure egoism: In the build team, I may have to stand the consequences ;-)
>
> Joerg Bruehe wrote:
>> Hi !
>>
>> [[...]]
>>
>> 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
>>>>
>>>> [[...]]
>>>
>>> Decided to remove icheck because
>>>
>>> [[...]]
>>
>> [[...]]
>>
>> 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
> [[...]]
>
> Another output of trying to parse my_bitmap.h
>
> [[...]]
>
Ok, you explained why "icheck" doesn't do the job with our current code.
I accept that, it means your "gcc -E" approach is more general and works.
I still propose not (yet) to delete the "icheck" rule from
"include/Makefile", as it might be useful if
a) we change to pure C,
b) we want to protect some other interface which is pure C (client
library !).
That applies to the rule only, not to the generated file (as I
understand you say its contents is by now wrong anyway).
>
>>
>> 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.
Accepted - see above.
>
>>
>> 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.
Ok, agreed then.
>
> 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.
I assume the separate "icheck" target was not run, and for "abi-check"
we need to prevent that.
You are right that doing the ABI check from the main build run is a good
way (probably the best) to ensure it cannot be forgotten.
OTOH, I am convinced there are cases where such a check is simply too
late or isn't useful for other reasons, release builds (all
configurations !) is just one of them.
I have written about this in my reply to Serg, so I propose to pursue
this in that other subthread.
For example, we might make it part of "Bootstrap" which is called once
when we start a release build.
For pushbuild, we should also find a way to run it once.
>
>>
>>>
>>>
>>>>
>>>>> [[...]]
>>>>>
>>>>> per-file messages:
>>>>> Makefile.am
>>>>> Added a rule abi_check that checks that the abi/api
>>>>> has not changed.
>>>>
>>>> [[...]]
>>
>> 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.
Right, for the storage engine API, as defined by include files.
That is what makes me feel it belongs into "storage/Makefile" or
"include/Makefile".
The "storage/" or "include/" directory would then hold both the check
(result) files and the rule, this looks consistent.
I know some files are currently stored in other directories, but the
check rules could access them from there as well IMO.
Currently you access files from directories below that with the rule, in
my proposal you would access files from directories beside the current one.
But if you feel strongly that going down is easier/better than going to
the side, so be it.
>
>
>>
>>>
>>>>
>>>>> The following steps are followed in the do_abi_check rule
>>>>> [[...]]
>>>>
>>
>>>
>>>> 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 ;) ?
AIUI, icheck combines the output of all header files in one .ic file so
it does the combined check of all headers. Agreed, I think individual
result files are better than a monolithic one.
>
> 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.
It seems I did not express myself as clear as I should:
I do *not* propose to have one big monolithic file with the result, I am
fine with (or rather: I also prefer) having separate ones (one by header
file checked).
But I do want to have one run that creates all these files, not stop at
the first one that differs.
Above, you write that moving a declaration "from 1.h to 2.h it is not
one sin, it is two".
Assuming that both header files must be included (maybe: they are both
included by a common, mandatory .h file), I do not necessarily see that
as a sin, it may be a proper cleanup / restructuring.
Definitely, it would not change the ABI (if the declaration itself isn't
changed) - for linking, the names and types matter, but not the include
file(s) they come from.
Only if the run created all those check files, the developer(s) can
determine whether the change is legal or not.
If the run stops after 1.diff got created, the only way to make it
proceed to 2.h is to accept the change in 1.h by modifying the 1.check
file - knowing "Oh yes, I moved that from 1.h to 2.h".
Then, in the next run, 2.diff is created, and the developer again thinks
"Oh yes, I moved that to here".
In this schedule, there is *no* point where 1.diff and 2.diff
simultaneously exist - so if the move contains some other change (by
accident, a copy-paste error), this will go undetected.
We need that *simultaneous* existence of 1.diff and 2.diff for the
"Storage engine API team" to check for such copy-paste errors.
>>
>
> [[...]]
I deleted sections where we agree, or that were discussed above.
>
>>
>>>
>>>
>>>>
>>>>> [[...]]
>>>>> === 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.
I need to improve my knowledge there, too. I recently found good stuff:
http://www.gnu.org/software/libtool/manual/automake
http://www.lrde.epita.fr/~adl/autotools.html
>>>
>>> [[...]]
>>>
>>> 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.
See my reply to Serg.
Also see there for *not* making it .PHONY if we want a "touch abi-check"
to prevent this in release builds.
We should discuss that in the separate subthread, IMO the question
"which builds should include this check?" can (and should) be handled
separate from "how do we implement the check?".
>
>>
>> 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?
1) You can override that immediate stop by "make -k", if you like.
This is very useful when you want to fix several compiler/linker
bugs in one go before the next "make", very important if each "make" has
a long startup time (preprocessing ?) before reporting the error(s).
2) See above why 1.diff and 2.diff must be available simultaneously.
>
> 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.
This depends on the failure:
1) It runs on all platforms in parallel, does not stop after a failure.
2) It runs all test suites, even if one had failures.
>
>>
>> [[...]]
>>
>>>
>>>
>>>>
>>>>> +
>>>>> +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
>
That explains why you delete all-blank lines.
OTOH, one could also keep them, and then manually accept a change that
affects only such lines - matter of (developer) taste, and preference of
the Storage Engine API team.
>
>
> 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
>
Yes, "-b" will still see a difference between "void*" and "void *".
OTOH, "-B" will not see a difference between "void" and "vo id", but
this is something the compiler will catch.
>
>
> Maybe I can change it because it becomes moot after disabling CC, but I
> am not sure.
All in all, I do not see any problem in the commands you use.
>
>
>
>>
>>>
>>>>
>>>>> +
>>>>> # 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!
Ok, thanks.
>
>>
>> [[...]]
>>
>>>
>>>>
>>>>> +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)
Thanks.
Regards,
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
