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

RESOLVED FIXED in mozilla29

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla29
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
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...
let me try to launch that command and see what's the exact output
Nota: file incluso  C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\INCLUDE\stdio.h

kudos to the localizer who forgot the colon!
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.
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

4 years ago
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.
(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.
(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.
(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.
In addition, mozilla harness is not suffered from this bug at all. This bug is about non-Western developers in the first place.
(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>")
(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.
Created attachment 8362839 [details] [diff] [review]
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?
Attachment #8362839 - Flags: review?(mh+mozilla)
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.
Attachment #8362839 - Flags: review?(mh+mozilla) → feedback+
Created attachment 8362867 [details] [diff] [review]
cl.diff

Something like this? (as stated on irc I didn't use double space since it's barely distinguishable from a single one)
Attachment #8362839 - Attachment is obsolete: true
Attachment #8362867 - Flags: review?(mh+mozilla)
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.
Attachment #8362867 - Flags: review?(mh+mozilla) → review+
Created attachment 8362874 [details] [diff] [review]
patch for checkin

thank you, my build moved from almost 1 hour to a bit more than 17 minutes :)
Assignee: nobody → mak77
Attachment #8362867 - Attachment is obsolete: true
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/mozilla-inbound/rev/17220466f40b
Target Milestone: --- → mozilla29
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.
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.
Created attachment 8362923 [details] [diff] [review]
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
Attachment #8362874 - Attachment is obsolete: true
Attachment #8362923 - Flags: review?(mh+mozilla)
Created attachment 8362925 [details] [diff] [review]
patch v2.1

sigh, with proper comment update
Attachment #8362923 - Attachment is obsolete: true
Attachment #8362923 - Flags: review?(mh+mozilla)
Attachment #8362925 - Flags: review?(mh+mozilla)
Created attachment 8362926 [details] [diff] [review]
patch v2.2

both comments updated... sorry, I'm distracted today
Attachment #8362925 - Attachment is obsolete: true
Attachment #8362925 - Flags: review?(mh+mozilla)
Attachment #8362926 - Flags: review?(mh+mozilla)
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.
Attachment #8362926 - Flags: review?(mh+mozilla) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/a8c663915e02
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.