Bug 35441

Summary: [PATCH] Mesa does not find nouveau include files with --enable-shared-dricore
Product: Mesa Reporter: Robert White <rwhite>
Component: Mesa coreAssignee: mesa-dev
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: highest CC: alexxy, chalserogers, fabio.ped, johannesobermayr, jordan.yelloz+fdo
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
Attachments: a suitable patch
just nouveau *.pc
use pkgconfig in configure.ac and INCLUDES in Makefiles
uses pkgconfig in configure.ac and configure and INCLUDES in Makefiles
use pkgconfig in configure.ac and INCLUDES in Makefiles
uses pkgconfig in configure.ac and configure and INCLUDES in Makefiles
split Makefile.template and use pkgconfig in configure.ac and INCLUDES in Makefiles
split Makefile.template and use pkgconfig in configure.ac and configure and INCLUDES in Makefiles
Final fix!
Final fix #2

Description Robert White 2011-03-18 20:43:25 UTC
The current git repository as of this date is missing at least two header files referenced by src/mesa/drivers/dri/nouveau/nouveau_driver.h

The file nouveau_device.h is missing and when I just experimentally commented that out then nouveau_gobj.h cropped up missing so I stopped.

All the other header files in the same block of includes that I could locate are all apparently local to this directory. They are also all local includes (e.g. in "" not <> quotes) so I figure they are all from here.

spot checking I think

nouveau_device.h, nouveau_grobj.h, nouveau_channel.h, and nouveau_notifier.h and any .c files they front for are suspiciously missing or live somewhere inobvious to me.

This was first observed about two weeks ago but I didn't realize this was the right place to report it. /doh.
Comment 1 Себастьян Gliţa Κατινα 2011-03-20 00:15:41 UTC
Created attachment 44629 [details] [review]
a suitable patch

In file `src/mesa/drivers/dri/nouveau/Makefile.am' use `LIBDRM_CFLAGS' instead of just `CFLAGS': the pkg-config flags from `libdrm_nouveau.pc' (installed by `libdrm') must yield its `-I/usr/include/nouveau' where you find all these "missing" header files. (`INCLUDES' variable instead does not seem to work).

c.
Comment 2 Себастьян Gliţa Κατινα 2011-03-24 02:49:54 UTC
Created attachment 44779 [details] [review]
just nouveau *.pc

no need to for libdrm, see configs/linux-dri
(it was `../Makefile' not `../Makefile.am')
Comment 3 Себастьян Gliţa Κατινα 2011-03-24 02:56:53 UTC
*** Bug 35609 has been marked as a duplicate of this bug. ***
Comment 4 Francisco Jerez 2011-03-24 11:00:51 UTC
Apparently the problem is just that CFLAGS is not taken into account during the DRI driver compilation when using "--enable-shared-dricore".

Christopher, was that the intended behavior of that option?
Comment 5 Себастьян Gliţa Κατινα 2011-03-25 01:36:40 UTC
Created attachment 44812 [details] [review]
use pkgconfig in configure.ac and INCLUDES in Makefiles


- moved `+=' stuff after "include ../Makefile.template"

- placing stuff in configure.ac

shortcoming:
should consider for nouveau also nv50 etc. like for radeon r300 r600 ...

proposal:
split `Makefile.template' into `Makefile.common' and `Makefile.targets'
Comment 6 Себастьян Gliţa Κατινα 2011-03-25 01:38:48 UTC
Created attachment 44813 [details] [review]
uses pkgconfig in configure.ac and configure and INCLUDES in Makefiles

same patch but added stuff also for `configure' script
Comment 7 Peter Hjalmarsson 2011-03-27 04:51:33 UTC
Comment on attachment 44813 [details] [review]
uses pkgconfig in configure.ac and configure and INCLUDES in Makefiles

Emm, why rename the radeon variable?


