[PATCH] BaseTools/Python: Add missing FatalError handling

[PATCH] BaseTools/Python: Add missing FatalError handling

[Name]

From: Irene Park <ipark@nvidia.com>

AutoGenWorker doesn't handle the exception from FatalError therefore
the build fails to return the proper error code at the exit.

Signed-off-by: Irene Park <ipark@nvidia.com>
---
 BaseTools/Source/Python/AutoGen/AutoGenWorker.py | 6 ++++++
 BaseTools/Source/Python/build/build.py           | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
index 563d91b..2395964 100755
--- a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
@@ -24,6 +24,7 @@ import traceback
 import sys
 from AutoGen.DataPipe import MemoryDataPipe
 import logging
+from Common.BuildToolError import FatalError
 
 def clearQ(q):
     try:
@@ -101,6 +102,7 @@ class AutoGenManager(threading.Thread):
         self.autogen_workers = autogen_workers
         self.feedback_q = feedback_q
         self.Status = True
+        self.Error = 0
         self.error_event = error_event
     def run(self):
         try:
@@ -113,6 +115,7 @@ class AutoGenManager(threading.Thread):
                     fin_num += 1
                 else:
                     self.Status = False
+                    self.Error = self.feedback_q.get()
                     self.TerminateWorkers()
                 if fin_num == len(self.autogen_workers):
                     self.clearQueue()
@@ -282,6 +285,9 @@ class AutoGenWorkerInProcess(mp.Process):
 
         except Empty:
             pass
+        except FatalError as e:
+            self.feedback_q.put(taskname)
+            self.feedback_q.put(e.args[0])
         except:
             self.feedback_q.put(taskname)
         finally:
diff --git a/BaseTools/Source/Python/build/build.py b/BaseTools/Source/Python/build/build.py
index ed3a3b9..d6e3d84 100755
--- a/BaseTools/Source/Python/build/build.py
+++ b/BaseTools/Source/Python/build/build.py
@@ -880,7 +880,10 @@ class Build():
 
             self.AutoGenMgr.join()
             rt = self.AutoGenMgr.Status
-            return rt, 0
+            err = 0
+            if not rt:
+                err = self.AutoGenMgr.Error
+            return rt, err
         except FatalError as e:
             return False, e.args[0]
         except:
-- 
2.7.4

Re: [PATCH] BaseTools/Python: Add missing FatalError handling

[Irene Park]

A gentle reminder and adding Bob Feng and Liming Gao to CC.
Thank you,
Irene

-----Original Message-----
From: Irene Park <ipark@nvidia.com> 
Sent: Wednesday, May 27, 2020 3:42 PM
To: devel@edk2.groups.io
Cc: Irene Park <ipark@nvidia.com>
Subject: [PATCH] BaseTools/Python: Add missing FatalError handling

From: Irene Park <ipark@nvidia.com>

AutoGenWorker doesn't handle the exception from FatalError therefore the build fails to return the proper error code at the exit.

Signed-off-by: Irene Park <ipark@nvidia.com>
---
 BaseTools/Source/Python/AutoGen/AutoGenWorker.py | 6 ++++++
 BaseTools/Source/Python/build/build.py           | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
index 563d91b..2395964 100755
--- a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
@@ -24,6 +24,7 @@ import traceback
 import sys
 from AutoGen.DataPipe import MemoryDataPipe  import logging
+from Common.BuildToolError import FatalError
 
 def clearQ(q):
     try:
@@ -101,6 +102,7 @@ class AutoGenManager(threading.Thread):
         self.autogen_workers = autogen_workers
         self.feedback_q = feedback_q
         self.Status = True
+        self.Error = 0
         self.error_event = error_event
     def run(self):
         try:
@@ -113,6 +115,7 @@ class AutoGenManager(threading.Thread):
                     fin_num += 1
                 else:
                     self.Status = False
+                    self.Error = self.feedback_q.get()
                     self.TerminateWorkers()
                 if fin_num == len(self.autogen_workers):
                     self.clearQueue()
@@ -282,6 +285,9 @@ class AutoGenWorkerInProcess(mp.Process):
 
         except Empty:
             pass
+        except FatalError as e:
+            self.feedback_q.put(taskname)
+            self.feedback_q.put(e.args[0])
         except:
             self.feedback_q.put(taskname)
         finally:
diff --git a/BaseTools/Source/Python/build/build.py b/BaseTools/Source/Python/build/build.py
index ed3a3b9..d6e3d84 100755
--- a/BaseTools/Source/Python/build/build.py
+++ b/BaseTools/Source/Python/build/build.py
@@ -880,7 +880,10 @@ class Build():
 
             self.AutoGenMgr.join()
             rt = self.AutoGenMgr.Status
-            return rt, 0
+            err = 0
+            if not rt:
+                err = self.AutoGenMgr.Error
+            return rt, err
         except FatalError as e:
             return False, e.args[0]
         except:
