938489 - configure requires GL/glx.h even with --disable-webgl

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Christophe Jarry]

User Agent: Mozilla/5.0 (X11; U; Linux mips64; en-US; rv:1.9.1.16) Gecko/20121216 Iceweasel/3.5.16 (like Firefox/3.5.16) (like Firefox/3.5.16) (Nightly/Aurora)
Build ID: 20121216123242

Steps to reproduce:

tar xf firefox-25.0.source.tar.bz2
mkdir firefox-build
cd firefox-build
../mozilla-release/configure \
    --prefix=/usr \
    --enable-application=browser \
    --disable-alsa \
    --disable-gstreamer \
    --disable-webgl \
    --disable-tests \
    --disable-jsd \
    --disable-crashreporter


Actual results:

configure:28168:20: fatal error: GL/glx.h: No such file or directory


Expected results:

configure should complete successfully.

Comment 1

[Mike Hommey [:glandium] (high latency)]

(In reply to Christophe Jarry from comment #0)
> configure:28168:20: fatal error: GL/glx.h: No such file or directory

I guess the actual error is:
configure: error: Can't find header GL/glx.h for WebGL (install mesa-common-dev (Ubuntu), mesa-libGL-devel (Fedora), or Mesa-devel (openSUSE))

I think GL is required for more than webgl nowadays, even in 25. I guess one of the Benoits would know.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

OpenGL is needed for various things (like WebGL) but there should never be a hard dependency on any OpenGL-related package being installed. We should never use system-wide OpenGL headers provided by distro packages. Instead, we carry our own GL headers; and we never link to GL libraries, instead we dlopen them.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

So this looks like a bug in the configure script; maybe you could attach your configure script to this bug.

Comment 4

[Benoit Girard (:BenWa)]

Agreed with bjacob. -needinfo.

Comment 5

[Mike Hommey [:glandium] (high latency)]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> OpenGL is needed for various things (like WebGL) but there should never be a
> hard dependency on any OpenGL-related package being installed. We should
> never use system-wide OpenGL headers provided by distro packages. Instead,
> we carry our own GL headers; and we never link to GL libraries, instead we
> dlopen them.

we don't have a glx.h header in our tree. And it's used from several places in the tree. How do you expect that builds?

Comment 6

[Mike Hommey [:glandium] (high latency)]

As for "his" configure script, it's the same on mozilla-central:
if test "$MOZ_GL_DEFAULT_PROVIDER" = "GLX"; then
    MOZ_CHECK_HEADER(GL/glx.h)
    if test "$ac_cv_header_GL_glx_h" != "yes"; then
        AC_MSG_ERROR([Can't find header GL/glx.h for WebGL (install mesa-common-dev (Ubuntu), mesa-libGL-devel (Fedora), or Mesa-devel (openSUSE))])
    fi
fi # MOZ_GL_DEFAULT_PROVIDER=GLX
fi # COMPILE_ENVIRONMENT

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Benoit Jacob [:bjacob] from comment #2)
> > OpenGL is needed for various things (like WebGL) but there should never be a
> > hard dependency on any OpenGL-related package being installed. We should
> > never use system-wide OpenGL headers provided by distro packages. Instead,
> > we carry our own GL headers; and we never link to GL libraries, instead we
> > dlopen them.
> 
> we don't have a glx.h header in our tree. And it's used from several places
> in the tree. How do you expect that builds?

The stuff that we would need from glx.h is instead provided by other headers that we have:

GLX constants are provided by:
http://hg.mozilla.org/mozilla-central/file/7b014f0f3b03/gfx/gl/GLConsts.h

GLX API entry points are provided by:
http://hg.mozilla.org/mozilla-central/file/7b014f0f3b03/gfx/gl/GLXLibrary.h

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Mike Hommey [:glandium] from comment #6)
> As for "his" configure script, it's the same on mozilla-central:
> if test "$MOZ_GL_DEFAULT_PROVIDER" = "GLX"; then
>     MOZ_CHECK_HEADER(GL/glx.h)
>     if test "$ac_cv_header_GL_glx_h" != "yes"; then
>         AC_MSG_ERROR([Can't find header GL/glx.h for WebGL (install
> mesa-common-dev (Ubuntu), mesa-libGL-devel (Fedora), or Mesa-devel
> (openSUSE))])
>     fi
> fi # MOZ_GL_DEFAULT_PROVIDER=GLX
> fi # COMPILE_ENVIRONMENT

This configure check is wrong, it should be removed, the presence of a glx.h anywhere is irrelevant to us.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Wait, I didn't realize that GLXLibrary.h and glxtest.cpp are currently including glx.h.

That's what we should fix.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Note that both GLXLibrary.h and glxtest.h are compiled regardless of --disable-webgl (and used outside of WebGL), so at least the error message in that configure check is wrong.

Comment 11

[Mike Hommey [:glandium] (high latency)]

(In reply to Benoit Jacob [:bjacob] from comment #9)
> Wait, I didn't realize that GLXLibrary.h and glxtest.cpp are currently
> including glx.h.
> 
> That's what we should fix.

So does cairo and skia (and a webrtc test that we probably don't build).
https://mxr.mozilla.org/mozilla-central/search?string=GL/glx.h

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Indeed, but I am not sure that we use these parts of these libraries (haven't checked).

Maybe we'll still have a configure check for glx, but it should not claim that it has anything to do with WebGL.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

I have put a #error in my system GL/glx.h header, and rebuilt gfx/skia, gfx/cairo and media/webrtc, with debug and tests enabled, on Linux x86-64. None of them were actually including glx.h.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't include glx.h in GLXLibrary.h

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't include GL headers in glxtest

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Remove configure check for glx.h

With the two patches above, we actually aren't including glx.h at the moment, as verified above.

If people want to start relying on the parts of cairo/skia/webrtc that include glx.h in the future, they'll have to re-add a configure check (or more likely, patch these libs to not include glx.h) but for now let's remove this configure check whose error message was misleadingly pointing to WebGL.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 832587 [details] [diff] [review]
Don't include glx.h in GLXLibrary.h

Oops, I remember now, Karl is the one whom I used to get my glxtest reviews from, so let's have you do the two reviews here.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't include glx.h in GLXLibrary.h

Fixed some search&replace problems. Also, on second thought, Matt _is_ the right reviewer for this one :-)

Comment 19

[Mike Hommey [:glandium] (high latency)]

Comment on attachment 832592 [details] [diff] [review]
Remove configure check for glx.h

Review of attachment 832592 [details] [diff] [review]:
-----------------------------------------------------------------

Please update mach bootstrap to not require mesa devel packages, too. I'm also tempted to request that we add something to make including GL/glx.h fail if we ever happen to build code including it, so that the dependency doesn't go unnoticed then, but if enough people build without the mesa devel packages from their machine, that should be caught quickly enough.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 832592 [details] [diff] [review]
> Remove configure check for glx.h
> 
> Review of attachment 832592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update mach bootstrap to not require mesa devel packages, too.

Will look into that (have no experience hacking mach).

> I'm
> also tempted to request that we add something to make including GL/glx.h
> fail if we ever happen to build code including it, so that the dependency
> doesn't go unnoticed then, but if enough people build without the mesa devel
> packages from their machine, that should be caught quickly enough.

That sounds like a very good idea.

Comment 21

[Christophe Jarry]

I have applied the 3 patches to firefox 25.0 sources. I am trying to regenerate configure from the patched configure.in with:

    autoconf configure.in > configure

It errors with:

    build/autoconf/acwinpaths.m4:10: error: dfn: undefined macro: AC_OUTPUT_FILES
    build/autoconf/acwinpaths.m4:10: the top level
    autom4te: /usr/bin/m4 failed with exit status 1.

My autoconf is 2.69, M4 is 1.4.16.

How may I fix this issue?

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Have you tried ./mach build (or build-backend) instead of invoking autoconf manually?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Firefox requires autoconf 2.13.

Comment 24

[Christophe Jarry]

./mach build requires autoconf 2.13. So installed autoconf 2.13 and did autoconf configure.in > configure.

When I run:

../mozilla-release/configure \
    --prefix=/usr \
    --enable-application=browser \
    --disable-alsa \
    --disable-gstreamer \
    --disable-webgl \
    --disable-tests \
    --disable-jsd \
    --disable-crashreporter

I get the following message:

checking for sys/int_types.h... no
checking for posix_fallocate... yes
../mozilla-release/configure: line 30031: syntax error: unexpected end of file
------ config.log ------
...
...
...
configure:26644:27: fatal error: sys/int_types.h: No such file or directory
compilation terminated.
configure: failed program was:
#line 26642 "configure"
#include "confdefs.h"

I will attach my initial (unpatched) configure named configure.orig, and the one generated by autoconf after having applied the 3 patches you proposed.
#include <sys/int_types.h>
int main() {

; return 0; }

Comment 25

[Christophe Jarry]

Attachment: configure.orig (initial configure script from Firefox 25.0 sources)

Comment 26

[Christophe Jarry]

Attachment: configure (generated with autoconf 2.13 from patched configure.in)

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=27f1efd13c18

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8bbb6bcfcd50
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6aec2ae23025
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5641d2ab4d52

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8bbb6bcfcd50
https://hg.mozilla.org/mozilla-central/rev/6aec2ae23025
https://hg.mozilla.org/mozilla-central/rev/5641d2ab4d52

Comment 30

[Christophe Jarry]

The following line in the "Remove configure check for glx.h" patch should not be deleted:

    fi # COMPILE_ENVIRONMENT

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Yes, the version that landed doesn't have that bug.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Sorry that I didn't see your comments earlier. mozilla-central should work fine.

Comment 33

[Christophe Jarry]

Now I have several errors like the one below at make:

    gfx/gl/GLContextProviderGLX.cpp:xxx:xx: error: 'LOCAL_GLX_XXX' was not declared in this scope

find mozilla-release -name '*.h' | xargs grep 'LOCAL_GLX' -l doesn't print anything.

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

Don't you have them defined in GLConsts.h like this:
http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLConsts.h#l5325

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 832592 [details] [diff] [review]
> Remove configure check for glx.h
> 
> Review of attachment 832592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update mach bootstrap to not require mesa devel packages, too.

I still don't know how to do that. Can you point me to the place where this is currently done?

Comment 36

[Nathan Froyd [:froydnj]]

(In reply to Benoit Jacob [:bjacob] from comment #35)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > Please update mach bootstrap to not require mesa devel packages, too.
> 
> I still don't know how to do that. Can you point me to the place where this
> is currently done?

http://mxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/debian.py#27

Search other .py files in that directory for further examples.

Comment 37

[Mike Hommey [:glandium] (high latency)]

$ grep -ri mesa python/mozboot/mozboot/
python/mozboot/mozboot/debian.py:        'mesa-common-dev',
python/mozboot/mozboot/fedora.py:            'mesa-libGL-devel',
python/mozboot/mozboot/centos.py:            'mesa-libGL-devel',

Comment 38

[(not currently active) Ted Mielczarek]

Don't we still require those devel packages in the default configuration?

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

Why would they be required? As far as I know, we (intentionally) don't include any header they provide or link to any library they provide. We use our own GL headers and dlopen GL libraries.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't suggest installing mesa packages

Let me verify locally that everything builds fine without such packages.

Comment 41

[Christophe Jarry]

(In reply to Benoit Jacob [:bjacob] from comment #34)
> Don't you have them defined in GLConsts.h like this:
> http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLConsts.h#l5325

mozilla-release/gfx/gl/GLConsts.h is not present in firefox 25.0.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

I was talking about mozilla-central! The patches here were not designed to apply against 25.0.

I realize - now - that you had file this against 25 branch, so if you want to backport these patches to 25, that should be easy enough, you only need to know that in that branch, the GL constants that now are in GLConsts.h were in GLDefs.h.

http://hg.mozilla.org/releases/mozilla-release/file/66755f89d981/gfx/gl/GLDefs.h

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

(But to be clear, such patches likely won't be backported to any channel, especially not the release channel).

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #40)
> Created attachment 8335243 [details] [diff] [review]
> Don't suggest installing mesa packages
> 
> Let me verify locally that everything builds fine without such packages.

So, just uninstalling packages somehow left GL headers in my /usr/include, so what I did was I added #error 's at the top of GL/gl.h, GL/glx.h and GL/glext.h, and verified that there was no other like-named headers anywhere under /usr/include.

Clobbered, built, all worked fine!

Comment 45

[Christophe Jarry]

(In reply to Benoit Jacob [:bjacob] from comment #42)
> the GL constants that now are in GLConsts.h were in GLDefs.h.

As I wrote, find mozilla-release -name '*.h' | xargs grep 'LOCAL_GLX' -l doesn't print anything. So LOCAL_GLX* constants have been defined after Firefox 25.0.

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

Ah, indeed, they were added by
http://hg.mozilla.org/mozilla-central/rev/a4d217987888
so if you want to locally backport that patch, you'll have to manually add these defines to GLDefs.h

Comment 47

[Christophe Jarry]

Thanks, running make inside my firefox-25.0/gfx/gl now succeeds.

Comment 48

[Mike Hommey [:glandium] (high latency)]

Comment on attachment 8335243 [details] [diff] [review]
Don't suggest installing mesa packages

Review of attachment 8335243 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't come to this soon enough. That's an unfortunate consequence of me using http://harthur.github.io/bugzilla-todos/ for so long, and it not showing review requests on closed bugs.

This probably needs an update in some more files nowadays. At this point, you probably want to file a separate bug for this.