[Mesa-dev] [PATCH] mesa: Remove the generated glapi from source control, and just build it.
Paul Berry
stereotype441 at gmail.com
Wed May 23 16:35:32 PDT 2012
On 23 May 2012 13:33, Ian Romanick <idr at freedesktop.org> wrote:
> On 05/23/2012 12:45 PM, Eric Anholt wrote:
>
>> Mesa already always depends on python to build. The checked in
>> changes are not reviewed (because any trivial change rewrites the
>> world). We also have been pushing commits between xml change and
>> regen where at-build-time xml-generated code disagrees with committed
>> xml-generated code. And worst of all, sometimes we ("I") check in
>> *stale* xml-generated code.
>>
>
> <broken_record>
> Though the person making the changes will typically review them. I renew
> my objection that making tiny changes to the generator scripts can result
> in catastrophic changes in the generated code. We've encountered several
> situations where changing one part of the generator causes sizes of broad
> categories of GLX protocol messages to be miscalculated. git-diff makes
> this easy to notice. "Wait, why did indirect_size.c change at all? That
> has nothing to do with the change I intended."
>
> Note also that some of the generated code is platform-specific and gets
> little testing. How often does anyone test the C-based dispatch code? Or
> the SPARC dispatch code? Or even the 32-bit x86 dispatch code?
>
> I know Paul asserts people can diff the files by hand. I assert that,
> while they can, nobody will do that ever. People will look for changes in
> the files that they expected to change. Given the size of the potential
> set of changed files, I expect that change in unexpected files will go
> unnoticed.
>
I wasn't going to get involved, but I'd rather have my direct opinion in
the thread rather than hearsay :) So here's my two cents:
<soapbox>
IMHO we need to distinguish between changes to the code generation scripts
vs. changes to the source files containing the input data to the code
generation scripts. Changing the code generation *scripts* is always
risky, and anyone who modifies the code generation scripts should review
the generated results to avoid unexpected changes. Changing the code
generation *input files*, on the other hand, ought to be low risk, and if
it isn't then that's a flaw in our code generation scripts (after all, one
of the primary purposes of code generation is to reduce the risk associated
with boilerplate code).
I believe that the assertion that "nobody will diff the generated files by
hand" is true for people who change the code generation input files, and
that's as it should be. People who change the code generation scripts
*should* diff the generated files, and they do (I do, anyhow, and I assume
Ian does). If anyone isn't responsible enough to do that we should let
them know that's unacceptable, just as we would let someone know that it's
unacceptable not to do a build before pushing your patches, or not to do
regular full piglit test runs. Checking generated files into git as a way
of forcing people to do a responsible amount of review of their own patches
is a technical solution to a social problem, and those don't generally work
out well.
But the real reason I'm against putting generated files in git is that it
runs against two default assumptions made by programmers. Firstly, we
assume that the things in source control are all "source files" in the
sense of being, to quote the GPL, "the preferred form of the work for
making modifications to it". Breaking this assumption means that
periodically someone will accidentally make manual changes to a generated
file, and those changes will disappear the next time the file is
regenerated. Secondly, we assume that once we have modified a source file,
all we need to do to cause our changes to take effect is to do a build (or,
in the all-too-common case of a project with broken makefile dependencies,
a clean build). Breaking this assumption means that periodically someone
will change a source file, forget to run the appropriate manual build step,
and as a consequence the intended change will not take effect. Or worse
yet, someone will change a source file and only rebuild the generated file
that they expect to be affected by it, unaware that they have inadvertently
broken some other generated file. The breakage to the other generated file
may not be detected until months later, when someone else decides to run
the manual step to regenerate it. It's my understanding that all of these
kinds of mistakes have happened within the last six months due to our
keeping generated files in source control. I think that violating
programmers' core assumptions about source code has been causing far more
mistakes than we've been preventing by having changes in the generated
files show up in git-diff.
Finally, I think we can have our cake and eat it too. For those of us who
are concerned about unexpected changes slipping into, say, inderect_size.c,
let's set up a daily script that builds the generated files we are
concerned about and emails concerned parties with a summary of which files
have changed since the previous day. I think this will be even more
effective than having changes to generated files show up in git-diff
because (a) it will catch changes to all generated files, not just the ones
that the programmer remembers to regenerate, and (b) it will catch changes
made by anyone, not just those of us with the discipline to inspect the
output of git-diff carefully and the knowledge to distinguish expected
changes to the generated files from unexpected ones.
</soapbox>
>
> Nobody runs indirect rendering tests. Therefore we have no testing of
> code that we might not even notice we were changing. That seems just about
> as awful as possible.
> </broken_record>
>
> I fully agree that the current situation sucks. However, the new
> situation will also suck. I reserve the right to say "I informed you
> thusly" the next time one of these kinds of bugs occurs. :p
>
>
> ---
>>
>> I dropped the giant file removals from the diff output to fit on the
>> mailing list.
>>
>> This doesn't fix the make clean requirement in Mesa, because
>> makedepend appears to be just plain broken. However, my upcoming
>> automake series does fix it by not using makedepend. Modifying glapi
>> and typing "make", woo!
>>
>> configure.ac | 2 +-
>> src/glx/.gitignore | 6 +
>> src/glx/indirect.c |10736 -----------
>> src/glx/indirect.h | 722 -
>> src/glx/indirect_init.c | 789 -
>> src/glx/indirect_size.c | 382 -
>> src/glx/indirect_size.h | 85 -
>> src/mapi/glapi/.gitignore | 9 +
>> src/mapi/glapi/gen/Makefile | 3 +-
>> src/mapi/glapi/glapi_gentable.**c | 9530 ----------
>> src/mapi/glapi/glapi_mapi_tmp.**h |14139 ---------------
>> src/mapi/glapi/glapi_sparc.S | 1613 --
>> src/mapi/glapi/glapi_x86-64.S |37708 ------------------------------**
>> ---------
>> src/mapi/glapi/glapi_x86.S | 1528 --
>> src/mapi/glapi/glapitable.h | 1023 --
>> src/mapi/glapi/glapitemp.h | 9742 ----------
>> src/mapi/glapi/glprocs.h | 2853 ---
>> src/mesa/main/.gitignore | 3 +
>> src/mesa/main/dispatch.h |12958 --------------
>> src/mesa/main/enums.c | 6638 -------
>> src/mesa/main/remap_helper.h | 5818 ------
>> 21 files changed, 21 insertions(+), 116266 deletions(-)
>> delete mode 100644 src/glx/indirect.c
>> delete mode 100644 src/glx/indirect.h
>> delete mode 100644 src/glx/indirect_init.c
>> delete mode 100644 src/glx/indirect_size.c
>> delete mode 100644 src/glx/indirect_size.h
>> delete mode 100644 src/mapi/glapi/glapi_gentable.**c
>> delete mode 100644 src/mapi/glapi/glapi_mapi_tmp.**h
>> delete mode 100644 src/mapi/glapi/glapi_sparc.S
>> delete mode 100644 src/mapi/glapi/glapi_x86-64.S
>> delete mode 100644 src/mapi/glapi/glapi_x86.S
>> delete mode 100644 src/mapi/glapi/glapitable.h
>> delete mode 100644 src/mapi/glapi/glapitemp.h
>> delete mode 100644 src/mapi/glapi/glprocs.h
>> delete mode 100644 src/mesa/main/dispatch.h
>> delete mode 100644 src/mesa/main/enums.c
>> delete mode 100644 src/mesa/main/remap_helper.h
>>
> ______________________________**_________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/**mailman/listinfo/mesa-dev<http://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120523/05cbddec/attachment.htm>
More information about the mesa-dev
mailing list