472093 - fix build system to use NTDDI_VERSION instead of random checks

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[timeless]

Attachment: switch configure to use NTDDI

the current system for building random bits that need system components on windows is totally broken.

what follows are a series of patches which replace the broken system with a correct system based on NTDDI (which is the proper way to do this)

Comment 1

[timeless]

Attachment: code changes for helper app

Comment 2

[timeless]

Attachment: code for stock icons

Comment 3

[timeless]

Attachment: code for parental controls

Comment 4

[timeless]

Attachment: code for application associations

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 355372 [details] [diff] [review]
switch configure to use NTDDI

+*)
+    AC_MSG_ERROR([Invalid value --with-windows-version, must be 500, 501 or 600]);

Do you think we need to keep a check for specific versions here, or could we just let the user pass any 3 digit number? Doesn't really bother me either way, I guess, just sucks to have to continually update stuff like this in the future. Patch looks fine otherwise.

Comment 6

[(not currently active) Ted Mielczarek]

Also, I think a peer of the code you're touching should review the rest of these patches.

Comment 7

[Nochum Sossonko [:Natch]]

adding dependency, so that when the win2k bug gets fixed, this should be updated.

Comment 8

[(not currently active) Ted Mielczarek]

Just curious, if you build with an SDK older than the Vista SDK, how does something like:
+#if NTDDI_VERSION >= NTDDI_LONGHORN
work? NTDDI_LONGHORN is probably undefined in that case, right? Will the preprocessor just error out on that?

Comment 9

[timeless]

ted, you're right (undefined items in #if are treated as 0, so the if's do the wrong thing for the case you described), the proper way to do it is this:

#define HAS_NTDDI_VERSION(X) X && (NTDDI_VERSION >= X)
#if HAS_NTDDI_VERSION(NTDDI_LONGHORN)

can you suggest a good place for a HAS_NTDDI_VERSION macro to live?

Comment 10

[Rain [:rain1] (inactive)]

Attachment: define MOZ_WINSDK_MAXVER instead of overloading NTDDI_VERSION

One of the problems with the above approach is that we're overloading NTDDI_VERSION to have two meanings:
- minimum version of windows that the program is guaranteed to run on (the meaning the SDK uses)
- maximum version of windows we build specific support for

Bumping up the NTDDI_VERSION or _WIN32_WINNT globally can cause problems with increased structure sizes, e.g. <http://blogs.msdn.com/oldnewthing/archive/2003/12/12/56061.aspx>, so there's a non-zero risk of bustage with older versions of Windows.

Another problem is that the Vista SDK is the first one to actually define NTDDI_VERSION. The older SDKs relied solely on _WIN32_WINNT and friends.

So, the Server 2008 and higher SDKs make it easy to find out the highest version we can have support for, using winsdkver.h. The Vista SDK has sdkddkver.h (which defines NTDDI_VERSION), and the older SDKs don't have either. The patch uses this information to figure out which version of the SDK is being used, and to define a constant MOZ_WINSDK_MAXVER based on it.

The reason I'm comparing the two values instead of just using NTDDI_MAXVER is that the Win7 beta SDK defines NTDDI_MAXVER to be 0x06000100 (Vista SP1 == Server 2008) instead of what it should be, 0x06010000. This will probably be fixed in the final SDK, though, so I'm fine with removing that check.

If MOZ_WINSDK_MAXVER is now substituted for NTDDI_VERSION, things should work. Since the Server 2003 SDK is the oldest we support, the HAS_NTDDI_VERSION macro will mean that we don't have to worry about NTDDI_* not being defined.

Tested, works fine with both the Win7 and the Vista SDKs. I'm not sure what's accurate for mingw, so right now I've set it to Server 2003 (0x05020000).

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 360478 [details] [diff] [review]
define MOZ_WINSDK_MAXVER instead of overloading NTDDI_VERSION

+                            ac_cv_winsdk_maxver=`cl -E -nologo conftest.h 2>/dev/null | tail -n1`

Just use $CPP here, it's set correctly earlier in the file.