--
2.7.4

Re: [PATCH] BaseTools/Python: Add missing FatalError handling

[Bob Feng]

Hi Irene,

For this piece of code
+            self.feedback_q.put(taskname)
+            self.feedback_q.put(e.args[0])

I think there will be the case that another autogenworker process insert its taskname before the current autogenworker process do self.feedback_q.put(e.args[0]). If that case happens, the build tool will hang.

Thanks,
Bob

-----Original Message-----
From: Irene Park <ipark@nvidia.com> 
Sent: Friday, May 29, 2020 3:24 AM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH] BaseTools/Python: Add missing FatalError handling

A gentle reminder and adding Bob Feng and Liming Gao to CC.
Thank you,
Irene

-----Original Message-----
From: Irene Park <ipark@nvidia.com> 
Sent: Wednesday, May 27, 2020 3:42 PM
To: devel@edk2.groups.io
Cc: Irene Park <ipark@nvidia.com>
Subject: [PATCH] BaseTools/Python: Add missing FatalError handling

From: Irene Park <ipark@nvidia.com>

AutoGenWorker doesn't handle the exception from FatalError therefore the build fails to return the proper error code at the exit.

Signed-off-by: Irene Park <ipark@nvidia.com>
---
 BaseTools/Source/Python/AutoGen/AutoGenWorker.py | 6 ++++++
 BaseTools/Source/Python/build/build.py           | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
index 563d91b..2395964 100755
--- a/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGenWorker.py
@@ -24,6 +24,7 @@ import traceback
 import sys
 from AutoGen.DataPipe import MemoryDataPipe  import logging
+from Common.BuildToolError import FatalError
 
 def clearQ(q):
     try:
@@ -101,6 +102,7 @@ class AutoGenManager(threading.Thread):
         self.autogen_workers = autogen_workers
         self.feedback_q = feedback_q
         self.Status = True
+        self.Error = 0
         self.error_event = error_event
     def run(self):
         try:
@@ -113,6 +115,7 @@ class AutoGenManager(threading.Thread):
                     fin_num += 1
                 else:
                     self.Status = False
+                    self.Error = self.feedback_q.get()
                     self.TerminateWorkers()
                 if fin_num == len(self.autogen_workers):
                     self.clearQueue()
@@ -282,6 +285,9 @@ class AutoGenWorkerInProcess(mp.Process):
 
         except Empty:
             pass
+        except FatalError as e:
+            self.feedback_q.put(taskname)
+            self.feedback_q.put(e.args[0])
         except:
             self.feedback_q.put(taskname)
         finally:
diff --git a/BaseTools/Source/Python/build/build.py b/BaseTools/Source/Python/build/build.py
index ed3a3b9..d6e3d84 100755
--- a/BaseTools/Source/Python/build/build.py
+++ b/BaseTools/Source/Python/build/build.py
@@ -880,7 +880,10 @@ class Build():
 
             self.AutoGenMgr.join()
             rt = self.AutoGenMgr.Status
-            return rt, 0
+            err = 0
+            if not rt:
+                err = self.AutoGenMgr.Error
+            return rt, err
         except FatalError as e:
             return False, e.args[0]
         except:
--
2.7.4

Re: [edk2-devel] [PATCH] BaseTools/Python: Add missing FatalError handling

[Irene Park]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

Hi Bob,

Thanks for the review.
Here is the original description of the issue.
There has been a build failure from SCT, related to https://edk2.groups.io/g/devel/message/60407?p=,,,20,0,0,0::relevance,,gEfiFormBrowserExProtocolGuid,20,2,0,74529350. ( https://edk2.groups.io/g/devel/message/60407?p=,,,20,0,0,0::relevance,,gEfiFormBrowserExProtocolGuid,20,2,0,74529350 )
The build log printed the error code obviously but returned with sys.exit(0).
Unfortunately this behavior confused the build system and made me notice the failure so late.
If there is a concern of the conflict among multiple autogenworkers, I wonder how you think this way.

1. No change in BaseTools/Source/Python/AutoGen/AutoGenWorker.py
2. BaseTools/Source/Python/build/build.py

--- a/BaseTools/Source/Python/build/build.py

+++ b/BaseTools/Source/Python/build/build.py

@@ -880,7 +880,10 @@ class Build():

self.AutoGenMgr.join()

rt = self.AutoGenMgr.Status

-            return rt, 0

+            err = 0

+            if not rt:

+                err = UNKNOWN_ERROR << Use a fixed error type of FatalError instead of getting the exact error type from the autogenworkers.

+            return rt, err

except FatalError as e:

return False, e.args[0]

except:

In this way, no change in the multi-thread operation and build.py is able to exit with a standard default error. It's not needed to catch the exact error type.
Please share your opinion.

Thanks,
Irene

[-- Attachment #2: Type: text/html, Size: 2083 bytes --]