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:[email protected]] > Sent: Saturday, November 23, 2019 12:47 AM > To: Feng, Bob C <[email protected]>; [email protected]; Fan, ZhijuX > <[email protected]> > Cc: Gao, Liming <[email protected]>; Leif Lindholm > <[email protected]> > 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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51232): https://edk2.groups.io/g/devel/message/51232 Mute This Topic: https://groups.io/mt/60828668/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
