On 23 May 2012 13:33, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 05/23/2012 12:45 PM, Eric Anholt wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> </div> <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. </blockquote><div> 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> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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<div class="HOEnZb"> <div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- 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! <a href="http://configure.ac" target="_blank">configure.ac</a> | 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 </blockquote></div></div><div class="HOEnZb"><div class="h5"> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div>