[PATCH] BaseTools:fixed Build failed issue for Non-English OS

[PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Fan, ZhijuX]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=2365

Build failed on Non-English OS if structurePcd is used in platform
dsc file.
When the output of some functions is converted to code, 
Because different OS Character encoding form differently,
there may be problems with some functions

The patch is going to fixed this issue

Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py b/BaseTools/Source/Python/Workspace/DscBuildData.py
index 9192077f90..901d95a413 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1752,7 +1752,7 @@ class DscBuildData(PlatformBuildClassObject):
         except:
             EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
         Result = Process.communicate()
-        return Process.returncode, Result[0].decode(), Result[1].decode()
+        return Process.returncode, Result[0].decode(errors='ignore'), Result[1].decode(errors='ignore')
 
     @staticmethod
     def IntToCString(Value, ValueSize):
-- 
2.14.1.windows.1


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 5554 bytes --]

Re: [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Bob Feng]

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: Fan, ZhijuX 
Sent: Wednesday, November 20, 2019 7:27 PM
To: devel@edk2.groups.io
Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
Subject: [PATCH] BaseTools:fixed Build failed issue for Non-English OS

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=2365

Build failed on Non-English OS if structurePcd is used in platform dsc file.
When the output of some functions is converted to code, Because different OS Character encoding form differently, there may be problems with some functions

The patch is going to fixed this issue

Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py b/BaseTools/Source/Python/Workspace/DscBuildData.py
index 9192077f90..901d95a413 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1752,7 +1752,7 @@ class DscBuildData(PlatformBuildClassObject):
         except:
             EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
         Result = Process.communicate()
-        return Process.returncode, Result[0].decode(), Result[1].decode()
+        return Process.returncode, Result[0].decode(errors='ignore'), 
+ Result[1].decode(errors='ignore')
 
     @staticmethod
     def IntToCString(Value, ValueSize):
--
2.14.1.windows.1

Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Laszlo Ersek]

(+Leif)

On 11/20/19 12:27, Fan, ZhijuX wrote:
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=2365
>
> Build failed on Non-English OS if structurePcd is used in platform
> dsc file.
> When the output of some functions is converted to code,
> Because different OS Character encoding form differently,
> there may be problems with some functions
>
> The patch is going to fixed this issue
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  BaseTools/Source/Python/Workspace/DscBuildData.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py b/BaseTools/Source/Python/Workspace/DscBuildData.py
> index 9192077f90..901d95a413 100644
> --- a/BaseTools/Source/Python/Workspace/DscBuildData.py
> +++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
> @@ -1752,7 +1752,7 @@ class DscBuildData(PlatformBuildClassObject):
>          except:
>              EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
>          Result = Process.communicate()
> -        return Process.returncode, Result[0].decode(), Result[1].decode()
> +        return Process.returncode, Result[0].decode(errors='ignore'), Result[1].decode(errors='ignore')
>
>      @staticmethod
>      def IntToCString(Value, ValueSize):

