On 05/23/2012 01:33 PM, Ian Romanick 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 disagree. I recently added a new extension (in a patch I haven't sent out yet) that added a few new API entrypoints and enums, and discovered that running the generator(*) caused: 11 files changed, 6691 insertions(+), 5692 deletions(-) I haven't looked at any of this generated code before, I don't know how it works, or have any sense of what should change. I just know that some absurd amount of data changed and there's no way in hell I'm reading it. I'm also not sure whether all of that was caused by my changes, or catching up after someone forgot to re-run it. (*) Technically, the build system was so broken that many files did *not* get automatically regenerated by 'make mesa' in glapi/gen. I had to manually use git log to find two or three previous "regenerate the world" commits, get a list of the changed files, and manually delete them prior to running the generator. And after that failed, I finally had to go find all files that said "DO NOT EDIT" and throw those out. Then un-throw out the generators themselves. If "make clean" or "git clean" threw out the generated files, I would've had a lot more confidence that everything was up to date. > 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." I believe -you- review the generated code because you understand the generators enough to know what a reasonable delta is. I imagine most people wouldn't realize that indirect_size.c shouldn't change. Obviously, if you're making changes to the generator themselves, you should manually diff the output to make sure it's okay. If you're adding new XML descriptions of an API, then I hope it isn't that catastrophically fragile. > 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? Regenerating SPARC dispatch incorrectly = fail. Not regenerating it at all also = fail. Untested platforms may break. That's bad, but suggests that we need more automated testing across platforms, not that we need to have a bunch of generated C files tracked in git. > 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. Agreed. > 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> So we should do automated regression testing on indirect rendering every week or so. This sounds like the perfect thing to throw on a lab, and actual testing is far more likely to actually find bugs than people manually diffing piles of unreadable C code. > 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 *100%* of the Khronos ES2 conformance suite fails *right now* and bisects to a commit that Eric made to add some new content to GL3.xml. I verified that commit several times in disbelief. The problem was that the build system didn't run the generators correctly and some rubbish got committed that routed glShaderSource to _mesa_LinkProgram. You're welcome to say "I told you so." I'm fine with that. There may be problems with not including the generated files, but there are serious problems today that this series claims to address. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev