Thanks once again for the detailed reply Joerg!
I appreciate your patience in dealing with all the related issues of
this worklog.
>>
>
> 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).
This differs from our previously reached conclusion of having only one
rule for all api/abi checks. I really do not like
the idea of having to maintain two separate rules for api/abi checks. We
then need to keep checking for alerts from
icheck as well as the gcc rules. This will only add to the confusion. If
gcc -E works it will work for all cases and we can
stick to that for everything.
For reference:
>
>>> Agreed that we need not use two similar mechanisms in parallel.
>>>
>>
>> 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.
Sergei has some nice points for running it in all builds in his thread.
I please request that this discussion be taken to the other thread.
>
>>
>> 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.
OK to taking it to the other thread.
> But if you feel strongly that going down is easier/better than going
> to the side, so be it.
I sincerely feel it will be managed much better in the top level
makefile. I please request that it be retained in the top level
makefile.
>
>> 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.
I really see each of these headers as separate, standalone, building
blocks of the storage engine API. You cannot move a header from mysql.h
to plugin.h
and call it a related change. You can however change from a header that
is included inside mysql.h to another inside mysql.h. This will be
surely seen apart of
the single mysql.h.pp file.
I do not think that the justification that we move from 1.h to 2.h to be
valid here because you cannot move between the headers (mysql.h, plugin.h or
mysql_priv.h). That is not agreeable. They have to be seen separately.
You can move within the headers. That is anyway being seen as a single unit.
>
> 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.
True, but it still qualifies as a change not approved by the storage
engine API team.
> 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.
I think the confusion is because we are still seeing things as header
files that are linked to each other. See plugin.h is different from
mysql.h which is different from mysql_priv.h.
You change within mysql.h it is seen in mysql.h.pp. You change from
mysql.h to plugin.h they are two separate entities of the API changing.
This is not possible and
even if possible each will have its own semantics.
They are placed in different directories because their semantics are
entirely different
mysql.h - include/
plugin.h - include/mysql/
mysql_priv.h - sql/
>
> 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.
No copy paste change from one header to another or even in the same file
will go undetected with this rule. The rule will fail and the user
*will* have to fix the .pp file.
If it is overlooked and the files are simply replaced, we the developers
will be informed of the change to the .pp file and the storage engine
API team will have to look closely
at the API file and catch it.
>
> We need that *simultaneous* existence of 1.diff and 2.diff for the
> "Storage engine API team" to check for such copy-paste errors.
Actually to detect the copy paste errors you need to do a bzr diff on
the file to find out the difference between the present version and the
previous version of the file. That would be the most easy way of
figuring out the problem.
>
>>
>
> 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?".
In our case if we decide that all builds should include these checks
this target should form a part of .PHONY or all-am. Otherwise it just
needs to be a standalone target.
IMHO they are related questions. I please request that they be treated
in sync with each other.
>
>>
>>>
>>> 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 have disagreed to this above. Pls refer to above explanation.
>
>
>>
>>
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> # 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.
Sergei has opposed this in his email. This discussion still continues in
the other thread. I request to please revoke my consent on this previously
and please request that the discussion on this be continued in the other
thread.
>
>>
>>>
>>> [[...]]
>>>
>>>>
>>>>>
>>>>>> +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.
Again this has been handled in the other thread. I please request to
revoke my consent here also and please allow this discussion to
continue in the other thread.
>
>
> Regards,
> Jörg
>