Let me quote the full method (pre-patch):


    @staticmethod
    def ExecuteCommand (Command):
        try:
            Process = subprocess.Popen(Command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
        except:
            EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
        Result = Process.communicate()
        return Process.returncode, Result[0].decode(), Result[1].decode()

Relevant documentation:

- https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
- https://docs.python.org/3/library/stdtypes.html#bytes.decode
- https://docs.python.org/3/library/codecs.html#error-handlers

So we launch a subprocess, read its stdout and stderr as byte arrays,
and then decode those byte arrays (interpreted in UTF-8) to string (=
sequences of unicode code points).

If the byte arrays turn out to be invalid (per UTF-8), we currently
fail.

With the patch, we don't fail, but return garbage (or I guess "partial
data" -- see the docs: "malformed data is ignored and encoding or
decoding is continued without further notice").

I think this patch is wrong. Here's why:

- I don't see why the symptom is inherently tied to structured PCDs.

- "Non-English OS" is not precise; it comes down to the encoding of the
  byte stream that the subprocess produces. Even if the current locale
  specifies English-language messages, the problem can surface if the
  subprocess prints *symbols*, with non-UTF-8 encoding. Conversely, if
  the locale specifies non-English messages, but the subprocess prints
  them in correct UTF-8 encoding, the problem will not arise.

- I think the code should attempt to decode the stdout / stderr byte
  arrays using the LC_CTYPE locale category (not fixed UTF-8), and if
  *even that* fails, we should use errors='replace' (or another
  replacement method).


... Finally, please compare commit 8ddec24dea74 ("BaseTools:Updata the
output encoding of the Popen function", 2019-08-01). At this point, I'm
completely confused. Consider:

(a) Commit 8ddec24dea74 removed the "encoding='utf-8'" parameter --
    which has no effect, as "encoding='utf-8'" is the default.

(b) It also removed "errors='ignore'". In effect, that change set
    'errors="strict"'.

(c) Okay... so the change in said commit actually means "be strict about
    UTF-8 decoding failures".  But then the *message* on commit
    8ddec24dea74, "Not all output works in utf-8, so change the encoding
    to the default", makes no sense at all!

(d) Given the above, the present patch, in effect, *reverts* the last
    hunk of commit 8ddec24dea74. The "encoding='utf-8'" parameter
    remains the default, and the patch basically says, "oh yea, you know
    what, I think I'd still like to ignore UTF-8 decoding errors, like I
    used to do before 8ddec24dea74".

In short... 8ddec24dea74 was wrong, at least in part, and now we're
reverting it (in part). The resultant code is still quite problematic in
my opinion (because we shouldn't *completely* ignore invalid UTF-8
sequences), but even if we do what the present patch proposes, can we at
least point out that we are *reverting* part of 8ddec24dea74? And drop
the "structure PCD" and "non-English OS" language?

Thanks
Laszlo

Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Bob Feng]

Hi Laszlo,

I add some comments inline.

Thanks,
Bob

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Friday, November 22, 2019 1:53 AM
To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

(+Leif)

On 11/20/19 12:27, Fan, ZhijuX wrote:
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=2365
>
> Build failed on Non-English OS if structurePcd is used in platform dsc 
> file.
> When the output of some functions is converted to code, Because 
> different OS Character encoding form differently, there may be 
> problems with some functions
>
> The patch is going to fixed this issue
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  BaseTools/Source/Python/Workspace/DscBuildData.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
> b/BaseTools/Source/Python/Workspace/DscBuildData.py
> index 9192077f90..901d95a413 100644
> --- a/BaseTools/Source/Python/Workspace/DscBuildData.py
> +++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
> @@ -1752,7 +1752,7 @@ class DscBuildData(PlatformBuildClassObject):
>          except:
>              EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
>          Result = Process.communicate()
> -        return Process.returncode, Result[0].decode(), Result[1].decode()
> +        return Process.returncode, Result[0].decode(errors='ignore'), 
> + Result[1].decode(errors='ignore')
>
>      @staticmethod
>      def IntToCString(Value, ValueSize):

Let me quote the full method (pre-patch):


    @staticmethod
    def ExecuteCommand (Command):
        try:
            Process = subprocess.Popen(Command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
        except:
            EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute command: %s' % Command)
        Result = Process.communicate()
        return Process.returncode, Result[0].decode(), Result[1].decode()

Relevant documentation:

- https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
- https://docs.python.org/3/library/stdtypes.html#bytes.decode
- https://docs.python.org/3/library/codecs.html#error-handlers

So we launch a subprocess, read its stdout and stderr as byte arrays, and then decode those byte arrays (interpreted in UTF-8) to string (= sequences of unicode code points).

If the byte arrays turn out to be invalid (per UTF-8), we currently fail.

With the patch, we don't fail, but return garbage (or I guess "partial data" -- see the docs: "malformed data is ignored and encoding or decoding is continued without further notice").

[Bob] That's correct. I agree.

I think this patch is wrong. Here's why:

- I don't see why the symptom is inherently tied to structured PCDs.

[Bob] The build failure was found on a platform which uses Structure Pcds in dsc and the build machine is a Win10 with Korean language.
The present patch descripts the build failure case.

- "Non-English OS" is not precise; it comes down to the encoding of the
  byte stream that the subprocess produces. Even if the current locale
  specifies English-language messages, the problem can surface if the
  subprocess prints *symbols*, with non-UTF-8 encoding. Conversely, if
  the locale specifies non-English messages, but the subprocess prints
  them in correct UTF-8 encoding, the problem will not arise.

[Bob] I agree. "Non-English OS" is not precise. For this case, the build failure is because the visual studio c compiler output message includes localized string. 

- I think the code should attempt to decode the stdout / stderr byte
  arrays using the LC_CTYPE locale category (not fixed UTF-8), and if
  *even that* fails, we should use errors='replace' (or another
  replacement method).

[Bob] I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough.

... Finally, please compare commit 8ddec24dea74 ("BaseTools:Updata the output encoding of the Popen function", 2019-08-01). At this point, I'm completely confused. Consider:

(a) Commit 8ddec24dea74 removed the "encoding='utf-8'" parameter --
    which has no effect, as "encoding='utf-8'" is the default.

(b) It also removed "errors='ignore'". In effect, that change set
    'errors="strict"'.

(c) Okay... so the change in said commit actually means "be strict about
    UTF-8 decoding failures".  But then the *message* on commit
    8ddec24dea74, "Not all output works in utf-8, so change the encoding
    to the default", makes no sense at all!

(d) Given the above, the present patch, in effect, *reverts* the last
    hunk of commit 8ddec24dea74. The "encoding='utf-8'" parameter
    remains the default, and the patch basically says, "oh yea, you know
    what, I think I'd still like to ignore UTF-8 decoding errors, like I
    used to do before 8ddec24dea74".

In short... 8ddec24dea74 was wrong, at least in part, and now we're reverting it (in part). The resultant code is still quite problematic in my opinion (because we shouldn't *completely* ignore invalid UTF-8 sequences), but even if we do what the present patch proposes, can we at least point out that we are *reverting* part of 8ddec24dea74? And drop the "structure PCD" and "non-English OS" language?

[Bob] I agree to mention the present patch is to revert part of commit 8ddec24dea74, drop "non-English OS" language, but keep "structure PCD".

Thanks
Laszlo

Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Laszlo Ersek]

