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
--
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
