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

["Bob Feng" <bob.c.feng@intel.com>]

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