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

["Bob Feng" <bob.c.feng@intel.com>]

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 = []
>