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

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

[Park, Aiden]

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

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

[Bob Feng]

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,
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

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

[aiden.park]

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

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

[Bob Feng]

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?

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

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

[Park, Aiden]

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

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

[Bob Feng]

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?

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

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

[Park, Aiden]

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