Thank you for the reply Joerg, I will stick to Serg's explanation for
this email.
Joerg Bruehe wrote:
> Hi,
>
>
> I have changed the subject to make it more obvious which aspects we
> are discussing in this subthread: How to best do it ?
>
> The "where and when" are discussed in the other subthread.
>
>
> V Narayanan wrote:
>> [[...]]
>>
>>>>
>>>
>>> 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 [[...]]
>>
>>
>> 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.
>
> Ok, I withdraw my proposal.
> I had not meant to maintain it, my idea was to protect it against
> being forgotten completely
> (more like a comment "previously, we did it this way ...").
>
> The chances to revert back to "icheck" are slim anyways.
>
>>
>> [[...]]
>>
>>> 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.
>
> Ok, accepted.
>
>>
>>>
>>>> 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.
>
> This is the point where you do not convince me.
>
> I do not claim such a move would be acceptable in general, not at all.
> However, I expect sometimes (rare !) there will be such cases.
>
> So I do request that for any change the differences of all checked
> header files be available simultaneously, for examination.
>
>>
>>>
>>> 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.
>
> Of course it would need such an approval, and maybe it won't get it.
> All I say is that the API team must be given all differences
> simultaneously, not piecemeal.
>
>>
>>> 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/
>
> I am not convinced that our split of different aspects into different
> header files is so perfect that we will never change it.
> Because I anticipate such changes, I want them all checked in one run.
>
> Consider, for example, that "storage/" is a new layer which got
> introduced in 5.1; up to 5.0, we had the engines (then: bdb/, heap/,
> innobase/, myisam/, myisammrg/, and ndb/) contained directly in the
> top directory.
> If we do such a change again, or a similar one, we need the "grand
> view" to check we didn't break anything by accident.
>
> There is another possibility we didn't mention yet: Related changes.
> Assume we add a function to the ABI, or we are forced to remove one ?
> Wouldn't that also quite possibly mean related changes in several
> header files ? Again, we need the simultaneous existence of all diff
> files.
>
>>
>>>
>>> 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.
>
> Of course each individual change will be detected, but the two related
> ones cannot be checked (on the .pp files) as a whole.
>
>> [[...]]
>>
>>>
>>>>
>>>
>>> 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.
>
> Agreed, and part of the other subthread.
>
>>
>> [[...]]
>>
>
> Regards,
> Jörg
>
