508905 - /Zc:wchar_t- is no longer required

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Masatoshi Kimura [:emk]]

Per bug 324842, it was added due to "This conflicts with various SDK headers that still want to define WCHAR". Is it still true with Vista SDK or later? I don't see any SDK errors without /Zc:wchar_t-.
I'm tired to see gazillions of MinGW build errors related to wchar_t.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: (Av1) patch in updater [Moved to bug 507338]

Comment 2

[Masatoshi Kimura [:emk]]

I could build fine even with 2003SP1 SDK (and all patches of dependent bugs).
So we have no reason to insist on using /Zc:wchar_t- anymore.

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

This will fix inconsistency with archivereader.h and readstrings.h

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

Looks good and thanks

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Jacek Caban]

It's not fixed yet. The committed patch fixed only a bug that this bug depends on.

Comment 7

[Masatoshi Kimura [:emk]]

Yes, I should have say that a follow-up patch will come. Reopening.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

Backed out... This doesn't work on WinCE.

http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1249892930.1249893343.6278.gz

Comment 9

[Jacek Caban]

It looks like windows.h (or winnt.h) needs to be included before progressui.h in progressui_null.cpp (or in progressui.h). I don't have setup to test it.

Comment 10

[Robert Strong (they/them - no direct email)]

I'm working on getting the progress ui working on WinCE / WinMo and could take care of this over in bug 507338. Sorry I didn't catch this error... turns out my work on bug 507338 makes it so this patch compiles fine at least with Firefox WinCE.

Comment 11

[Robert Strong (they/them - no direct email)]

bug 507338 landed on trunk and should land on the 1.9.2 branch in a couple of days

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: (Bv1) m-c patch

Although this patch works with mozilla-central alone, it will break c-c + m-c build.
We will need a temporary configure option which controls /Zc:wchar_t- flag.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: (Cv1-CC) comm-central patch

This is a temporary patch to avoid c-c + m-c build being broken. The else clause will be eventually removed.

Comment 14

[Robert Kaiser]

Comment on attachment 394664 [details] [diff] [review]
(Cv1-CC) comm-central patch

Oh, I see, you force this for now, then remove it from m-c then remove the else clause and make bot consistent again.

r=me on the condition that you will post a followup patch when the main one has landed on m-c to remove the else clause. Nit: please add a "(see bug 508905)" or so to the comment lines so that it's clear what this does.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: (Cv1a-CC) added bug number per review comment

Thank you!

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: (Dv1-CC) followup-patch after m-c landing

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

This patch needs to be landed after attachment 394682 [details] [diff] [review] and before 394683.

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

pending review until a build error caused by 493280 is fixed.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

The fix was pushed. Requesting review again.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 394682 [details] [diff] [review]
(Cv1a-CC) added bug number per review comment

Could you remove the MOZILLA_1_9_1_BRANCH checks now that c-c has branched? Thanks.

Comment 21

[Masatoshi Kimura [:emk]]

Unfortunately, trunk will be broken again if the patch is applied :(

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: (Cv2-CC) removed MOZILLA_1_9_1_BRANCH checks

Someone seems to have fixed offending codes.
Carrying forward r+.

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: (Dv2-CC) followup-patch after m-c landing

Comment 24

[Serge Gautherie (:sgautherie)]

Could you summarize which patch will land where/when/why? Thanks.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to comment #24)
1. Land attachment #408276 [details] [diff] [review] on comm-central first. This patch will force /Zc:wchar_t- to subdir.
2. If tinderbox doesn't burn, land attachment #394524 [details] [diff] [review] on mozilla-central. This patch removes /Zc:wchar_t- from m-c. c-c will be built with /Zc:wchar_t- at this point.
3. If tinderbox doesn't burn, land attachment #408277 [details] [diff] [review] on comm-central. /Zc:wchar_t- will be removed from c-c also.
The order is important to avoid c-c build being broken.

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)

