Last Comment Bug 737969 - Land Win8 metro build config changes on mozilla-central
: Land Win8 metro build config changes on mozilla-central
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 8.1
: -- normal (vote)
: mozilla15
Assigned To: Jim Mathies [:jimm]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: elm-merge
  Show dependency treegraph
 
Reported: 2012-03-21 11:46 PDT by Jim Mathies [:jimm]
Modified: 2014-07-24 11:06 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
changes (8.08 KB, patch)
2012-03-21 17:12 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
changes (7.64 KB, patch)
2012-03-21 17:15 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
build config (7.90 KB, patch)
2012-03-23 10:14 PDT, Jim Mathies [:jimm]
ted: review+
Details | Diff | Splinter Review
updated patch (8.28 KB, patch)
2012-04-16 05:12 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
updated patch (9.75 KB, patch)
2012-04-16 06:12 PDT, Jim Mathies [:jimm]
ted: review+
Details | Diff | Splinter Review
makepri patch (754 bytes, patch)
2012-06-13 04:51 PDT, Jim Mathies [:jimm]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2012-03-21 11:46:20 PDT
I need to post the patches necessary here and get official reviews.
Comment 2 Jim Mathies [:jimm] 2012-03-21 17:12:04 PDT
Created attachment 608151 [details] [diff] [review]
changes
Comment 3 Jim Mathies [:jimm] 2012-03-21 17:15:41 PDT
Created attachment 608154 [details] [diff] [review]
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] 2012-03-23 05:17:33 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-03-23 08:13:25 PDT
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] 2012-03-23 08:18:43 PDT
(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] 2012-03-23 08:31:42 PDT
(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] 2012-03-23 10:14:44 PDT
Created attachment 608745 [details] [diff] [review]
build config
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-03-27 07:21:29 PDT
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] 2012-04-16 05:12:45 PDT
Created attachment 615307 [details] [diff] [review]
updated patch
Comment 11 Jim Mathies [:jimm] 2012-04-16 06:12:58 PDT
Created attachment 615315 [details] [diff] [review]
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] 2012-04-16 07:12:17 PDT
Comment on attachment 615315 [details] [diff] [review]
updated patch

Probably best to get another r? for the updates and minor additions.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2012-04-23 06:03:28 PDT
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 16 Mike Hommey [:glandium] 2012-06-13 02:59:37 PDT
Note that MAKEPRI is not defined in autoconf.mk.in, and as such, won't be available in Makefiles.
Comment 17 Jim Mathies [:jimm] 2012-06-13 04:51:03 PDT
Created attachment 632641 [details] [diff] [review]
makepri patch

Nice catch, this change is on elm but I didn't get it in with the patches here.
Comment 19 Matt Brubeck (:mbrubeck) 2012-06-13 13:29:24 PDT
https://hg.mozilla.org/mozilla-central/rev/d9c95c879541

Note You need to log in before you can comment on or make changes to this bug.