From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.3143.1574683951310966722 for ; Mon, 25 Nov 2019 04:12:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JLM0iZjM; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574683950; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=c6eclxWf5miOWXgvZrm+7+zCT8//orEs2IqeGf1sHMU=; b=JLM0iZjMr7KzElbdhw6Ff9UnK79yuoe2fHRx9dCzrPsD+X2M5hWG1znOTgtzHZQv8QO9g8 27Y0DAVhvPLvqNXom0UsIoMr4DvkSYMgJgPnUe76IMP4M55PGS0M65YSfN72at/BhKVutS Dd8S7wHfILr/O+q7JHGiDSUxMyb3u3Y= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-244-BXnCPjByORiJRcsCjwqF_g-1; Mon, 25 Nov 2019 07:12:26 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB45010CE780; Mon, 25 Nov 2019 12:12:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-167.ams2.redhat.com [10.36.116.167]) by smtp.corp.redhat.com (Postfix) with ESMTP id 62A3960870; Mon, 25 Nov 2019 12:12:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] BaseTools:fixed Build failed issue for Non-English OS To: "Feng, Bob C" , "devel@edk2.groups.io" , "Fan, ZhijuX" Cc: "Gao, Liming" , Leif Lindholm References: <95a79c71-2eb4-af97-a738-170a2668599d@redhat.com> <08650203BA1BD64D8AD9B6D5D74A85D16156A292@SHSMSX104.ccr.corp.intel.com> <58388e00-7bcd-7a58-cc74-6a32d5e9589c@redhat.com> <08650203BA1BD64D8AD9B6D5D74A85D16156B346@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <1cc44f52-0c4b-8959-1122-4351d5e67b2d@redhat.com> Date: Mon, 25 Nov 2019 13:12:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <08650203BA1BD64D8AD9B6D5D74A85D16156B346@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: BXnCPjByORiJRcsCjwqF_g-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 ; devel@edk2.groups.io; Fan, ZhijuX > Cc: Gao, Liming ; Leif Lindholm > 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 >