+                # Assume the Server 2003 Platform SDK
+                MOZ_WINSDK_MAXVER=0x05020000

This is basically just an assumption here, right? I guess we don't actually have any existing checks that you're using a Win2k3 SDK, except that MozillaBuild doesn't look for anything older than that.

+#if (NTDDI_VERSION_FROM_WIN32_WINNT(_WIN32_WINNT_MAXVER) > NTDDI_MAXVER)

As discussed on IRC, I'd like a comment explaining that this is a workaround for the current broken Windows 7 SDK.

r=me with those comments addressed.

Comment 12

[Rain [:rain1] (inactive)]

(In reply to comment #11)
> +                # Assume the Server 2003 Platform SDK
> +                MOZ_WINSDK_MAXVER=0x05020000
> 
> This is basically just an assumption here, right? I guess we don't actually
> have any existing checks that you're using a Win2k3 SDK, except that
> MozillaBuild doesn't look for anything older than that.

Yeah, that's right.

According to Ted, our policy should be to not silently disable code if we can't build it. We should instead "error with an informative message telling you either a) what you need to install to build the code, or b) what you need to disable to not build that code".

In this case, we should target the Vista SDK by default, and error out if the Vista SDK is not present, telling him to either install the Vista SDK or target a lower version.

Comment 13

[Rain [:rain1] (inactive)]

Attachment: MOZ_WINSDK_MAXVER review nits fixed [Checkin: Comment 16]

Per discussion on IRC, I also removed the #define, as we're going to test against the target ver instead.

Comment 14

[Rain [:rain1] (inactive)]

Attachment: Part 2, define MOZ_WINSDK_TARGETVER + error out if SDK is too old

Comment 15

[Rain [:rain1] (inactive)]

Attachment: Part 2, define MOZ_WINSDK_TARGETVER + error out if SDK is too old, v2

The patch also defines both the targetver and MOZ_DISABLE_VISTA_SDK_REQUIREMENTS if one of them is set. The idea is to eventually get rid of the DISABLE_VISTA_SDK_REQUIREMENTS define and just check version numbers.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 363186 [details] [diff] [review]
MOZ_WINSDK_MAXVER review nits fixed
[Checkin: Comment 16]


http://hg.mozilla.org/mozilla-central/rev/d55509df48fa

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 363346 [details] [diff] [review]
Part 2, define MOZ_WINSDK_TARGETVER + error out if SDK is too old, v2

I don't want to add a --with-winsdk-target, we should just use the value of --with-windows-version (translated into a NTDDI_VERSION, ala timeless' patch) as the target value here.

+    AC_MSG_WARN([Resulting builds will not be compatible with Windows Vista. (bug 428970)])

No need for this, if someone is explicitly passing this option they ought to know they're not targeting Vista anymore.

+#if 0x$MOZ_WINSDK_TARGETVER > $MOZ_WINSDK_MAXVER
+#error "sdk not recent enough"
+#endif

This needs a way better error message. I might recommend that you make a small page on developer.mozilla.org, and provide a short error with the URL in this error.

Comment 18

[Rain [:rain1] (inactive)]

Attachment: Part 2, define MOZ_WINSDK_TARGETVER + error out if SDK is too old, v3

I've fixed Ted's nits, added a wiki page, and changed --with-windows-version to mean this. Since --with-windows-version isn't relevant in the JS configure.in any more, and JS doesn't support MOZ_DISABLE_VISTA_SDK_REQUIREMENTS, I've removed it from there entirely.

bsmedberg: since ted has indicated that he's quite busy, will it be fine if you review this patch? Thanks.

Comment 19

[Benjamin Smedberg]

Comment on attachment 366552 [details] [diff] [review]
Part 2, define MOZ_WINSDK_TARGETVER + error out if SDK is too old, v3

>diff --git a/configure.in b/configure.in

