Closed Bug 938489 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

25 Branch
SGI
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: christophe.jarry, Assigned: bjacob)

Details

Attachments

(6 files, 1 obsolete file)

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.
Component: Untriaged → Build Config
(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.
Flags: needinfo?(bjacob)
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bjacob)
So this looks like a bug in the configure script; maybe you could attach your configure script to this bug.
Agreed with bjacob. -needinfo.
Flags: needinfo?(bgirard)
(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?
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
(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
(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.
Wait, I didn't realize that GLXLibrary.h and glxtest.cpp are currently including glx.h.

That's what we should fix.
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.
(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
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.
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.
Attachment #832587 - Flags: review?(matt.woodrow)
Attachment #832590 - Flags: review?(matt.woodrow)
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.
Attachment #832592 - Flags: review?(mh+mozilla)
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.
Attachment #832587 - Flags: review?(matt.woodrow) → review?(karlt)
Attachment #832590 - Flags: review?(matt.woodrow) → review?(karlt)
Fixed some search&replace problems. Also, on second thought, Matt _is_ the right reviewer for this one :-)
Attachment #832597 - Flags: review?(matt.woodrow)
Attachment #832587 - Attachment is obsolete: true
Attachment #832587 - Flags: review?(karlt)
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.
Attachment #832592 - Flags: review?(mh+mozilla) → review+
(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.
Attachment #832597 - Flags: review?(matt.woodrow) → review+
Attachment #832590 - Flags: review?(karlt) → review+
Product: Firefox → Core
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?
Have you tried ./mach build (or build-backend) instead of invoking autoconf manually?
./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; }
The following line in the "Remove configure check for glx.h" patch should not be deleted:

    fi # COMPILE_ENVIRONMENT
Yes, the version that landed doesn't have that bug.
Sorry that I didn't see your comments earlier. mozilla-central should work fine.
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.
Don't you have them defined in GLConsts.h like this:
http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLConsts.h#l5325
(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?
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
$ 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',
Don't we still require those devel packages in the default configuration?
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.
Let me verify locally that everything builds fine without such packages.
Attachment #8335243 - Flags: review?(mh+mozilla)
(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.
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
(But to be clear, such patches likely won't be backported to any channel, especially not the release channel).
(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!
(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.
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
Thanks, running make inside my firefox-25.0/gfx/gl now succeeds.
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.
Attachment #8335243 - Flags: review?(mh+mozilla) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.