Hi Bob,

On 11/22/19 07:39, Feng, Bob C wrote:

> [Bob] The build failure was found on a platform which uses Structure Pcds in dsc and the build machine is a Win10 with Korean language.
> The present patch descripts the build failure case.

> [Bob] I agree. "Non-English OS" is not precise. For this case, the build failure is because the visual studio c compiler output message includes localized string. 

> [Bob] I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough.

> [Bob] I agree to mention the present patch is to revert part of commit 8ddec24dea74, drop "non-English OS" language, but keep "structure PCD".

thanks for following up.

I'm happy if you agree to mention commit 8ddec24dea74.

Furthermore, I understand that Visual Studio can print localized
strings. But, there are two things that remain unclear:

- Why are such messages tied to structure PCDs? Can Visual Studio *not*
print the same kind of localized message in other cases? Like, what if
you have a (VOID*) PCD and assign an L"..." string to it, in the
platform DSC?

I mean I always encourage patch authors to include as many details about
the failure scenario in the commit message as they feel comfortable
about. But the current commit message suggests the issue is specific to
structure PCDs *only*. That's the confusing part.

- You mention "I think for this case build tool does not need to handle
the non-ascii characters so 'Ignore' will be enough." -- Sure, I guess
for this particular case, "Ignore" could be OK -- but the method that's
being modified bears the generic name "ExecuteCommand".

What *else* is ExecuteCommand() used for? I'm not convinced that
ignoring decoding errors in the standard output / standard error of the
subprocess is acceptable in *all* cases.

If there is no other use case for ExecuteCommand()'s standard output /
standard error than to print them to the edk2 build log, then I agree
"ignore" is tolerable. But I don't know if that's the only use case. If
it is, it should be stated in the commit message.

Thanks
Laszlo

Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Bob Feng]

Hi Laszlo,

ExecuteCommand() name is so generic but this function is only used for Structure PCD. We may change that function name to eliminate the confusion.

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Saturday, November 23, 2019 12:47 AM
To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
Cc: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

