Thank you for the patient comments and guidance for this issue
Joerg Bruehe wrote:
> Hi,
>
>
> I have changed the subject to make it more obvious which aspects we
> are discussing in this subthread:
> Where (which platforms) and when (which build runs) to verify the ABI.
>
>
> Sergei Golubchik wrote:
>> Hi!
>>
>> On Aug 20, Joerg Bruehe wrote:
>>> Hi Sergei, all !
>>>
>>>
>>> Sergei Golubchik wrote:
>>>> Hi!
>>>> On Aug 18, Narayanan wrote:
>>>>> 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.
>>>> ...
>>>>> As long as it runs as part of pushbuild and someone helps me with
>>>>> doing this, I am OK.
>>>> But I'm not. I believe it must be run as a part of *every* build
>>>> (if the
>>>> prerequisites are met, of course).
>>> Assuming "every build" is meant to include the release builds, I
>>> *very* strongly disagree:
>>> 1) This check is done on the source code, and its result should be
>>> independent of the platform. Why do it more often than needed ?
>>
>> Taking into account the amount of #ifdef's in our code, I cannot believe
>> that you can seriously insist on running the check on one platform only
>> :)
>>
>> You know better than anybody else that running a build or a test suite
>> on one platform is not enough. Why would that be enough for abi_check ?
>
> I had assumed at least the modules that define the ABI are free of
> "#if" and related constructs.
>
> My approach was that the check should be run during development
> (possibly on the developers' machines) and during pushbuild,
> I had assumed that is sufficient coverage.
>
> So you think we should try to run it on every platform, provided we
> can make the compiler generate the check file ?
> That is a possible approach, but it implies we should make the
> commands (compiler options) configurable as much as possible (see below).
>
>
>>
>>> 2) If this check is done in release builds, it is simply too late.
>>> Any changes in the API/ABI need to be caught in pushbuild, and then
>>> either be rejected and fixed, or approved explicitly.
>>> How should the build team react if the check fails ?
>>
>> How should the build team react if there's a compilation failure ?
>> It's the same thing. And neither should happen in release build.
>
> You have a point.
> I had simply hoped our header files (for the API/ABI) are not so full
> of confitional compiling, so fewer platforms would suffice.
>
>
>>
>>> 3) If this check is done in release builds, in all configurations,
>>> it is
>>> just a waste of time and disk space.
>>
>> Waste of how much time and how much space ? It's far less than 1 per
>> cent of either.
>
> I have not taken any time measurements. VN, do you have any ?
I did a "time make abi_check", below are three time measurements
real 0m0.235s
user 0m0.052s
sys 0m0.004s
real 0m0.164s
user 0m0.052s
sys 0m0.016s
real 0m0.166s
user 0m0.056s
sys 0m0.024s
>
>>
>> Anyway, the point is not to make release builds more expensive. The
>> point is that it should be run on developer's boxes. For every WL, every
>> bugfix, every source code change. If pushbuild detects it - it's too
>> late, a bad changeset is already pushed. It's the same thing as
>> requiring a developer to build the tree and run the test suite before
>> pushing, although, in principle, pushbuild can do that. And doing it on
>> one platform only should be enough, right ? :)
>>
>> As for release builds, the added cost is so minor that there's nothing
>> to argue about, really.
>
> In a release build, we do it six times (for the different
> configurations).
>
> I would like to know the build times (compile + link, no tests) both
> with and without the ABI check.
> The answer should tell how important it is to avoid repeating the
> check when building different configurations of the same version on
> the same platform.
If I have done the right things in finding the time, I think this is not
much.
>
> *If* the time needed is considerable, it might be good to have a way
> of disabling the check (by not making it ".PHONY", so that a dummy
> output file would let "make" not run it).
> If it isn't, I will wthdraw my reservations.
>
>>
>>>>>> 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!
>>>> Is there a practical need to do that ? Any single host where we
>>>> have gcc
>>>> and diff -w doesn't work ?
>>> We use gcc on OS X and (sometimes) on Solaris, and should not block us
>>> from using it on other platforms as well.
>>
>> Right, but VN has this WL pushed in the team tree, and it's tested on
>> all pushbuild platforms already. As far as I remember this includes
>> Solaris and OS X with gcc.
>
> It is in the team tree before having two reviews ? No comment.
I thought Serg had super powers, and that his review was enough.
>
> The stronger you argue to "run it on as many platforms as possible",
> the more important it gets to make the "diff" binary configurable.
> Currently, "diff" output for the same file difference in the test
> suite differs by platform. (A weird sentence, but you get the meaning.)
But I have made it uniform across all platforms in pushbuild. I have
been struggling to get this working
on all platforms for the past month.
>
>>
>>>>>> 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)
>>>> Again, any practical need to do that ? Any compiler besides gcc that
>>>> supports a functionality identical to "-E -nostdinc -dI" ? Or, at
>>>> least,
>>>> to "-E -nostdinc" ?
>>> I didn't check. My proposal is not based on a currently perceived
>>> possibility, rather on it being more flexible.
>>
>> A perceived possibility of a hypothetical compiler that supports
>> -E -dI -nostdinc ? I'm afraid, it's over-engineering, adding the
>> complexity without necessity.
>
> Same argument as above: If you would like to cover all platforms, then
> include the mechanism now, so that we can add other compilers if we
> detect the need and the possibility (by adding a branch then, not
> changing the general approach).
We can always change the autoconf rule to do what we want, can't we?
>
>>
>>> Serg,
>>> to me your remarks appear to be somewhat contradictory:
>>>
>>> - On one side, you "believe it must be run as a part of *every* build
>>> (if the prerequisites are met, of course)",
>>> - while you also propose to check the "practical need" before doing
>>> more
>>> general mechanisms.
>>>
>>> If the easy implementation ("gcc" only, "diff -w" assumed) is good
>>> enough, why then in every build (including release builds) ?
>>
>> To have it run on developer's boxes, see above.
>
> Seems I didn't put the question sufficiently clear, but it should be
> covered by the above remarks.
>
>
> Regards,
> Jörg
>
