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

[Jose Fonseca]

----- Original Message -----
> 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.

The mere size of these files (several ten thousand lines) causes them to be blocked in mailing lists, which means that nobody will review before checking in, and one needs to manually exclude them in review requests. I don't see how this is handy.

I'd also prefer a bit more consistency, not only with what's being pushed on Mesa, but piglit itself -- there are a lot of generated piglit test cases which are not checked in.  Is the plan to check them all in as well?

Jose