493008 - Port |Bug 472093 - fix build system to use NTDDI_VERSION instead of random checks| to comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

'ac_add_options --with-windows-version=502' gives (me)
{
Bug 472093 comment 39:

configure: error: Invalid value --with-windows-version, must be 400, 500 or 501
}
due to old c-c and new m-c mismatch.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Part 1, define MOZ_WINSDK_MAXVER to be highest Windows version supported by the SDK [Checkin: Comment 16]

This part is safe to take as is, even if m-1.9.1 doesn't have it.

Comment 2

[Robert Kaiser]

Why do we need to port this at all when 1.9.1 doesn't have it?

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Why do we need to port this at all when 1.9.1 doesn't have it?

Because "I" can't build using m-c with the 'Windows Server 2003 R2' SDK anymore,
and I guess we'll want to restore this option when c-c branches anyway:
I can use an "ifndef 1.9.1" if you prefer.

Comment 4

[Robert Kaiser]

Ah, so this is needed for building with mozilla-central but building with mozilla-1.9.1 still works with the current/old code?

What I want to avoid is that c-c configure tells the user everything works but mozilla configure fails. Maybe I actually don't need to care about that though.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Ah, so this is needed for building with mozilla-central

Yes.

> but building with mozilla-1.9.1 still works with the current/old code?

Yes (it should): bug 472093 won't land on 1.9.1 :-|

> What I want to avoid is that c-c configure tells the user everything works but
> mozilla configure fails. Maybe I actually don't need to care about that though.

Sure, I'm paying attention that c-c should continue to work with both m-1.9.1 and m-c.

Comment 6

[Robert Kaiser]

Comment on attachment 377468 [details] [diff] [review]
(Av1) Part 1, define MOZ_WINSDK_MAXVER to be highest Windows version supported by the SDK
[Checkin: Comment 16]

If it makes things work, I can't see a real reason not to take it. I still wonder why Thunderbird boxes aren't running into this problem you're seeing, and I don't really understand what we are setting this var for, so I'd like a second opinion here.

Mark, can you check if it's OK to take this?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> why Thunderbird boxes aren't running into this problem you're seeing,

I would guess because TB boxes do not (need to) disable the Vista features.

> and I don't really understand what we are setting this var for

For nothing yet: it will be used by following patches.

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> (In reply to comment #6)
> > and I don't really understand what we are setting this var for
> 
> For nothing yet: it will be used by following patches.

Erm, we need more to get things working? I'm not sure I like that! Can I at least get some clue of what you want to stuff in here?

Comment 9

[Serge Gautherie (:sgautherie)]

Just the 3 patches from bug 472093: at least the blocking parts (until c-c branches).

Comment 10

[Robert Kaiser]

Could you please give us that as one consolidated patch or at least attach and request review for everything needed at once? Going with one single thing and expecting us to swallow it and then say "oh, but we need more" is not really a good idea.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Part 2, define MOZ_WINSDK_TARGETVER and error out if the SDK is too old [Checkin: Comment 19]

I had to move MOZILLA_1_9_1_BRANCH definition block up.


(In reply to comment #10)
> expecting us to swallow it and then say "oh, but we need more"

?!
The first patch is explicitly labeled as 'Part 1',
and my idea was only not to loose time preparing more patches if you would have say no to that first one... :-|

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Part 3, add MOZ_NTDDI_* defines [Checkin: Comment 20]

The 'switch code over to use these defines' will be a follow-up bug, to do after c-c has branched.

Comment 13

[Robert Kaiser]

Comment on attachment 377985 [details] [diff] [review]
(Bv1) Part 2, define MOZ_WINSDK_TARGETVER and error out if the SDK is too old
[Checkin: Comment 19]

I'd like Mark's opinion on this one as well, esp. for what it does with MOZILLA_1_9_1_BRANCH (move the block around, last hunk, etc.)

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 377468 [details] [diff] [review]
(Av1) Part 1, define MOZ_WINSDK_MAXVER to be highest Windows version supported by the SDK
[Checkin: Comment 16]

sr=Standard8

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 377985 [details] [diff] [review]
(Bv1) Part 2, define MOZ_WINSDK_TARGETVER and error out if the SDK is too old
[Checkin: Comment 19]

I need to take another look at this tomorrow, but in the meantime, I spotted this:

>@@ -5516,16 +5591,18 @@ MOZ_ARG_DISABLE_BOOL(parental-controls,
> [  --disable-parental-controls
>                            Do not build parental controls],
>    MOZ_DISABLE_PARENTAL_CONTROLS=1,
>    MOZ_DISABLE_PARENTAL_CONTROLS=)
> if test -n "$MOZ_DISABLE_PARENTAL_CONTROLS"; then
>     AC_DEFINE(MOZ_DISABLE_PARENTAL_CONTROLS)
> fi
> 
>+if test "$MOZILLA_1_9_1_BRANCH" = "1"; then
>+
> dnl ========================================================
> dnl Vista SDK specific api
> dnl ========================================================
> MOZ_ARG_DISABLE_BOOL(vista-sdk-requirements,
> [  --disable-vista-sdk-requirements
>                            Do not build Vista SDK specific code],
>    MOZ_DISABLE_VISTA_SDK_REQUIREMENTS=1,
>    MOZ_DISABLE_VISTA_SDK_REQUIREMENTS=)
>@@ -5545,16 +5622,22 @@ case "$target" in
>     fi
>     ;;
> esac
> fi
> fi
> AC_SUBST(MOZ_DISABLE_PARENTAL_CONTROLS)
> AC_SUBST(MOZ_DISABLE_VISTA_SDK_REQUIREMENTS)
> 
>+else
>+AC_SUBST(MOZ_DISABLE_PARENTAL_CONTROLS)
>+dnl Dumb statement, not to have an empty 'else' block.
>+MOZILLA_1_9_1_BRANCH=$MOZILLA_1_9_1_BRANCH
>+fi

MOZ_DISABLE_PARENTAL_CONTROLS applies whether or not we're on the 1.9.1 branch, so just move the AC_SUBST to after the check for --disable-parental-controls section.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 377468 [details] [diff] [review]
(Av1) Part 1, define MOZ_WINSDK_MAXVER to be highest Windows version supported by the SDK
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/59bb99cbe97c

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> MOZ_DISABLE_PARENTAL_CONTROLS applies whether or not we're on the 1.9.1 branch,
> so just move the AC_SUBST to after the check for --disable-parental-controls
> section.

I'll do. (I did it that way not to touch the existing lines only...)

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 377985 [details] [diff] [review]
(Bv1) Part 2, define MOZ_WINSDK_TARGETVER and error out if the SDK is too old
[Checkin: Comment 19]

sr=Standard8 (not my email address please) with the change mentioned in comment 15.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 377985 [details] [diff] [review]
(Bv1) Part 2, define MOZ_WINSDK_TARGETVER and error out if the SDK is too old
[Checkin: Comment 19]


http://hg.mozilla.org/comm-central/rev/dda9efe76799

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 377986 [details] [diff] [review]
(Cv1) Part 3, add MOZ_NTDDI_* defines
[Checkin: Comment 20]


http://hg.mozilla.org/comm-central/rev/a4ee7c0f12ec

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> The 'switch code over to use these defines' will be a follow-up bug, to do
> after c-c has branched.

I filed bug 496225.