737969 - Land Win8 metro build config changes on mozilla-central

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jim Mathies [:jimm]]

I need to post the patches necessary here and get official reviews.

Comment 1

[Jim Mathies [:jimm]]

http://hg.mozilla.org/projects/elm/rev/6abe25dbc66f
http://hg.mozilla.org/projects/elm/rev/a9cf7747488c

Comment 2

[Jim Mathies [:jimm]]

Attachment: changes

Comment 3

[Jim Mathies [:jimm]]

Attachment: changes

This adds support for the 602 sdk and a new MOZ_WIN8METRO define we've been using to wrap winrt code / build stuff.

Comment 4

[Jim Mathies [:jimm]]

I'm going to remove the '8' from the define and the build option. In retrospect there's no point in having the version number in there.

Comment 5

[(not currently active) Ted Mielczarek]

I would probably just simplify that even more, to "MOZ_METRO / --enable-metro". Doesn't seem like there's much possibility for confusion. You should also make your configure option error out if the target OS is not Windows.

Do we need a preprocessor define for targeting Metro? If so I'd suggest XP_METRO to be in-line with our existing defines.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted] from comment #5)
> I would probably just simplify that even more, to "MOZ_METRO /
> --enable-metro". Doesn't seem like there's much possibility for confusion.
> You should also make your configure option error out if the target OS is not
> Windows.
> 
> Do we need a preprocessor define for targeting Metro? If so I'd suggest
> XP_METRO to be in-line with our existing defines.

I'm currently exporting 'MOZ_WIN8METRO' to mozilla_config.h and through autoconf.mk.in for use in makefiles and code. By "preprocessor define" I'm guessing this is what you are referring to? I'll go with your suggestions and fix these patches up.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #6)
> (In reply to Ted Mielczarek [:ted] from comment #5)
> > I would probably just simplify that even more, to "MOZ_METRO /
> > --enable-metro". Doesn't seem like there's much possibility for confusion.
> > You should also make your configure option error out if the target OS is not
> > Windows.
> > 
> > Do we need a preprocessor define for targeting Metro? If so I'd suggest
> > XP_METRO to be in-line with our existing defines.
> 
> I'm currently exporting 'MOZ_WIN8METRO' to mozilla_config.h and through
> autoconf.mk.in for use in makefiles and code. By "preprocessor define" I'm
> guessing this is what you are referring to? I'll go with your suggestions
> and fix these patches up.

Actually I don't think we want XP_METRO, we rely on XP_WIN for all metro code, and MOZ_METRO for selectively choosing what windows code we build and for make options.

Comment 8

[Jim Mathies [:jimm]]

Attachment: build config

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 608745 [details] [diff] [review]
build config

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

r=me with a couple of tweaks.

::: config/autoconf.mk.in
@@ +545,5 @@
>  MOZ_ENABLE_DWRITE_FONT	= @MOZ_ENABLE_DWRITE_FONT@
>  MOZ_ENABLE_D2D_SURFACE	= @MOZ_ENABLE_D2D_SURFACE@
>  MOZ_ENABLE_D3D9_LAYER	= @MOZ_ENABLE_D3D9_LAYER@
>  MOZ_ENABLE_D3D10_LAYER  = @MOZ_ENABLE_D3D10_LAYER@
> +MOZ_METRO	= @MOZ_METRO@

nit: does this actually line up? Hard to tell with the diff viewer.

::: configure.in
@@ +702,5 @@
>  
>  MOZ_ARG_WITH_STRING(windows-version,
>  [  --with-windows-version=WINSDK_TARGETVER
>                            Highest Windows version to target using this SDK
> +                          602: Windows 8],

This is supposed to be a list of all the accepted values, so leave the old one in place and add the new one.

@@ +798,5 @@
>  
> +        dnl Confirm we have the pri tools on win8 builds
> +        if test -n "$MOZ_METRO"; then
> +          AC_MSG_CHECKING([for makepri])
> +          AC_CHECK_PROGS(MAKEPRI, makepri, "")

I think you need to explicitly check the value of $MAKEPRI after AC_CHECK_PROGS and error if it's missing. We don't do a very good job of that in our configure...

Comment 10

[Jim Mathies [:jimm]]

Attachment: updated patch

Comment 11

[Jim Mathies [:jimm]]

Attachment: updated patch

Adding a new check that makes sure the target and the tools match up right. I noticed I could add enable-metro in an msvc 10 terminal and configure would succeed. This won't work in practice sine we require the msvc 11 toolset to build metro code.

Output of the error looks like this:

configure: error: Your MOZ_MSVCVERSION equals 10 and you've enabled metro build support. You can't target metro without msvc 11 or higher. Disable metro support or switch to a newer set of tools.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 615315 [details] [diff] [review]
updated patch

Probably best to get another r? for the updates and minor additions.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 615315 [details] [diff] [review]
updated patch

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

Just a couple of minor nits:

::: configure.in
@@ +708,5 @@
>  
>  MOZ_ARG_WITH_STRING(windows-version,
>  [  --with-windows-version=WINSDK_TARGETVER
> +                          Windows SDK version to target. Lowest version
> +                          currently allowed is 601, highest is 602],

The previous comment was a bit more informative, mapping version numbers to products. Can you keep it in that style?

@@ +806,5 @@
> +        if test -n "$MOZ_METRO"; then
> +          AC_MSG_CHECKING([for makepri])
> +          AC_CHECK_PROGS(MAKEPRI, makepri, "")
> +          if test -z "MAKEPRI" ; then
> +              AC_MSG_ERROR([makepri.exe is required for generating metro browser install components. It should be in the 602 SDK.])

When you say "602 SDK" you mean "Windows 8 SDK" right? That would be a more useful error message.

Comment 14

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19f1387c2402

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/19f1387c2402

Comment 16

[Mike Hommey [:glandium]]

Note that MAKEPRI is not defined in autoconf.mk.in, and as such, won't be available in Makefiles.

Comment 17

[Jim Mathies [:jimm]]

Attachment: makepri patch

Nice catch, this change is on elm but I didn't get it in with the patches here.

Comment 18

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9c95c879541

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d9c95c879541