Hi Bob,

On 11/22/19 07:39, Feng, Bob C wrote:

> [Bob] The build failure was found on a platform which uses Structure Pcds in dsc and the build machine is a Win10 with Korean language.
> The present patch descripts the build failure case.

> [Bob] I agree. "Non-English OS" is not precise. For this case, the build failure is because the visual studio c compiler output message includes localized string. 

> [Bob] I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough.

> [Bob] I agree to mention the present patch is to revert part of commit 8ddec24dea74, drop "non-English OS" language, but keep "structure PCD".

thanks for following up.

I'm happy if you agree to mention commit 8ddec24dea74.

Furthermore, I understand that Visual Studio can print localized strings. But, there are two things that remain unclear:

- Why are such messages tied to structure PCDs? Can Visual Studio *not* print the same kind of localized message in other cases? Like, what if you have a (VOID*) PCD and assign an L"..." string to it, in the platform DSC?

I mean I always encourage patch authors to include as many details about the failure scenario in the commit message as they feel comfortable about. But the current commit message suggests the issue is specific to structure PCDs *only*. That's the confusing part.

- You mention "I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough." -- Sure, I guess for this particular case, "Ignore" could be OK -- but the method that's being modified bears the generic name "ExecuteCommand".

What *else* is ExecuteCommand() used for? I'm not convinced that ignoring decoding errors in the standard output / standard error of the subprocess is acceptable in *all* cases.

If there is no other use case for ExecuteCommand()'s standard output / standard error than to print them to the edk2 build log, then I agree "ignore" is tolerable. But I don't know if that's the only use case. If it is, it should be stated in the commit message.

Thanks
Laszlo

Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS

[Laszlo Ersek]

On 11/25/19 02:37, Feng, Bob C wrote:
> Hi Laszlo,
> 
> ExecuteCommand() name is so generic but this function is only used for Structure PCD.

Ah, thanks. I couldn't tell that from the source code.

> We may change that function name to eliminate the confusion.

Perhaps, or maybe clarify it in the commit message. (State that this
function is only invoked for structure PCDs.)

Thanks!
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Saturday, November 23, 2019 12:47 AM
> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS
> 
> Hi Bob,
> 
> On 11/22/19 07:39, Feng, Bob C wrote:
> 
>> [Bob] The build failure was found on a platform which uses Structure Pcds in dsc and the build machine is a Win10 with Korean language.
>> The present patch descripts the build failure case.
> 
>> [Bob] I agree. "Non-English OS" is not precise. For this case, the build failure is because the visual studio c compiler output message includes localized string. 
> 
>> [Bob] I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough.
> 
>> [Bob] I agree to mention the present patch is to revert part of commit 8ddec24dea74, drop "non-English OS" language, but keep "structure PCD".
> 
> thanks for following up.
> 
> I'm happy if you agree to mention commit 8ddec24dea74.
> 
> Furthermore, I understand that Visual Studio can print localized strings. But, there are two things that remain unclear:
> 
> - Why are such messages tied to structure PCDs? Can Visual Studio *not* print the same kind of localized message in other cases? Like, what if you have a (VOID*) PCD and assign an L"..." string to it, in the platform DSC?
> 
> I mean I always encourage patch authors to include as many details about the failure scenario in the commit message as they feel comfortable about. But the current commit message suggests the issue is specific to structure PCDs *only*. That's the confusing part.
> 
> - You mention "I think for this case build tool does not need to handle the non-ascii characters so 'Ignore' will be enough." -- Sure, I guess for this particular case, "Ignore" could be OK -- but the method that's being modified bears the generic name "ExecuteCommand".
> 
> What *else* is ExecuteCommand() used for? I'm not convinced that ignoring decoding errors in the standard output / standard error of the subprocess is acceptable in *all* cases.
> 
> If there is no other use case for ExecuteCommand()'s standard output / standard error than to print them to the edk2 build log, then I agree "ignore" is tolerable. But I don't know if that's the only use case. If it is, it should be stated in the commit message.
> 
> Thanks
> Laszlo
>