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

["Laszlo Ersek" <lersek@redhat.com>]

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