960458 - cl.py doesn't properly parse showIncludes prefix on localized Windows

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

cl.py tries to detect -showIncludes output using CL_INCLUDES_PREFIX and if it doesn't match it prints to the output.
CL_INCLUDES_PREFIX is localized and on my italian system it's "Nota: file incluso  C:". I'm not sure why it includes the disk pointer, though the code in cl.py doesn't seem to take that into account, so the startsWith always fails, since it's comparing Nota: file incluso  C:" to Nota: file incluso    C:\something..." (2 spaces in the first case, 4 spaces or more, depending on indentation, in the second one).
Even if it would properly compare them, it would add dependencies paths without the disk letter since it strips all of the prefix (not sure if that'd be an issue)

This causes a bunch of output spew that slowdowns the build crazily.

Comment 1

[Mike Hommey [:glandium]]

This is what is in configure.in:
CL_INCLUDES_PREFIX=`${CC} -showIncludes -c -Fonul dummy-hello.c 2>&1 | sed -ne 's/^\([^:]*:[^:]*:\).*stdio.h$/\1/p'`

So it's looking for "anything, colon, anything, colon, anything, stdio.h" and catches everything up to the second colon.

In other languages, there is a colon right before the file name, that isn't there in italian...

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

let me try to launch that command and see what's the exact output

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Nota: file incluso  C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\INCLUDE\stdio.h

kudos to the localizer who forgot the colon!

Comment 4

[Mike Hommey [:glandium]]

I guess the best thing to do is to start from the end and find the longest string that is a valid path, and keep the remainder as CL_INCLUDES_PREFIX. Better do that in python.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what about detecting the double space, instead of the final colon? would that work on asian translations?
Something like s/^\([^:]*:.*[ ]\{2\}\).*stdio.h$/\1/p'

Comment 6

[Gregory Szorc [:gps]]

It's issues like this that make me question whether we should just force everything to build in LANG=C or en-US. Is there enough translated output in the build for non-English speakers to derive a benefit? If we forced everything as English, we'd get more deterministic builds, fewer bugs like this, and parsing output (e.g. compiler warnings) would be much, much simpler. 

Today, the only translated output we'll likely encounter is from make and the C/C++ compiler and linker. Can non-English speakers expect to develop for Firefox much less most C/C++ code bases without a basic understanding of English? make and C++ compiler errors barely make sense to me in English!

Being a single language speaker, I concede I'm ignorant of how non-English speakers cope with the English dominance in programming. Someone please set me straight if I'm very wrong to question the utility of these translations.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Gregory Szorc [:gps] from comment #6)
> It's issues like this that make me question whether we should just force
> everything to build in LANG=C or en-US.

MSVC ignored LANG environment variable :(
But if it is possible, another befit is that Western developers would be able to read error messages when I pasted the build log on Bugzilla.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Gregory Szorc [:gps] from comment #6)
> It's issues like this that make me question whether we should just force
> everything to build in LANG=C or en-US. Is there enough translated output in
> the build for non-English speakers to derive a benefit? If we forced
> everything as English

Is this possible in a simple way though our harness, without asking anyone to manually fix their environment (since once you install ms stuff, you get the localized version by default).
I don't think anyone cares about developing with localized versions, there's really no benefit to it.  MS is famous for making the wrong calls about localizing technical stuff.

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Marco Bonardo [:mak] from comment #8)
> I don't think anyone cares about developing with localized versions,

I care. And I'm certain most Japanese developers will also care. MS command prompt really sucks.

Comment 10

[Masatoshi Kimura [:emk]]

In addition, mozilla harness is not suffered from this bug at all. This bug is about non-Western developers in the first place.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> I care. And I'm certain most Japanese developers will also care. MS command
> prompt really sucks.

I'm sorry if that was looking too much generic, I was only referring to localized output from compilers/linkers. Most of the output from our harness is english, regardless.

(In reply to Masatoshi Kimura [:emk] from comment #10)
> This bug is about non-Western developers in the first place.

And contributors, if their builds take more than one hour just due to bugs like this one, we are going to lose precious resources.
In this case the issue is with italian, bu I can't really tell if other locales are affected, it's possible, I don't have other localized versions of msvc.

Btw, Masatoshi-san, could you please check if the regex I suggest in comment 5 would work with japanese localized version? it would be:
cl -showIncludes -c -Fonul dummy-hello.c 2>&1 | sed -ne 's/^\([^:]*:.*[ ]\{2\}\).*stdio.h$/\1/p' (where dummy-hello.c is just "#include <stdio.h>")

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Marco Bonardo [:mak] from comment #11)
> Btw, Masatoshi-san, could you please check if the regex I suggest in comment
> 5 would work with japanese localized version? it would be:
> cl -showIncludes -c -Fonul dummy-hello.c 2>&1 | sed -ne 's/^\([^:]*:.*[
> ]\{2\}\).*stdio.h$/\1/p' (where dummy-hello.c is just "#include <stdio.h>")

"メモ: インクルード ファイル:" (Japanese localization of "Note: including file:") displayed. So it seems to work for me.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: cl.diff

Considered it seems to work properly on asian locales, would you accept this patch to solve the issue in the meanwhile you evaluate changes to how parsing of localized output works?

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8362839 [details] [diff] [review]
cl.diff

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

::: configure.in
@@ +7732,5 @@
>      _DEFINES_CXXFLAGS='$(ACDEFINES) -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT'
>    else
>      echo '#include <stdio.h>' > dummy-hello.c
>      changequote(,)
> +    CL_INCLUDES_PREFIX=`${CC} -showIncludes -c -Fonul dummy-hello.c 2>&1 | sed -ne 's/^\([^:]*:.*[ ]\{2\}\).*stdio.h$/\1/p'`

"[ ]\{2\}" can be written "  ".
Would you mind at least capturing stdio.h's path, and check if it works? If it doesn't, then abort configure with an error message saying we're sorry their compiler is using a locale with unsupported formatting.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: cl.diff

Something like this? (as stated on irc I didn't use double space since it's barely distinguishable from a single one)

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8362867 [details] [diff] [review]
cl.diff

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

::: configure.in
@@ +7742,5 @@
>          AC_MSG_ERROR([Cannot find cl -showIncludes prefix.])
>      fi
> +    if ! test -e "$_CL_STDIO_PATH"; then
> +        AC_MSG_ERROR([Unable to parse cl -showIncludes prefix. This compiler's locale has an unsupported formatting.])
> +    fi

Please put this test before the other. r+ with this changed.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch for checkin

thank you, my build moved from almost 1 hour to a bit more than 17 minutes :)

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17220466f40b

Comment 19

[Carsten Book [:Tomcat]]

had to backout this changes because of bustages on windows like https://tbpl.mozilla.org/php/getParsedLog.php?id=33321808&tree=Mozilla-Inbound

 configure: error: Unable to parse cl -showIncludes prefix. This compiler's locale has an unsupported formatting.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't have an EN license here to test, I think we may replace the first space with (either space or colon), I can test that on Try.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

use either colon or space for the first char.
The trybuild is still ongoing, though if this wouldn't work, it would have stopped by this time. https://tbpl.mozilla.org/?tree=Try&rev=0749bbe11db8

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.1

sigh, with proper comment update

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.2

both comments updated... sorry, I'm distracted today

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8362926 [details] [diff] [review]
patch v2.2

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

r+ assuming this doesn't break the tree again ;)

::: configure.in
@@ +7733,5 @@
>    else
>      echo '#include <stdio.h>' > dummy-hello.c
>      changequote(,)
> +    dnl This output is localized, split at the first double space or colon and space.
> +    _CL_PREFIX_REGEX="^\([^:]*:.*[ :]\ \)\(.*stdio.h\)$"

You can remove the backslash before the "second" space, now that there aren't two consecutive spaces.

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

without the backslash:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c663915e02

well, at least this time would be Try's fault... Looking at the tree.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8c663915e02