Avoid IPC x ipc confusion on case insensitive filesystems

RESOLVED FIXED in mozilla12

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
Created attachment 579410 [details] [diff] [review]
Avoid IPC x ipc confusion on case insensitive filesystems
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #579410 - Flags: review?(ted.mielczarek)
https://tbpl.mozilla.org/?tree=Try&rev=81e95d69913a
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.
Created attachment 579418 [details] [diff] [review]
now without the changes to StartupTimeline.h and mozconfig.common
Attachment #579410 - Attachment is obsolete: true
Attachment #579410 - Flags: review?(ted.mielczarek)
Attachment #579418 - Flags: review?(ted.mielczarek)
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.
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
Attachment #579418 - Attachment is obsolete: true
Attachment #579418 - Flags: review?(ted.mielczarek)
Attachment #579580 - Flags: review?(jmuizelaar)
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
Attachment #579580 - Attachment is obsolete: true
Attachment #579580 - Flags: review?(jmuizelaar)
Attachment #579688 - Flags: review?(jmuizelaar)
One more try at fixing windows
https://tbpl.mozilla.org/?tree=Try&rev=67fc90ce4792
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 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.
Attachment #579688 - Flags: review?(jmuizelaar) → review+
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?
Created attachment 581060 [details] [diff] [review]
last version of the patch

Unfortunately I was not able to push this to try yet.
A new try push is at
https://tbpl.mozilla.org/?tree=Try&rev=0ad2a578f4c4
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.
Attachment #579688 - Flags: review?(ted.mielczarek)
Attachment #579688 - Flags: review+
Attachment #579688 - Flags: feedback?(joe)
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.
Attachment #579688 - Flags: feedback?(joe) → review-
> 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.
(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.
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
Attachment #579688 - Attachment is obsolete: true
Attachment #581060 - Attachment is obsolete: true
Attachment #579688 - Flags: review?(ted.mielczarek)
Attachment #582942 - Flags: review?(joe)
ping?
Attachment #582942 - Flags: review?(joe) → review+
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=35b3c5ac6e8f
https://hg.mozilla.org/mozilla-central/rev/35b3c5ac6e8f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.