476201 - devenv builds shunt library in the source tree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

binary files should be built in the object directory

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: remove visual studio project and solution

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 363702 [details] [diff] [review]
remove visual studio project and solution

+NATIVE_CFLAGS = $(CFLAGS) \

I don't understand what this is all doing. Do you mean to use HOST_* here?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

normally I'd want to use TARGET_*, but TARGET_CC is arm-wince-gcc, and I want to use x86_arm/cl (the arm cross compiling cl).

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 363702 [details] [diff] [review]
remove visual studio project and solution

I'm not really excited about the hardcoding of compiler paths etc in a Makefile. Brad says this is because the arm-wince-gcc tools force inclusion of the shunt library, which is what this Makefile is trying to build. I think we could work around this by having an environment variable or commandline switch to the tools to skip linking in the shunt. I think that would result in a cleaner patch.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated based on ted's comments

for extra bonus points, this patch fixes the tools not to put their .obj, .ilk, and .pdb files in the source tree as well as to prevent them from rebuilding every time

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

please ignore the two printfs in arm-wince-link, I'll remove them before checking in.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

The last getenv("NO_SHUNT") NULL check should be !getent("NO_SHUNT") for consistency with other two?

Comment 8

[Benjamin Smedberg]

Comment on attachment 365548 [details] [diff] [review]
updated based on ted's comments

I can't review this; I have no clue what it does. Back to Ted.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 365548 [details] [diff] [review]
updated based on ted's comments

+# if we're building the tools in configure, we want them to go directly to the sdk 
+# so they get rebuilt once we have a full environment
+ifdef VPATH
+MOZCE_TOOLS_BIN_DIR=$(OBJDIR)/build/wince/tools
+else
 MOZCE_TOOLS_BIN_DIR=$(OBJDIR)/dist/sdk/bin
+endif

Could you move this comment down into the else block, which is what it's describing?

Also, I think you should add a comment at the top of this Makefile (below the license block) that mentions that it's used both directly and included from another makefile.

Also, instead of relying on VPATH, could you just set some explicit variable in the other makefile and use that? It's not exactly clear when you're using VPATH here.

+EXPORTS =$(MOZCE_TOOLS_BIN_DIR)/arm-wince-as.exe \

Don't use EXPORTS, that has a specific meaning in the build system--headers to wind up in dist/include. Pick a different variable name here.

Looks good otherwise, r=me with those changes.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/904abd7c7527 and http://hg.mozilla.org/mozilla-central/rev/48d8667789b0

(sorry, missed some of the changes for ted's comments in the first push)

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 365548 [details] [diff] [review]
updated based on ted's comments

clearing the review request because this is not part of the build

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff8eb1029c69 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b7d23e068c85