[PATCH] BaseTools:Makefiles cause data errors on some platforms

[PATCH] BaseTools:Makefiles cause data errors on some platforms

[Fan, ZhijuX]

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

As the Dict is unordered, an error occurs when using the Dict
while creating a new item.So for now, use OrdereDict instead
of Dict.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
---
 BaseTools/Source/Python/AutoGen/GenMake.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py b/BaseTools/Source/Python/AutoGen/GenMake.py
index 7562dc68b3..426e6ca51a 100644
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -429,7 +429,7 @@ cleanlib:
         self.CommonFileDependency = []
         self.FileListMacros = {}
         self.ListFileMacros = {}
-        self.ObjTargetDict = {}
+        self.ObjTargetDict = OrderedDict()
         self.FileCache = {}
         self.LibraryBuildCommandList = []
         self.LibraryFileList = []
@@ -954,11 +954,9 @@ cleanlib:
                     self.ListFileMacros[T.IncListFileMacro] = []
                 if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
                     NewFile = self.PlaceMacro(str(T), self.Macros)
-                    if self.ObjTargetDict.get(T.Target.SubDir):
-                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
-                    else:
+                    if not self.ObjTargetDict.get(T.Target.SubDir):
                         self.ObjTargetDict[T.Target.SubDir] = set()
-                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
+                    self.ObjTargetDict[T.Target.SubDir].add(NewFile)
 
                 Deps = []
                 CCodeDeps = []
-- 
2.14.1.windows.1


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 3904 bytes --]

Re: [PATCH] BaseTools:Makefiles cause data errors on some platforms

[Bob Feng]

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: Fan, ZhijuX 
Sent: Wednesday, April 17, 2019 10:02 AM
To: devel@edk2.groups.io
Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
Subject: [PATCH] BaseTools:Makefiles cause data errors on some platforms

As the Dict is unordered, an error occurs when using the Dict while creating a new item.So for now, use OrdereDict instead of Dict.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
---
 BaseTools/Source/Python/AutoGen/GenMake.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py b/BaseTools/Source/Python/AutoGen/GenMake.py
index 7562dc68b3..426e6ca51a 100644
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -429,7 +429,7 @@ cleanlib:
         self.CommonFileDependency = []
         self.FileListMacros = {}
         self.ListFileMacros = {}
-        self.ObjTargetDict = {}
+        self.ObjTargetDict = OrderedDict()
         self.FileCache = {}
         self.LibraryBuildCommandList = []
         self.LibraryFileList = []
@@ -954,11 +954,9 @@ cleanlib:
                     self.ListFileMacros[T.IncListFileMacro] = []
                 if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
                     NewFile = self.PlaceMacro(str(T), self.Macros)
-                    if self.ObjTargetDict.get(T.Target.SubDir):
-                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
-                    else:
+                    if not self.ObjTargetDict.get(T.Target.SubDir):
                         self.ObjTargetDict[T.Target.SubDir] = set()
-                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
+                    self.ObjTargetDict[T.Target.SubDir].add(NewFile)
 
                 Deps = []
                 CCodeDeps = []
--
2.14.1.windows.1

Re: [edk2-devel] [PATCH] BaseTools:Makefiles cause data errors on some platforms

[Laszlo Ersek]

Hi,

On 04/17/19 04:02, Fan, ZhijuX wrote:
> As the Dict is unordered, an error occurs when using the Dict
> while creating a new item.So for now, use OrdereDict instead
> of Dict.

this problem statement is impenetrable.

- What is the exact symptom?

- What triggers it, and where?

- When we make the dict ordered, why does that mitigate the issue?

- Do we have a BZ for this?

The subject line of the patch is similarly unhelpful: "data errors on
some platforms". It's basically devoid of information.

--*--

Recently I've also seen a number of commits (with different code
changes) where the subject line was reused identically.... Yup:

     1  2914e8153dd3 BaseTools: Fix corner-cases of --hash feature
     2  1b8caf0d87ea BaseTools: Fix corner-cases of --hash feature
     3  f2b5e04acad2 BaseTools: Fix corner-cases of --hash feature
     4  db4d47fd3ae9 BaseTools: Fix corner-cases of --hash feature

What's really sad here is that the commit messages are otherwise pretty
helpful, it's just that the subject lines were chosen in an unfortunate way.