-RADEON_LIBS = `pkg-config --libs libdrm_radeon`
+NOUVEAU_LIBS = `shell pkg-config libdrm_nouveau --libs`
+NOUVEAU_CFLAGS = `shell pkg-config libdrm_nouveau --cflags`
+
+RADEON_LDFLAGS = `pkg-config --libs libdrm_radeon`
Comment 8 Себастьян Gliţa Κατινα 2011-03-27 12:44:29 UTC
(In reply to comment #7)
> (From update of attachment 44813 [details] [review])
> Emm, why rename the radeon variable?
> 
> 
> -RADEON_LIBS = `pkg-config --libs libdrm_radeon`
> +NOUVEAU_LIBS = `shell pkg-config libdrm_nouveau --libs`
> +NOUVEAU_CFLAGS = `shell pkg-config libdrm_nouveau --cflags`
> +
> +RADEON_LDFLAGS = `pkg-config --libs libdrm_radeon`

It seemed as a bug w.r.t. uninformed use in all Makefiles.

RADEON_LIBS looks like gone (not LIBDRM_RADEON_LIBS though with one occurence in `configure.ac':1053); it's not coming from normal PKG_CHECK_MODULES m4 macro: LIBDRM_RADEON_* do in lines 870-873, and then are read in lines 1052-1053.

Is this better?

-RADEON_LIBS = `pkg-config --libs libdrm_radeon`
-RADEON_CFLAGS = `pkg-config --cflags libdrm_radeon`
+LIBDRM_RADEON_LIBS = `pkg-config --libs libdrm_radeon`
+LIBDRM_RADEON_CFLAGS = `pkg-config --cflags libdrm_radeon`
+RADEON_CFLAGS = "-DHAVE_LIBDRM_RADEON=1 $LIBDRM_RADEON_CFLAGS"
+RADEON_LDFLAGS = "$LIBDRM_RADEON_LIBS"
Comment 9 Себастьян Gliţa Κατινα 2011-03-29 09:30:05 UTC
Created attachment 45008 [details] [review]
use pkgconfig in configure.ac and INCLUDES in Makefiles

configs/linux-dri is a makefile
Comment 10 Себастьян Gliţa Κατινα 2011-03-29 09:31:01 UTC
Created attachment 45009 [details] [review]
uses pkgconfig in configure.ac and configure and INCLUDES in Makefiles

configs/linux-dri is a makefile
Comment 11 Себастьян Gliţa Κατινα 2011-03-30 03:57:03 UTC
Created attachment 45037 [details] [review]
split Makefile.template and use pkgconfig in configure.ac and INCLUDES in Makefiles

split `Makefile.template' into `Makefile.defines' and `Makefile.targets'
Comment 12 Себастьян Gliţa Κατινα 2011-03-30 03:58:57 UTC
Created attachment 45038 [details] [review]
split Makefile.template and use pkgconfig in configure.ac and configure and INCLUDES in Makefiles

split `Makefile.template' into `Makefile.defines' and `Makefile.targets'
Comment 13 Fabio Pedretti 2011-05-13 00:00:18 UTC
*** Bug 37147 has been marked as a duplicate of this bug. ***
Comment 14 Johannes Obermayr 2011-05-14 10:58:48 UTC
Created attachment 46722 [details] [review]
Final fix!

This patches fixes whole compiling.

Tested on openSUSE (with and without --enable-shared-dricore).

https://build.opensuse.org/package/show?package=Mesa&project=home%3Ajobermayr
(with --enable-shared-dricore)
Comment 15 Johannes Obermayr 2011-05-14 11:01:46 UTC
Set 'Component: Mesa core' since the patch touches core parts.
Comment 16 Johannes Obermayr 2011-05-21 14:02:26 UTC
Created attachment 46984 [details] [review]
Final fix #2

Thanks to Chris Bandy for the hint that line 48 missed a trailing double quote.

But I am still wondering that it needs ~ 1 week and a further hint by me on IRC until somebody had a (first?) look on it ...

Because some gave me hints on IRC that my (hard) wording sometimes is not reasonable I am referring to:
http://tsdgeos.blogspot.com/2010/10/qt-gitorious-merge-requests-and-open.html
(My worry is that this fix also will pushed someday after a couple of releases or even forgotten ...)

Btw. somebody told me that quality is: Away from accident.
Another one told me that source code should always be in a compileable state (indifferent whether you use [supported] configure switches) ...
Some other projects (e.g. Gnash or Lightspark or Gluon) are happy if they get hints for compile errors or compiler warnings on specific distributions/configure switches (and there the main/core developers are also reachable on their IRC channels ...)
Comment 17 Johannes Obermayr 2011-06-02 12:39:35 UTC
Set RESOLVED -> WONTFIX since nobody is man enough to push or comment on the patch which solves the issue within more than two weeks ...
Comment 18 Carl Worth 2011-06-02 15:53:02 UTC
(In reply to comment #17)
> Set RESOLVED -> WONTFIX since nobody is man enough to push or comment on the
> patch which solves the issue within more than two weeks ...

Hi Johannes,

It seems clear that you're frustrated that you're not getting a faster response to your contribution. However, WONTFIX is not the correct resolution for a bug that simply hasn't been examined closely enough yet.

(I'm not personally in a position to evalutate a change to Mesa's build system, but I would prefer to see a bug like this left open so that someone who is in such a position could find it.)

-Carl
Comment 19 Brian Paul 2011-06-02 16:21:01 UTC
I don't think anyone's actively working on the old nouveau driver any more (changes to such drivers tend to get ignored).
I applied the patch here and it seems OK (though I can't test nouveau).
I'll commit it in a minute.  It can always be reverted if anyone has problems.
Comment 20 Johannes Obermayr 2011-06-02 20:02:21 UTC
Just to clarify when and why I am getting angry:

1. Who is the person I must call? Usually I do not know it exactly: https://bugs.freedesktop.org/show_bug.cgi?id=37471 ; http://people.freedesktop.org/~cbrill/dri-log/index.php?date=2011-05-18&channel=dri-devel&highlight_names=&update=Update&date=2011-05-19 (at 10:50, 11:15 and )

2. Here I am lucky to know the person who is responsible for nouveau_vieux.so but assigned it to Mesa core (as a matter of prudence ...)

3. Write to the maintainer on IRC (at 17:03): http://people.freedesktop.org/~cbrill/dri-log/index.php?date=2011-05-14&channel=nouveau&highlight_names=&update=Update&date=2011-05-14

4. Waiting a week to get answer from Mesa core developers -> no response ...

5. Reminder (at 09:07): http://people.freedesktop.org/~cbrill/dri-log/index.php?date=2011-05-21&channel=dri-devel&highlight_names=&update=Update&date=2011-05-21

6. Packed developers on their honor (comment #17) after ~ 3 weeks

7. The patch got applied ...

8. I know it is not the correct way but it was not the first time developers/maintainers make a decision only after I packed them on their honor (also on other projects) ...

So my suggestion:
Overthink the development process and provide information about maintainers and people with permission to commit (I have not known who is part of Mesa core team ...).