>+    # If the maximum version supported by this SDK is lower than the target
>+    # version, error out
>+    AC_MSG_CHECKING([for Windows SDK being recent enough])
>+    AC_TRY_COMPILE([],
>+#if 0x$MOZ_WINSDK_TARGETVER > $MOZ_WINSDK_MAXVER
>+#error "sdk not recent enough"
>+#endif

It has been my experience that AC_TRY_COMPILE doesn't work with MSVC at all... are you sure this does what you expect? And, aren't MOZ_WINSDK_TARGETVER and MOZ_WINSDK_MAXVER both shell variables? Why do you need AC_TRY_COMPILE instead of something simpler?

Comment 20

[Rain [:rain1] (inactive)]

(In reply to comment #19)
> (From update of attachment 366552 [details] [diff] [review])
> >diff --git a/configure.in b/configure.in
> 
> >+    # If the maximum version supported by this SDK is lower than the target
> >+    # version, error out
> >+    AC_MSG_CHECKING([for Windows SDK being recent enough])
> >+    AC_TRY_COMPILE([],
> >+#if 0x$MOZ_WINSDK_TARGETVER > $MOZ_WINSDK_MAXVER
> >+#error "sdk not recent enough"
> >+#endif
> 
> It has been my experience that AC_TRY_COMPILE doesn't work with MSVC at all...
> are you sure this does what you expect? 

Yes, this works fine, at least with MSVC9. I assumed it would work fine with earlier versions too, because of <http://mxr.mozilla.org/mozilla-central/source/configure.in#492>.

> And, aren't MOZ_WINSDK_TARGETVER and
> MOZ_WINSDK_MAXVER both shell variables? Why do you need AC_TRY_COMPILE instead
> of something simpler?

Is there a better way to compare two hexadecimal values? The trouble is that we extract the MOZ_WINSDK_MAXVER straight from the header file, so that has to have the leading 0x.

Comment 21

[Benjamin Smedberg]

perl -e "exit((0x01 < 0x01) ? 0 : 1)" or somesuch

Comment 22

[Rain [:rain1] (inactive)]

Attachment: Part 2, v4

Thanks -- I've changed the check to perl.

Comment 23

[Jim Mathies [:jimm]]

Can we get quick summary of how to use this? Also, once this last part lands, are we good to go for some win7 checkins?

Comment 24

[Rain [:rain1] (inactive)]

(In reply to comment #23)
> Can we get quick summary of how to use this?

We need to figure out where to place the HAS_NTDDI_VERSION macro, as mentioned in comment 9. Once that is done, we'll have to wrap (say) Win7 specific blocks with

#if HAS_NTDDI_VERSION(NTDDI_WIN7)

> Also, once this last part lands,
> are we good to go for some win7 checkins?

After the HAS_NTDDI_VERSION thing lands, sure.

Comment 25

[Rain [:rain1] (inactive)]

Attachment: part 2, merged to m-c tip [Checkin: Comment 33]

Comment 26

[Rain [:rain1] (inactive)]

Attachment: part 3, add NTDDI definitions to configure.in

Per a discussion with Ted, this shouldn't cause trouble.

Comment 27

[Rain [:rain1] (inactive)]

Attachment: Code changes for app associations

Comment 28

[Rain [:rain1] (inactive)]

Attachment: Code changes for the download manager/scanner

Jim, you seem to be the go-to guy in this area as well -- is it fine if you review this?

Comment 29

[Rain [:rain1] (inactive)]

Attachment: Code changes for the helper app service

Comment 30

[Rain [:rain1] (inactive)]

Attachment: Code changes for stock icons

These patches should cover all the disable-vista-sdk uses in m-c.

I've tested these, and these seem to work fine with both --with-windows-version=502 and 600.

I haven't touched parental controls because I've kept it as is in configure.in as well.

Comment 31

[Jim Mathies [:jimm]]

(In reply to comment #30)
> Created an attachment (id=372327) [details]
> Code changes for stock icons
> 
> These patches should cover all the disable-vista-sdk uses in m-c.
> 
> I've tested these, and these seem to work fine with both
> --with-windows-version=502 and 600.
> 
> I haven't touched parental controls because I've kept it as is in configure.in
> as well.

Are you planning on leaving that flag in for pc or migrating that over as well? I don't see any reason to split that out, seems like it should use this new scheme as well.

Comment 32

[Rain [:rain1] (inactive)]

(In reply to comment #31)
> (In reply to comment #30)
> > Created an attachment (id=372327) [details] [details]
> > Code changes for stock icons
> > 
> > These patches should cover all the disable-vista-sdk uses in m-c.
> > 
> > I've tested these, and these seem to work fine with both
> > --with-windows-version=502 and 600.
> > 
> > I haven't touched parental controls because I've kept it as is in configure.in
> > as well.
> 
> Are you planning on leaving that flag in for pc or migrating that over as well?
> I don't see any reason to split that out, seems like it should use this new
> scheme as well.

I presumed that
- we might add parental controls support for other OSes in the future
- someone might want to build with the Vista SDK but with parental controls disabled.

At least that's why I thought there were separate flags for parental controls and the Vista SDK -- maybe I'm wrong there. If you think the disable parental controls flag should be merged, I'll file a follow-up bug for it.

Comment 33

[Rain [:rain1] (inactive)]

Comment on attachment 372121 [details] [diff] [review]
part 2, merged to m-c tip
[Checkin: Comment 33]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/fcaf50dc12d2

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 372327 [details] [diff] [review]
Code changes for stock icons

You don't need to ask me for review on these patches that don't touch the build system, but this looks fine.

Comment 35

[Jim Mathies [:jimm]]

> - someone might want to build with the Vista SDK but with parental controls
> disabled.

That reason WFM. The original bug is bug 425979. 

I've got another patch coming for some code related to gesture support on win7 that landed a few weeks ago as well.

Comment 36

[Jim Mathies [:jimm]]

Attachment: Code changes for gesture support

Comment 37

[Rain [:rain1] (inactive)]

Comment on attachment 373868 [details] [diff] [review]
Code changes for gesture support

This errors out at nsWinGesture.h while building with the Windows 7 beta SDK, with --with-windows-version=601.

This isn't the right thing to do, as the gesture defines are included from the SDK headers only when NTDDI_VERSION is >= NTDDI_WIN7, and we aren't bumping it up here.

I see three options:
1. leave it as it is
2. make the check #if NTDDI_VERSION < MOZ_NTDDI_WIN7
3. bump NTDDI_VERSION in nsWinGesture.h up when MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7, though that's a bit riskier as future code won't have the natural checks that the lower NTDDI_VERSION provides.

I'd be fine with either 1 or 2.

Comment 38

[Rain [:rain1] (inactive)]

Attachment: [checkin: comment 41] patches to check in, combined

Jim's indicated that he'd like to leave the gestures code as it is.

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 372121 [details] [diff] [review]
part 2, merged to m-c tip
[Checkin: Comment 33]


What about 1.9.1?

I wanted to port this to comm-central, but I guess SeaMonkey needs 1.9.1 and 1.9.2 to be synchronized first?
{
configure: error: Invalid value --with-windows-version, must be 400, 500 or 501
}

Comment 40

[Rain [:rain1] (inactive)]

(In reply to comment #39)
> (From update of attachment 372121 [details] [diff] [review])
> 
> What about 1.9.1?
> 

If you'd like to port it to c-c, then yes, all the patches here need to be checked in to 1.9.1 as well. Not sure if this will get approval though.

Of course, the patch needs to be checked in to m-c before approval's asked, so it'd be great if you or someone else could do that. :)

Comment 41

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/047ec54f7a9d

Comment 42

[Rain [:rain1] (inactive)]

One advantage of having this in 1.9.1 is that it'll be a lot easier to backport some Win7 features from m-c, if we decide to do so.

All these patches are pretty low risk, and they shouldn't affect official builds, as they use neither --disable-vista-sdk-requirements nor --with-windows-version.

Comment 43

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

Comment on attachment 374083 [details] [diff] [review]
[checkin: comment 41] patches to check in, combined

I don't think we'll be backporting Windows 7 things to branch, unless you think there are security fixes that would require these changes.

Comment 44

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_DISABLE_VISTA_SDK_REQUIREMENTS&case=on&find=%2Fconf.*%5C.in%24

The option is kept as deprecated,
but why set/define/subst/... it anymore?