1275916 - Fix ICU cross-compilation with mingw-w64

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Georg Koppen]

Attachment: esr45-mingw-w64-ICU-fix.patch

For Firefox 45 we needed to fix the ICU cross-compilation issues as the context menu on Windows was broken. We used the attached patch. This ticket is for upstreaming the Mozilla related parts. The ICU part is taken care of in https://ssl.icu-project.org/trac/ticket/12563.

I guess "upstreaming" actually means writing a new patch given bug 1239083 but we'll see.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 8756874 [details] [diff] [review]
esr45-mingw-w64-ICU-fix.patch

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

This shouldn't be needed at all anymore, and I'm not sure that's something we'd take on ESR (well, the icu.m4 change would clearly not make it to ESR as is). (Also, the patch contains changes to a file that doesn't exist in tree (I guess it exists in torbrowser's tree))

Comment 2

[Mike Hommey [:glandium]]

Sylvestre, is mingw patches something we'd take on ESR?

Comment 3

[Georg Koppen]

(In reply to Mike Hommey [:glandium] from comment #1)
> Comment on attachment 8756874 [details] [diff] [review]
> esr45-mingw-w64-ICU-fix.patch
> 
> Review of attachment 8756874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This shouldn't be needed at all anymore, and I'm not sure that's something
> we'd take on ESR (well, the icu.m4 change would clearly not make it to ESR
> as is). (Also, the patch contains changes to a file that doesn't exist in
> tree (I guess it exists in torbrowser's tree))

I attached the patch mainly for reference purposes in case others hit that problem too and are looking for a workaround (which is why I did not mark it as being up for review). I think there is no need to include it or a similar one into ESR45 (at least not from our point of view).

So, ICU cross-compilation with mingw-w64 should just work now? Interesting. Is that due to bug 1239083 as well? Anyway, I'll give it a try and close this bug as WORKSFORME if that succeeds.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Why not, build patches have usually only a tiny risk associated with them.

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to Georg Koppen from comment #3)
> So, ICU cross-compilation with mingw-w64 should just work now? Interesting.
> Is that due to bug 1239083 as well? Anyway, I'll give it a try and close
> this bug as WORKSFORME if that succeeds.

I don't know precisely, but those changes should have made cross-compilation easier, since we checked-in a copy of the ICU data file instead of requiring a host system build of ICU to generate it, and we're using the existing Mozilla build system to build the target ICU sources, so if everything else in Gecko builds ICU ought to build as well. It's possible I missed some ICU-specific build settings that would make this not work.

Can you confirm if Firefox 48 and later build ICU OK in this configuration? If so we can close this bug.

Comment 6

[Georg Koppen]

While investigating the problems we have with the current aurora in our cross-compile setup for Windows it seems to me ICU is not affected. Thus, we are fine in that regard. We might want to have a way to avoid using the ICU data file and generate that one during the actual build instead. But I am not sure about that yet. In either case that would be a different bug anyway.