Last Comment Bug 708051 - Avoid IPC x ipc confusion on case insensitive filesystems
: Avoid IPC x ipc confusion on case insensitive filesystems
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 12:26 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-01-04 04:46 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Avoid IPC x ipc confusion on case insensitive filesystems (20.75 KB, patch)
2011-12-06 12:30 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
now without the changes to StartupTimeline.h and mozconfig.common (18.86 KB, patch)
2011-12-06 12:43 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Remove one leve of recursion on gfx (14.12 KB, patch)
2011-12-06 20:34 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Remove one level of recursion on gfx (14.12 KB, patch)
2011-12-07 07:59 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
joe: review-
Details | Diff | Review
last version of the patch (15.66 KB, patch)
2011-12-12 14:42 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Avoid IPC x ipc confusion on case insensitive filesystems (2.17 KB, patch)
2011-12-19 13:44 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
joe: review+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 12:26:11 PST

    
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 12:30:36 PST
Created attachment 579410 [details] [diff] [review]
Avoid IPC x ipc confusion on case insensitive filesystems
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 12:30:58 PST
https://tbpl.mozilla.org/?tree=Try&rev=81e95d69913a
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 12:38:30 PST
I found this issue while trying to build firefox with distcc-pump on OS X. I was using a client with a case insensitive file system and and a server with a case sensitive one.

The problem is that when a header is included via "foo.h", distcc will find and send to the server the path .../ipc/foo.h. If the same compilation unit also includes "IPC/foo.h", it will conclude that the file was already sent, but the server, being case sensitive disagrees.

This patch just replaces IPC with ipc2. I am testing a more lightweight approach too and will update the bug if it works.
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 12:43:10 PST
Created attachment 579418 [details] [diff] [review]
now without the changes to StartupTimeline.h and mozconfig.common
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 15:50:00 PST
The problem is simpler (and scarier) than I first thought.  Something as simple as 

clang -arch x86_64 -o CanvasLayerOGL.o -c -I/Users/espindola/mozilla-central/gfx/layers  -I../../dist/include    /Users/espindola/mozilla-central/gfx/layers/opengl/CanvasLayerOGL.cpp -w

will use the headers from /Users/espindola/mozilla-central/gfx/layers/ipc on a case insensitive filesystem and the ones from
../../dist/include/IPC on a case sensitive one.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-06 20:34:49 PST
Created attachment 579580 [details] [diff] [review]
Remove one leve of recursion on gfx

This is probably a better way to solve the problem. By removing one level of recursion, the -I flag now points one level up and the includes are not dependent on the filesystem being case sensitive or not.

Removing recursion is also a good thing for build times. I have also removed the extra vpaths so that make doesn't have to search as much.

The patch can be improved a bit, by adding one moz.mk file to each directory and trying some Makefile magic to reduce the repetitiveness, but I am interested in any feedback you might have already. Is this patch in the correct path?

https://tbpl.mozilla.org/?tree=Try&rev=62def5bb153e
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-07 07:59:59 PST
Created attachment 579688 [details] [diff] [review]
Remove one level of recursion on gfx

This one hopefully fixes the windows builds
https://tbpl.mozilla.org/?tree=Try&rev=006159c3be07
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-07 08:42:57 PST
One more try at fixing windows
https://tbpl.mozilla.org/?tree=Try&rev=67fc90ce4792
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-07 12:16:02 PST
My lest try is at

https://tbpl.mozilla.org/?tree=Try&rev=5b4718afe987

I will probably need to get a windows machine to figure out what is going on :-(
Comment 10 Jeff Muizelaar [:jrmuizel] 2011-12-12 11:58:08 PST
Comment on attachment 579688 [details] [diff] [review]
Remove one level of recursion on gfx

I'm not thrilled with this. Having to specify the full path to these files in the Makefile is a bit of a pain. At the same time I don't feel that strongly either. It would be interesting to hear what others think.
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-12 12:51:17 PST
I think I have a version that also works on windows in here:

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

From whom do you want to hear opinions on this patch?
Comment 12 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-12 14:42:14 PST
Created attachment 581060 [details] [diff] [review]
last version of the patch

Unfortunately I was not able to push this to try yet.
Comment 13 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-13 11:47:28 PST
A new try push is at
https://tbpl.mozilla.org/?tree=Try&rev=0ad2a578f4c4
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-16 05:37:51 PST
Comment on attachment 579688 [details] [diff] [review]
Remove one level of recursion on gfx

Jeff asked if you guys could review this too. Not sure if I can add multiple reviewers to a bug, so I added Joe in feedback.
Comment 15 Joe Drew (not getting mail) 2011-12-16 11:36:18 PST
Comment on attachment 579688 [details] [diff] [review]
Remove one level of recursion on gfx

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

You should definitely add a layers/Makefile.in that includes moz.mk so we can still build from within layers/.

I'm r- based on the fact that we've got a mish-mash of layers/ and non-layers/ includes here. I'd prefer to just include the file from the CWD without layers/, and include the bits in sub-directories, like ipc/, from layers/ipc/.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +47,1 @@
>  #include "ThebesLayerOGL.h"

It's very odd to me that some of these are prefixed with layers/ and some aren't.
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-16 12:31:35 PST
> You should definitely add a layers/Makefile.in that includes moz.mk so we
> can still build from within layers/.

you are still able to say

make layers/foo.o

Making a moz.mk that can be included from two directories would require quiet a bit of makefile magic to add or remove prefixes depending on who is doing the include.

This would also be problematic since our build system changes the command line flags depending on what makefile is being used:

INCLUDES = \
  $(LOCAL_INCLUDES) \
  -I$(srcdir) \
  -I.

so you would get different commands depending on where you executed make from.

Is this a sine qua non? If so, would you be ok with the old patch I proposed for avoiding the IPC x ipc confusion?

https://bug708051.bugzilla.mozilla.org/attachment.cgi?id=579418

> I'm r- based on the fact that we've got a mish-mash of layers/ and
> non-layers/ includes here. I'd prefer to just include the file from the CWD
> without layers/, and include the bits in sub-directories, like ipc/, from
> layers/ipc/.
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +47,1 @@
> >  #include "ThebesLayerOGL.h"
> 
> It's very odd to me that some of these are prefixed with layers/ and some
> aren't.

This is because of the above Makefile snippet. This particular case is found by the "-I.". If you are OK with above restrictions I will update this patch to use layers/ prefix everywhere.
Comment 17 Joe Drew (not getting mail) 2011-12-16 15:16:27 PST
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #16)
> Is this a sine qua non? If so, would you be ok with the old patch I proposed
> for avoiding the IPC x ipc confusion?
> 
> https://bug708051.bugzilla.mozilla.org/attachment.cgi?id=579418

"gfxipc" would be better. Or "layersipc".

> This is because of the above Makefile snippet. This particular case is found
> by the "-I.". If you are OK with above restrictions I will update this patch
> to use layers/ prefix everywhere.

I'm totally fine with that.
Comment 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-19 13:44:35 PST
Created attachment 582942 [details] [diff] [review]
Avoid IPC x ipc confusion on case insensitive filesystems

This version renames just the directory where the gfx ipc files are installed.

https://tbpl.mozilla.org/?tree=Try&rev=bbd9b0955adb
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-01-03 10:17:16 PST
ping?
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-01-03 11:38:29 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=35b3c5ac6e8f
Comment 21 Marco Bonardo [::mak] 2012-01-04 04:46:48 PST
https://hg.mozilla.org/mozilla-central/rev/35b3c5ac6e8f

Note You need to log in before you can comment on or make changes to this bug.