[Piglit] [PATCH v2 08/13] piglit-dispatch: Add generated files.

[Chad Versace]

On 03/13/2012 09:42 AM, Jose Fonseca wrote:
> ----- Original Message ----- 
> 
>> On 12 March 2012 15:08, Jose Fonseca < jfonseca at vmware.com > wrote:
> 
>>> ----- Original Message -----
>>
>>>> This patch adds the files glapi/glapi.json,
>>
>>>> tests/util/generated_dispatch.c, and
>>>> tests/util/generated_dispatch.h,
>>
>>>> which were generated using the command "make
>>>> piglit_dispatch_gen".
>>
>>>> ---
>>
>>>> glapi/glapi.json |55751
>>
>>>> +++++++++++++++++++++++++++++++++++++++
>>
>>>> tests/util/generated_dispatch.c |39920
>>>> ++++++++++++++++++++++++++++
>>
>>>> tests/util/generated_dispatch.h |14993 +++++++++++
>>
> 
>>> Do any of these files really need to be commited, given the source
>>> .spec have been committed too?
>>
> 
>> That's a good question, and I don't really know the best answer. See
>> below for more discussion.
> 
>>> If so, have you ensured that everytime they are regenerate they
>>> (Care
>>> must be taken with python dictionaries as they easily lead to
>>> variable ordering).
>>
> 
>> Yes, I was careful about this in the code generation script. All the
>> output of the code generator is sorted so that it will happen in a
>> consistent order and so that the diffs will be reasonable when the
>> generated code changes.
> 
>>> But I feel that either the derived result is committed (and the
>>> source is left out and manually downloaded when upstream changes)
>>> or
>>> the sources (and the derived files generated during the build).
>>> Including both seems redundant.
>>
> 
>>> Jose
>>
> 
>> Yeah, I agree that what I've put on the mailing list seems redundant.
>> It seems like we have three options on the table:
> 
>> (1) Leave the patches as they are.
> 
>> CON: Seems redundant.
> 
>> CON: There's a danger that someone will edit the source files (or the
>> generator script) and forget to re-run code generation.
> 
>> (2) Don't check the generated code into CMake; instead generate it at
>> build time.
> 
>> PRO: Since this forces everyone to do code generation all the time,
>> it will help ensure that the code generation script is platform
>> independent and doesn't accumulate bugs. Also it ensures that I'm
>> not the only person who knows how to run the code generator and
>> remembers to do so :)
> 
>> PRO: There's no danger of someone accidentally editing the generated
>> files and checking in the results.
> 
>> CON: I'm not 100% sure how to program the correct dependencies into
>> CMake, to ensure that (a) the code generation script runs before any
>> C files are built, and (b) all C files are rebuilt after the
>> generated code changes. (Chad helped me get this set up on an early
>> draft of the code but I was unable to keep it working properly).
> 
> Apitrace does a lot of code generation with cmake. Feel free to look into https://github.com/apitrace/apitrace/blob/master/CMakeLists.txt as inspiration. For example:
> 
>     add_custom_command (
>         OUTPUT glxtrace.cpp
>         COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/glxtrace.py > ${CMAKE_CURRENT_BINARY_DIR}/glxtrace.cpp
>         DEPENDS glxtrace.py gltrace.py trace.py specs/glxapi.py specs/glapi.py specs/glparams.py specs/gltypes.py specs/stdapi.py
>     )
> 
> You jut need to ensure that all DEPENDS args are there. When generating .h files, you also need to ensure that ${CMAKE_CURRENT_BINARY_DIR} is in the include path.
> 
>> CON: When changing the code generation scripts (or the .spec and .tm
>> files), more work will be required to manually verify that the
>> generated code is correct, because the generated code won't show up
>> in git diffs.
> 
> It's a test suite. I suppose tests would suddenly fail/skip if there's a bug here, so it should be easy to catch this.
> 
>> (3) Don't check the source files into CMake; instead download them
>> and manually rebuild whenever upstream changes.
> 
>> CON: the source files from opengl.org contained errors that I had to
>> manually correct, and some information was missing (see patches 5
>> and 6 in the series); if we take this approach then I'm not sure
>> where to store corrections and additions.
> 
> Fair enough. (3) doesn't work.
> 
> This is also the reason why I don't code generate from Khronos .spec .tm in apitrace neither, as apitrace needs a lot of extra manual info that's not included there.
> 
>> CON: if the OpenGL folks ever decide to drop the .spec and .tm files
>> from their web site, we will have to scrounge to find copies of
>> them.
> 
> 
>> Personally I have a slight preference for sticking with approach (1)
>> for now (in the interest of getting these patches into the codebase
>> soon) and experimenting with approach (2) as a follow-up patch
>> series. But I could be talked into changing my tune. I'd like to
>> hear more opinions.git 
> 
>> Ian, I recall that you had some strong opinions about whether
>> generated code should be stored in source control. Would you like to
>> weigh in on the discussion?

I also prefer (1) to (2), but could be convinced of (2).
A nice benefit of (1) is that the git log would reveal, in a easily understandable form,
the changes to the generated C sources. The git log of the generator script is not as enlightening.
 
----
Chad Versace
chad.versace at linux.intel.com