Re: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode locale Windows

["Park, Aiden" <aiden.park@intel.com>]

Hi Bob,

> -----Original Message-----
> From: Feng, Bob C <bob.c.feng@intel.com>
> Sent: Friday, December 20, 2019 12:19 AM
> To: Park, Aiden <aiden.park@intel.com>; devel@edk2.groups.io
> Subject: RE: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode locale
> Windows
> 
> Hi Aiden,
> 
> Thanks for clarifying this issue. Since I have no Unicode locales windows
> environment, I can't help to do the verification.
> For the following 2# message = stdout.decode(encoding='utf-8',
> errors='ignore').encode('utf-8'), after encode(), on python3, the message
> will not a string, it's bytes type data.
> 
> BTW, would you help check whether "message =
> stdout.decode(errors='ignore')" works?
Your recommendation works as expected. The issue is not reproducible anymore with this change.
I have also dry-run on https://github.com/tianocore/edk2/pull/255 and all checks passed.
Thanks for giving better recommendation.

> 
> Thanks,
> Bob
> 
> -----Original Message-----
> From: Park, Aiden
> Sent: Friday, December 20, 2019 3:03 PM
> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io
> Subject: RE: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode locale
> Windows
> 
> Hi Bob,
> 
> > -----Original Message-----
> > From: Feng, Bob C <bob.c.feng@intel.com>
> > Sent: Thursday, December 19, 2019 10:12 PM
> > To: Park, Aiden <aiden.park@intel.com>; devel@edk2.groups.io
> > Subject: RE: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode
> > locale Windows
> >
> > Hi Aiden,
> >
> > I'd like to know why need to call 'edksetup.bat forcerebuild' with
> > python subprocess.call().
> > Is there other way to execute edksetup.bat?
> >
> I forgot to mention one more thing in reproduce steps. This is also
> reproducible by just calling 'edksetup.bat forcerebuild' in Windows command
> prompt w/o python subprocess.call().
> I have tested on 'Chinese - Traditional, Taiwan' and 'Korean' locale Windows
> and it's reproducible on both Unicode locales. And below change works
> around the issue on both locales.
> 
> Some experiment. Not sure if this is python2 bug.
> message = "" <= type 'str'
> temp = stdout.decode(encoding='utf-8', errors='ignore') <= type 'unicode'
> which has unicode string message = temp.encode('utf-8') <= pass message =
> temp <= deadlock
> 
> > Thanks,
> > Bob
> > -----Original Message-----
> > From: Park, Aiden
> > Sent: Friday, December 20, 2019 8:23 AM
> > To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io
> > Subject: RE: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode
> > locale Windows
> >
> > Hi Bob,
> >
> > > -----Original Message-----
> > > From: Feng, Bob C <bob.c.feng@intel.com>
> > > Sent: Tuesday, December 17, 2019 9:23 PM
> > > To: devel@edk2.groups.io; Park, Aiden <aiden.park@intel.com>
> > > Cc: Feng, Bob C <bob.c.feng@intel.com>
> > > Subject: RE: [PATCH] [edk2/BaseTools] edksetup.bat stuck on unicode
> > > locale Windows
> > >
> > > Hi Aiden,
> > >
> > > If set kwargs["stdout"] = sys.stdout, then stdout and stderr
> > > messages from sub process will directly write to sys.stdout.
> > > I think this patch works because sys.stdout.encoding is the correct
> > encoding.
> > > So what do you think if we fix this Non-Ascii issue by Changing the
> > > line of message = stdout.decode(encoding='utf-8', errors='ignore')
> > > #for compatibility in python 2 and 3 to message =
> > > stdout.decode(encoding=sys.stdout.encoding , errors='ignore') #for
> > > compatibility in python 2 and 3
> > >
> > > Otherwise, I think this patch may need to take care of the code
> > > after this line kwargs["stdout"] = sys.stdout, I mean
> > > p.communicate() will return empty string after your change, and the
> > > following code would be useless.
> > >
> > Thanks for your comment. You are right. This change makes the
> > following code useless. It should use PIPE.
> >
> > I did double-check the environment to make sure reproduce steps, and
> > it turns out that it's reproducible on Unicode locale Windows + Python 2.
> > Python 3 does not have the issue.
> >
> > There seems to be 2 locations which make deadlock.
> > 1. subprocess.Popen()
> > It looks there is a race condition when creating a child process with PIPE.
> > Therefore, a lock is put for Popen().
> > +    popen_lock.acquire(True)
> >      p = subprocess.Popen(Args, cwd=WorkDir, stderr=kwargs["stderr"],
> > stdout=kwargs["stdout"])
> > +    popen_lock.release()
> >
> > 2. stdout.decode()
> > stdout variable is decoded to 'unicode' type message variable. But,
> > it's stuck if stdout has Unicode string even if errors='ignore'.
> > Converting The 'unicode' type message to 'str' type message resolves
> > the issue.
> > -        message = stdout.decode(encoding='utf-8', errors='ignore') #for
> > compatibility in python 2 and 3
> > +        message = stdout.decode(encoding='utf-8',
> > + errors='ignore').encode('utf-8') #for compatibility in python 2 and
> > + 3
> >
> > FYI, your recommendation encoding=sys.stdout.encoding does not help to
> > resolve this issue.
> >
> > I look forward to your feedback. Thanks.
> >
> > > Thanks,
> > > Bob
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > Of Park, Aiden
> > > Sent: Thursday, December 12, 2019 6:43 AM
> > > To: devel@edk2.groups.io
> > > Subject: [edk2-devel] [PATCH] [edk2/BaseTools] edksetup.bat stuck on
> > > unicode locale Windows
> > >
> > > This issue happens under two conditions.
> > >   1. Unicode language environment in Windows
> > >   2. Call 'edksetup.bat forcerebuild' with python subprocess.call()
> > >
> > > Steps to reproduce
> > >   C:\edk2>python
> > >   Python 2.7.12 (v2.7.12:d33e0cf91556, Jun 27 2016, 15:24:40)
> > >   Type help, copyright, credits or license for more information.
> > >   >>> import subprocess
> > >   >>> subprocess.call(['edksetup.bat', 'forcerebuild'])
> > >
> > >   The edksetup.bat stuck at 'nmake cleanall'.
> > >
> > > One of multi-threads is on deadlock when python handles stdout and
> > > stderr in a subprocess pipe only if the outputs include unicode chars.
> > > Only stderr will be handled in the pipe same as a single thread call.
> > >
> > > Reported in Slim Bootloader.
> > >   https://github.com/slimbootloader/slimbootloader/issues/478
> > > Local fix has been made in Slim Bootloader.
> > >   https://github.com/slimbootloader/slimbootloader/pull/490
> > >
> > > Signed-off-by: Aiden Park <aiden.park@intel.com>
> > > ---
> > >  BaseTools/Source/C/Makefiles/NmakeSubdirs.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/BaseTools/Source/C/Makefiles/NmakeSubdirs.py
> > > b/BaseTools/Source/C/Makefiles/NmakeSubdirs.py
> > > index 356f5ac..c77bfb0 100644
> > > --- a/BaseTools/Source/C/Makefiles/NmakeSubdirs.py
> > > +++ b/BaseTools/Source/C/Makefiles/NmakeSubdirs.py
> > > @@ -33,7 +33,7 @@ def RunCommand(WorkDir=None, *Args, **kwargs):
> > >      if "stderr" not in kwargs:
> > >          kwargs["stderr"] = subprocess.STDOUT
> > >      if "stdout" not in kwargs:
> > > -        kwargs["stdout"] = subprocess.PIPE
> > > +        kwargs["stdout"] = sys.stdout
> > >      p = subprocess.Popen(Args, cwd=WorkDir,
> > > stderr=kwargs["stderr"],
> > > stdout=kwargs["stdout"])
> > >      stdout, stderr = p.communicate()
> > >      message = ""
> > > --
> > > 2.10.2.windows.1
> > >
> > > 
> > >
> >
> > Best Regards,
> > Aiden
> >
> 
> Best Regards,
> Aiden
> 

Best Regards,
Aiden