Closed Bug 708051 Opened 13 years ago Closed 12 years ago

Avoid IPC x ipc confusion on case insensitive filesystems

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla12

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file, 5 obsolete files)

      No description provided.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #579410 - Flags: review?(ted.mielczarek)
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.
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.
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)
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)
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?
Attached patch last version of the patch (obsolete) — Splinter Review
Unfortunately I was not able to push this to try yet.
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.
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)
Attachment #582942 - Flags: review?(joe) → review+
https://hg.mozilla.org/mozilla-central/rev/35b3c5ac6e8f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: