[Mesa-dev] [PATCH] glapi: Fix incorrect enum value.

[Ian Romanick]

On 02/22/2012 04:14 PM, Paul Berry wrote:
> On 22 February 2012 16:06, Chad Versace <chad.versace at linux.intel.com
> <mailto:chad.versace at linux.intel.com>> wrote:
>
>     On 02/22/2012 02:22 PM, Ian Romanick wrote:
>      > On 02/22/2012 02:17 PM, Paul Berry wrote:
>      >> On 22 February 2012 13:52, Ian Romanick <idr at freedesktop.org
>     <mailto:idr at freedesktop.org>
>      >> <mailto:idr at freedesktop.org <mailto:idr at freedesktop.org>>> wrote:
>      >>
>      >>     On 02/22/2012 01:41 PM, Paul Berry wrote:
>      >>
>      >>           From
>      >> http://www.opengl.org/__registry/specs/ARB/seamless___cube_map.txt
>      >> <http://www.opengl.org/registry/specs/ARB/seamless_cube_map.txt>:
>      >>
>      >>              Accepted by the<cap>  parameter of Enable, Disable and
>      >>         IsEnabled,
>      >>              and by the<pname>  parameter of GetBooleanv,
>     GetIntegerv,
>      >>         GetFloatv
>      >>              and GetDoublev:
>      >>
>      >>              TEXTURE_CUBE_MAP_SEAMLESS                   0x884F
>      >>
>      >>
>      >>     Oops.  That was my typo.  You'll also have to regenerate the
>     various
>      >>     files that depend on the XML definitions.  I think this change
>      >>     should only cause changes in src/mesa/main/enums.c.
>      >>
>      >>     Reviewed-by: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>
>      >> <mailto:ian.d.romanick at intel.com <mailto:ian.d.romanick at intel.com>>>
>      >>
>      >>
>      >> Oops.  I didn't realize that those files weren't built
>     automatically.
>      >> I'll send an updated patch.
>      >
>      > Dear god, no!  The patch will be huge.
>
>      >> Is there any good reason why we don't automatically generate
>     files like
>      >> enum.c as part of the mesa build process?  The comment at the top of
>      >> src/mapi/glapi/gen/Makefile says "This file isn't used during a
>     normal
>      >> compilation since we don't want to require Python in order to
>     compile
>      >> Mesa."  But I don't think that makes sense anymore, because
>     Python is
>      >> required to build files like src/mapi/es2api/glapi_mapi_tmp.h,
>     as well
>      >> as some files in src/glsl.
>      >>
>      >> In point of fact, it seems really strange that a file like
>      >> src/mapi/es2api/glapi_mapi_tmp.h is autogenerated during the build
>      >> process, but src/mesa/main/enums.c isn't, since both files are built
>      >> from the same set of xml sources.
>      >
>      > A couple reasons:
>
>     I really want to see all *build* artifacts removed from Mesa's
>     *source* control. I recall the
>     pain of having the bison and flex output managed by git, and I don't
>     like continuing that fallacy.
>
>      > 1. The generated files really, really, really should be in git so
>     that accidental changes will be noticed.  This has bitten us a
>     couple times.
>
>     Wouldn't `git log *.xml *.py` also alert you that the generated
>     headers have changed?
>      >
>      > 2. Changes to the XML files frequently precipitate changes to
>     files in the xserver.  There's no way to automatically handle that.
>
>     I don't understand how 2 affects this discussion. Is it that the
>     xserver build relies on these generated headers? If so, then
>     we can move the generated headers to the xserver repo and merrily
>     remove these build artifacts from Mesa's source control.
>     Afterwards, it would be the xserver's devs responsibility to
>     manually update their headers, or fix their makefiles to gen them
>     at build time.
>
>      > 3. Several of the scripts take a really, really long time to run.
>       I'm not eager to add a few minutes to my clean-build times.
>
>     $ cd src/mapi/glapi/gen
>     $ make mesa
>     real    0m5.634s
>
>     I don't think an adding 5 seconds to a clean build is cause for
>     concern if the addition allows us
>     to remove build artifacts from source control.
>
>
> I agree with this.  In my experience, including build outputs in source
> control is nearly always a mistake; as a rule, I'd rather exclude build
> artifacts from source control except in situations where there is no
> feasible alternative.  IMHO this doesn't seem like one of those situations.

I am strongly opposed to not including generated code in source control. 
  A lot of the generated code doesn't get a lot of testing (e.g., GLX 
protocol code), so subtle breaks in generated code may not be noticed 
for too long.  We've had several cases over the years where seemingly 
innocuous changes to generator scripts or XML cause, literally, dozens 
of functions to be generated completely broken.  Some cases have 
included calculated packet sizes being wrong in all functions of a 
certain category, all functions of a certain category having empty 
bodies, and dispatch code for other CPUs being broken or empty.

Having the generated files tracked by GIT at least makes it a lot easier 
to find the break.  When someone notices that src/glx/indirect.c has 
errors, they can git-log that file instead of having to either bisect or 
view the git-log of all the files that contribute to that file being 
built (I'd estimate that there are about a dozen).

Having the person making the commits see the changes also helps prevent 
bad changes from ever landing.  If you haven't added any new functions, 
it should set of alarm bells when you're committing changes to 
src/mapi/glapi/glapi_sparc.S.  That extra safeguard alone has saved my 
ass several times.

I'm not willing to give up that safety net without something to replace it.