474737 - Windows ce tools refactor

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

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

Attachment: all changes in the queue rolled up

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 358122 [details] [diff] [review]
all changes in the queue rolled up

the only weird part was:

-    if test -z "$AS_BIN"; then
+    if test "$AS_BIN"; then

Maybe add a comment.

Comment 2

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

(In reply to comment #1)
> (From update of attachment 358122 [details] [diff] [review])
> the only weird part was:
> 
> -    if test -z "$AS_BIN"; then
> +    if test "$AS_BIN"; then
> 
> Maybe add a comment.

actually, that hunk was accidentally picked up from bug 472165, so should be dropped from this patch anyway.

Comment 3

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

Attachment: drops AS_BIN change, adds build/Makefile.in change

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 358197 [details] [diff] [review]
drops AS_BIN change, adds build/Makefile.in change

+++ b/build/Makefile.in

+ifdef WINCE
+DIRS += wince
 endif

Just do:
DIRS += wince/tools wince/shunt
and drop build/wince/Makefile.in entirely. No point in having makefiles that just contain DIRS.

+++ b/build/wince/shunt/Makefile.in
+OBJDIR = $(DEPTH)

Are you actually using this variable anywhere?

+EXPORTS += build/vs$(MOZ_MSVCVERSION)/mozce_shunt.lib \
+	include/windows.h \

Having a .lib in EXPORTS does not seem right. What are you trying to do there? Also, you don't need to += EXPORTS, you can just set it = (same with DIRS below). Finally, don't use tabs in the continuation lines, they're unnecessary. Just use two spaces.

+SUBMAKEFILES += include/sys/Makefile

You don't need this. Also, you could list that Makefile in allmakefiles.sh.

+DEVENV_FLAG=-

This seems like overkill. Is there any reason not to use a literal - in the *_SWITCH definitions? Actually, the _SWITCH definitions look like overkill too, and you only appear to be using BUILD_SWITCH.

+$(srcdir)/build/vs$(MOZ_MSVCVERSION)/mozce_shunt.dll:
+	devenv $(MOZCE_SHUNT_SLN) $(BUILD_SWITCH) $(MOZCE_PROJECT)

So this gets built in-place in the srcdir? That's unfortunate. Is there any way we can convince devenv to build it in the objdir? Not a deal-breaker but we try to avoid putting junk in the srcdir, since it makes it much harder to clobber.

+MODULE = shunt/sys

That feels...wrong. I guess you want these headers to wind up in $(DIST)/include/shunt/sys ? I'd rather you just wrote your own export:: rule to do that instead.

+CFLAGS += -DVC_PATH='"$(subst \,\\,$(VCINSTALLDIR))\\"'
+CFLAGS += -DWM_SDK_PATH='"$(subst \,\\,$(WINCE_SDK))\\"'

Could you write these as one line with a continuation?

CFLAGS += \
 -D ... \

Also, are all these variables from MozillaBuild or what? You might want to explicitly check that they're set to something, and $(error) if not. I don't mind having MozillaBuild be a requirement for the wince build, but we should make it explicit.

+++ b/build/wince/tools/Makefile.in

So you have a build/wince/tools/Makefile and a Makefile.in? Does this not confuse make? Are you just doing this so you can have the build recurse into that dir and check dependencies properly? It feels like there has to be a better way do to things.

+++ b/build/wince/tools/arm-wince-as.c

-  dumpargs(args);
+  //  dumpargs(args);

Just remove it, don't comment it out.

+++ b/configure.in
     mk_add_options WINCE=1

Kill that mk_add_options while you're in there.

Comment 5

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

(In reply to comment #4)

> +++ b/build/wince/shunt/Makefile.in
> +OBJDIR = $(DEPTH)
> 
> Are you actually using this variable anywhere?

in build/wince/tools/Makefile (included from build/wince/tools/Makefile.in)


> 
> +$(srcdir)/build/vs$(MOZ_MSVCVERSION)/mozce_shunt.dll:
> +    devenv $(MOZCE_SHUNT_SLN) $(BUILD_SWITCH) $(MOZCE_PROJECT)
> 
> So this gets built in-place in the srcdir? That's unfortunate. Is there any way
> we can convince devenv to build it in the objdir? Not a deal-breaker but we try
> to avoid putting junk in the srcdir, since it makes it much harder to clobber.
> 

This patch fixes a lot of building in-place.  I'm not sure if its possible to get devenv to build in the objdir, but its something I'd like to get done so I'd be happy to look into it in a follow up bug.



> +++ b/build/wince/tools/Makefile.in
> 
> So you have a build/wince/tools/Makefile and a Makefile.in? Does this not
> confuse make? Are you just doing this so you can have the build recurse into
> that dir and check dependencies properly? It feels like there has to be a
> better way do to things.

Turns out it doesn't confuse make.  The reason I do this is configure needs to be able to check arm-wince-gcc, but we'd also like build to recurse into that dir and check dependencies.  If you can suggest a better way, I'm all ears.

Comment 6

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

Attachment: updated based on ted's comments

Comment 7

[(not currently active) Ted Mielczarek]

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

+libs:: $(MOZCE_SHUNT_BUILDDIR)/mozce_shunt.dll $(MOZCE_SHUNT_BUILDDIR)/mozce_shunt.lib
+	$(INSTALL) $(MOZCE_SHUNT_BUILDDIR)/mozce_shunt.dll  $(DIST)/bin
+	$(INSTALL) $(MOZCE_SHUNT_BUILDDIR)/mozce_shunt.lib  $(DIST)/lib

You can shorten this to one call to $(INSTALL), like:
$(INSTALL) $^ $(DIST)/lib
$^ is "all the prerequisites of this rule". You do things like this in a few other places, might as well shorten them too.

+++ b/build/wince/shunt/include/windows.h.in
+++ b/build/wince/shunt/include/ymath.h.in

It might be nice to include comments mentioning the bug numbers of what bug you're fixing with the workarounds in these header files, since the blame won't be useful if you're going to roll that all up in this one patch. (Unless you want to commit those bits as separate changesets, your choice.)

+++ b/build/wince/tools/Makefile
+ifeq ($(VCINSTALLDIR),)
+$(error)
+endif

$(error Environment variable VCINSTALLDIR not set! Are you using MozillaBuild?)

same for the other cases of $(error).

+CFLAGS += -DVC_PATH='"$(subst \,\\,$(VCINSTALLDIR))\\"' \
+  -DWM_SDK_PATH='"$(subst \,\\,$(WINCE_SDK))\\"' \

My preferred style in cases like this is:
CFLAGS += \
  -DVC_PATH='"$(subst \,\\,$(VCINSTALLDIR))\\"' \
  -DWM_SDK_PATH='"$(subst \,\\,$(WINCE_SDK))\\"' \

+++ b/configure.in

     MOZ_ARG_WITH_STRING(wince-sdk,
     [  --with-wince-sdk=WINCE_SDK
                         The path to the windows mobile sdk],
     WINCE_SDK=$withval)

Could you rename this variable to WINCE_SDK_DIR like I asked for in the bug that added this?

r=me with those nits addressed.

Comment 8

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

pushed http://hg.mozilla.org/mozilla-central/rev/69502a5ec1af

Comment 9

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

(In reply to comment #5)
> (In reply to comment #4)

> > 
> > +$(srcdir)/build/vs$(MOZ_MSVCVERSION)/mozce_shunt.dll:
> > +    devenv $(MOZCE_SHUNT_SLN) $(BUILD_SWITCH) $(MOZCE_PROJECT)
> > 
> > So this gets built in-place in the srcdir? That's unfortunate. Is there any way
> > we can convince devenv to build it in the objdir? Not a deal-breaker but we try
> > to avoid putting junk in the srcdir, since it makes it much harder to clobber.
> > 
> 
> This patch fixes a lot of building in-place.  I'm not sure if its possible to
> get devenv to build in the objdir, but its something I'd like to get done so
> I'd be happy to look into it in a follow up bug.
> 

bug 476201 filed as a follow up

Comment 11

[Doug Turner (:dougt)]

brad, when you land this on 1.9.1, please also land:

https://bugzilla.mozilla.org/show_bug.cgi?id=481780

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 13

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

pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d88305edf63d