> (In reply to comment #24)
> 1. Land attachment #408276 [details] [diff] [review] on comm-central first. This patch will force
> /Zc:wchar_t- to subdir.

If you're unsure that this patch will pass, then it should probably be submitted to c-c Try server first, should it not?

> 2. If tinderbox doesn't burn, land attachment #394524 [details] [diff] [review] on mozilla-central. This
> patch removes /Zc:wchar_t- from m-c. c-c will be built with /Zc:wchar_t- at
> this point.

Was that tried at least locally, for m-c alone then c-c + m-c?

> 3. If tinderbox doesn't burn, land attachment #408277 [details] [diff] [review] on comm-central.
> /Zc:wchar_t- will be removed from c-c also.
> The order is important to avoid c-c build being broken.

That said,
isn't it possible to do all this the other way round: just removing existing code(s)?
(I guess not, but why?)

Comment 27

[Masatoshi Kimura [:emk]]

(In reply to comment #26)
> If you're unsure that this patch will pass,
I believe the patch will pass. I just worried about unexpected contingency.

> then it should probably be
> submitted to c-c Try server first, should it not?
How to get a try server access? (Sorry for the dumb question.)

> Was that tried at least locally, for m-c alone then c-c + m-c?
Yes. I've tested all of combinations locally.

> That said,
> isn't it possible to do all this the other way round: just removing existing
> code(s)?
I'm concerned about timing issue. Is it possible to push the patches to c-c and m-c at the same time?

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> (In reply to comment #26)

> > submitted to c-c Try server first, should it not?
> How to get a try server access? (Sorry for the dumb question.)

https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer#Gaining_Access

> > isn't it possible to do all this the other way round: just removing existing
> > code(s)?
> I'm concerned about timing issue. Is it possible to push the patches to c-c and
> m-c at the same time?

Yes, each patch could be pushed to its respective repo one (right) after the other.

Unless there is some kind of need for clobber or depend builds that would brake and not recover otherwise, the two target patches should just be pushed directly, at the "same" time if you say so ;-)
(A transient red/orange (on c-c) would be no problem.)

Comment 29

[Phil Ringnalda (:philor)]

attachment 394524 [details] [diff] [review] breaks WinCE and WinMo on the tryserver:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260732762.1260733959.9775.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260732762.1260733773.7638.gz&fulltext=1

Comment 30

[Jacek Caban]

Attachment: (Ev1) fix for wince

The attached patch should fix the problem, but I don't have compile environment to test it. (Also there are some duplications between environment.h and mozce_shunt.h and GetEnvironmentVariableW returns char instead of DWORD, but these are not related problems).

Comment 31

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260737382.1260738263.26275.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260737382.1260738263.26276.gz&fulltext=1

Isn't poking at things blindly fun? :)

Comment 32

[Jacek Caban]

Yeah :) I guess I might try setting up an environment if there will be more problems. I will attach another try that should fix this issue using wchar_t instead of WCHAR in header files as I've found that mozce_shunt.h is included by command line options, so WCHAR is not yet defined then.

Comment 33

[Jacek Caban]

Attachment: (Ev2) fix for wince

Another try

Comment 34

[Phil Ringnalda (:philor)]

Another 100+ errors - http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260742122.1260742781.12598.gz&fulltext=1

Comment 35

[Jacek Caban]

Attachment: (Ev3) Final fix

I've setup an environment for WinCE building and the attached patch works for me.

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 417929 [details] [diff] [review]
(Ev3) Final fix

at quick glance, this is a "unsigned short" -> WCHAR change.  Why is it important?

Comment 37

[Jacek Caban]

wchar_t is usually a builtin compiler type, so it's not the same as unsigned short and explicit casts are needed when we mix wchar_t and unsigned short. It's not true in MSC if you pass /Zc:wchar_t- option and that's what's currently done. It causes patches without explicit casts to be accepted. MinGW uses builtin type and thus compilation on mingw break often. We want to get rid of /Zc:wchar_t- so that both compilers will tread wchar_t the same way (another argument is a technical correctness).

Comment 38

[Jacek Caban]

Doug, do you need more explanation? Could you please review the patch?

Comment 39

[Doug Turner (:dougt)]

Comment on attachment 417929 [details] [diff] [review]
(Ev3) Final fix

why do we need to use both WCHAR and wchar_t?

I think the functions that we expose should match whatever msdn does.  For example:

_wgetcwd  should be 

wchar_t *_wgetcwd( 
   wchar_t *buffer,
   int maxlen 
);

that is, wchar_t not WCHAR.  is there a problem doing it this way?

Comment 40

[Jacek Caban]

Attachment: (Ev4) fixed wchar_t/WCHAR [Checked in: Comment 57]

Thanks for review, I've changed it. We can't change wchar_t->WCHAR in headers of functions like GetEnvironmentVariable in headers, because they are used in context that doesn't have WCHAR (and LPWSTR/LPCWSTR) defined. I don't think it's something we should care about.

There are other things that differ from MSDN like char instead of BOOL (that is defined as int), but it's out of scope of the problem this patch is meant to fix.

Comment 41

[Jacek Caban]

Attachment: (Bv2) updated m-c patch

Thanks for review. I'm attaching updated m-c patch, that may be landed after "fixed wchar_t/WCHAR" (preferable with try server check first).

Comment 42

[Justin Wood (:Callek)]

With both these patches applied; I pushed a try build, and it failed on windows:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272347054.1272350184.647.gz&fulltext=1

e:/builds/moz2_slave/win32-hg/build/gfx/cairo/cairo/src/cairo-dwrite-font.cpp(280) : error C2664: 'DWriteFactory::FindSystemFontFamily' : cannot convert parameter 1 from 'uint16_t *' to 'const WCHAR *'
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

(among other warnings).

Once it is safe to push please add the keyword again.

Comment 43

[Serge Gautherie (:sgautherie)]

(In reply to comment #42)
> With both these patches applied; I pushed a try build, and it failed on
> windows:

Interesting: how do you add a m-c patch to a MozillaTry build?

Comment 44

[Serge Gautherie (:sgautherie)]

(In reply to comment #43)
> Interesting: how do you add a m-c patch to a MozillaTry build?

Never mind: I misread MozillaTry as ThunderbirdTry :-<

Comment 45

[Jacek Caban]

Attachment: (Fv1) cairo-dwrite fix

The attached patch fixes the cairo problem. This patch together with my fix from bug 449292 allows landing this bug (tested on try server).

Comment 46

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 451008 [details] [diff] [review]
(Fv1) cairo-dwrite fix

>diff --git a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>index 127ea85..0f12fbd 100644
>--- a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>+++ b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>@@ -277,7 +277,7 @@ _cairo_dwrite_font_face_create_for_toy (cairo_toy_font_face_t   *toy_face,
>     status = _cairo_utf8_to_utf16 (toy_face->family, -1,
> 				   &face_name, &face_name_len);
> 
>-    IDWriteFontFamily *family = DWriteFactory::FindSystemFontFamily(face_name);
>+    IDWriteFontFamily *family = DWriteFactory::FindSystemFontFamily(reinterpret_cast<wchar_t*>(face_name));
>     if (!family) {
> 	*font_face = (cairo_font_face_t*)&_cairo_font_face_nil;
> 	return CAIRO_STATUS_FONT_TYPE_MISMATCH;

Perhaps we can use MultiByteToWideChar instead of _cairo_utf8_to_utf16 so that we get the right type and don't need to cast?

Comment 47

[Jacek Caban]

Attachment: (Fv2) cairo-dwrite fix x.1.1 [Checked in: Comment 57]

Thanks for the review. The attached version uses MultiByteToWideChar (also fixes memory leak in error path).

Comment 48

[Jacek Caban]

Patches from comment 40 and comment 47 can be checked in. After landing the patch from bug 449292 comment 146, the final patch from comment 41 can be landed. I'd appreciate quick checkin to avoid another breakages preventing landing the final patch.

Comment 49

[[e-mail]]

Attachment: (Gv1) Harfbuzz fix [Moved to bug 449292]

hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*

Comment 50

[[e-mail]]

Comment on attachment 424234 [details] [diff] [review]
(Bv2) updated m-c patch

>@@ -558,7 +558,6 @@ case "$target" in
>             _CC_SUITE=7
>         elif test "$_CC_MAJOR_VERSION" = "14"; then
>             _CC_SUITE=8
>-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7 block? (-Zc:wchar_t- is the default for that version.)

Comment 51

[Jacek Caban]

(In reply to comment #49)
> Created an attachment (id=452219) [details]
> Harfbuzz fix
> 
> hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*

This is what bug 449292 comment 146 is about.

(In reply to comment #50)
> (From update of attachment 424234 [details] [diff] [review])
> >@@ -558,7 +558,6 @@ case "$target" in
> >             _CC_SUITE=7
> >         elif test "$_CC_MAJOR_VERSION" = "14"; then
> >             _CC_SUITE=8
> >-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
> Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> block? (-Zc:wchar_t- is the default for that version.)

I think it should be possible, but I don't have one to test.

Comment 52

[[e-mail]]

(In reply to comment #51)
> (In reply to comment #49)
> > Created an attachment (id=452219)
> > Harfbuzz fix
> > hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*
> This is what bug 449292 comment 146 is about.
Sorry for the duplication.

> (In reply to comment #50)
> > (From update of attachment 424234 [details] [diff] [review])
> > >@@ -558,7 +558,6 @@ case "$target" in
> > >             _CC_SUITE=7
> > >         elif test "$_CC_MAJOR_VERSION" = "14"; then
> > >             _CC_SUITE=8
> > >-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
> > Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> > block? (-Zc:wchar_t- is the default for that version.)
> I think it should be possible, but I don't have one to test.
How do you think I found the Harfbuzz compile error ;-)

(In reply to comment #0)
> Per bug 324842, it was added due to "This conflicts with various SDK headers
> that still want to define WCHAR". Is it still true with Vista SDK or later? I
> don't see any SDK errors without /Zc:wchar_t-.
I think VC7.1 was partly converted to wchar_t - WinNT.h uses it, but MAPIDefS.h and WabDefs.h still used WORD; the 2003 SDK uses wchar_t throughout.

Comment 53

[Jacek Caban]

Attachment: (Bv3) the patch with wchar_t enabled for _CC_SUITE=7 [Backed out: Comment 63]

> > > Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> > > block? (-Zc:wchar_t- is the default for that version.)
> > I think it should be possible, but I don't have one to test.
> How do you think I found the Harfbuzz compile error ;-)

Let's add it then :)

Comment 54

[Masatoshi Kimura [:emk]]

Reassigning to an actual current contributer. Thank you, Jacek!
(In reply to comment #52)
> (In reply to comment #0)
> > Per bug 324842, it was added due to "This conflicts with various SDK headers
> > that still want to define WCHAR". Is it still true with Vista SDK or later? I
> > don't see any SDK errors without /Zc:wchar_t-.
> I think VC7.1 was partly converted to wchar_t - WinNT.h uses it, but MAPIDefS.h
> and WabDefs.h still used WORD; the 2003 SDK uses wchar_t throughout.
I only considered VC8 or later because /Zc:wchar_t- was the default for VC7.1.
I don't oppose adding a option for VC7.1 because it seems harmless.

Comment 55

[Masatoshi Kimura [:emk]]

Note that this patch will conflict with a patch for bug 570365.

Comment 56

[Jacek Caban]

Attachment: (Hv1) jetpack fix

Jetpack added new PRUnichar->jschar problem. The attached patch fixes it.

Comment 57

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/9ca328e60bdd
http://hg.mozilla.org/mozilla-central/rev/dceb0c65850a

The final configure.in patch is blocked by jetpack fix.

Comment 58

[Benjamin Smedberg]

Comment on attachment 454286 [details] [diff] [review]
(Hv1) jetpack fix

talk about ugly :-(

Comment 59

[Jacek Caban]

(In reply to comment #58)
> Comment on attachment 454286 [details] [diff] [review]
> jetpack fix
> 
> talk about ugly :-(

Thanks for the review. There is bug 578340 in hope to make things less ugly...

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you tested that sizeof(PRUnichar) is still 2 with this patch?

I think nsString.cpp should perhaps contain:

PR_STATIC_ASSERT(sizeof(PRUnichar) == 2)
PR_STATIC_ASSERT(sizeof(nsString::char_type) == 2)
PR_STATIC_ASSERT(sizeof(nsCString::char_type) == 1)

to double-check that we don't break those.

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll deal with comment 60 in a different bug, though.

Comment 62

[Jacek Caban]

Thanks. Pushed to m-c:

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

Comment 63

[Jacek Caban]

Reverted due to failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281204510.1281205892.28536.gz

http://hg.mozilla.org/mozilla-central/rev/45e0643ad28a

Comment 64

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

Comment on attachment 452314 [details] [diff] [review]
(Bv3) the patch with wchar_t enabled for _CC_SUITE=7
[Backed out: Comment 63]

(bookkeeping)

Comment 65

[Masatoshi Kimura [:emk]]

Attachment: (Bv4) unbitrotted

Let's try again.
Tryserver log:
http://tbpl.mozilla.org/?tree=Try&rev=53cdd1b87b28
Jetpack failures are known. Non-windows test failures are obviously unrelated.

Comment 66

[(not currently active) Ted Mielczarek]

Comment on attachment 529312 [details] [diff] [review]
(Bv4) unbitrotted

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

Comment 67

[Jacek Caban]

Pushed to b-s:

http://hg.mozilla.org/projects/build-system/rev/f8849773f7f4

Comment 68

[(not currently active) Ted Mielczarek]

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

Comment 69

[[e-mail]]

Is it expected that my build will need a clobber after updating to this push? I had a few link errors due to mismatched types (wchar_t vs. unsigned short).

Comment 70

[Jacek Caban]

Yes, it's quite likely, every file using wchar_t needs to be recompiled