Bob: please enforce higher standards for commit messages under
BaseTools. People *care*. (I certainly do.) Good commit messages teach
details about the subsystem or module.

Thanks,
Laszlo

> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  BaseTools/Source/Python/AutoGen/GenMake.py | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py b/BaseTools/Source/Python/AutoGen/GenMake.py
> index 7562dc68b3..426e6ca51a 100644
> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
> @@ -429,7 +429,7 @@ cleanlib:
>          self.CommonFileDependency = []
>          self.FileListMacros = {}
>          self.ListFileMacros = {}
> -        self.ObjTargetDict = {}
> +        self.ObjTargetDict = OrderedDict()
>          self.FileCache = {}
>          self.LibraryBuildCommandList = []
>          self.LibraryFileList = []
> @@ -954,11 +954,9 @@ cleanlib:
>                      self.ListFileMacros[T.IncListFileMacro] = []
>                  if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
>                      NewFile = self.PlaceMacro(str(T), self.Macros)
> -                    if self.ObjTargetDict.get(T.Target.SubDir):
> -                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
> -                    else:
> +                    if not self.ObjTargetDict.get(T.Target.SubDir):
>                          self.ObjTargetDict[T.Target.SubDir] = set()
> -                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
> +                    self.ObjTargetDict[T.Target.SubDir].add(NewFile)
>  
>                  Deps = []
>                  CCodeDeps = []
> 

Re: [edk2-devel] [PATCH] BaseTools:Makefiles cause data errors on some platforms

[Bob Feng]

Hi Laszlo,

Yes. The commit message is very important. I'll care more on the commit message in future code review.
For this patch I'll ask submitter to enter a BZ and refine this patch commit message. 

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, April 17, 2019 9:23 PM
To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
Cc: Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH] BaseTools:Makefiles cause data errors on some platforms

Hi,

On 04/17/19 04:02, Fan, ZhijuX wrote:
> As the Dict is unordered, an error occurs when using the Dict while 
> creating a new item.So for now, use OrdereDict instead of Dict.

this problem statement is impenetrable.

- What is the exact symptom?

- What triggers it, and where?

- When we make the dict ordered, why does that mitigate the issue?

- Do we have a BZ for this?

The subject line of the patch is similarly unhelpful: "data errors on some platforms". It's basically devoid of information.

--*--

Recently I've also seen a number of commits (with different code
changes) where the subject line was reused identically.... Yup:

     1  2914e8153dd3 BaseTools: Fix corner-cases of --hash feature
     2  1b8caf0d87ea BaseTools: Fix corner-cases of --hash feature
     3  f2b5e04acad2 BaseTools: Fix corner-cases of --hash feature
     4  db4d47fd3ae9 BaseTools: Fix corner-cases of --hash feature

What's really sad here is that the commit messages are otherwise pretty helpful, it's just that the subject lines were chosen in an unfortunate way.

Bob: please enforce higher standards for commit messages under BaseTools. People *care*. (I certainly do.) Good commit messages teach details about the subsystem or module.

Thanks,
Laszlo

> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  BaseTools/Source/Python/AutoGen/GenMake.py | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
> b/BaseTools/Source/Python/AutoGen/GenMake.py
> index 7562dc68b3..426e6ca51a 100644
> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
> @@ -429,7 +429,7 @@ cleanlib:
>          self.CommonFileDependency = []
>          self.FileListMacros = {}
>          self.ListFileMacros = {}
> -        self.ObjTargetDict = {}
> +        self.ObjTargetDict = OrderedDict()
>          self.FileCache = {}
>          self.LibraryBuildCommandList = []
>          self.LibraryFileList = []
> @@ -954,11 +954,9 @@ cleanlib:
>                      self.ListFileMacros[T.IncListFileMacro] = []
>                  if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
>                      NewFile = self.PlaceMacro(str(T), self.Macros)
> -                    if self.ObjTargetDict.get(T.Target.SubDir):
> -                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
> -                    else:
> +                    if not self.ObjTargetDict.get(T.Target.SubDir):
>                          self.ObjTargetDict[T.Target.SubDir] = set()
> -                        self.ObjTargetDict[T.Target.SubDir].add(NewFile)
> +                    self.ObjTargetDict[T.Target.SubDir].add(NewFile)
>  
>                  Deps = []
>                  CCodeDeps = []
>