Re: [edk2-devel] [Patch 02/11] BaseTools: Split WorkspaceAutoGen._InitWorker into multiple functions

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Bob,

On 7/29/19 10:44 AM, Bob Feng wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875
> 
> The WorkspaceAutoGen.__InitWorker function is too long, it's hard
> to read and understand.
> This patch is to separate the __InitWorker into multiple small ones.

Patch looks good, however not trivial to review, you are refactoring 1
big function into 13 (or 14) ones, it would be much simpler to review if
you use 1 patch per function extracted. If you mind and that is not too
much effort, that would be appreciated. If you prefer not, I'll review
your patch more carefully.

Thanks,

Phil.

> 
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Bob Feng <bob.c.feng@intel.com>
> ---
>  BaseTools/Source/Python/AutoGen/AutoGen.py | 247 +++++++++++++--------
>  1 file changed, 152 insertions(+), 95 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py b/BaseTools/Source/Python/AutoGen/AutoGen.py
> index c5b3fbb0a87f..9e06bb942126 100644
> --- a/BaseTools/Source/Python/AutoGen/AutoGen.py
> +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
> @@ -333,13 +333,58 @@ class WorkspaceAutoGen(AutoGen):
>          self._GuidDict = {}
>  
>          # there's many relative directory operations, so ...
>          os.chdir(self.WorkspaceDir)
>  
> +        self.MergeArch()
> +        self.ValidateBuildTarget()
> +
> +        EdkLogger.info("")
> +        if self.ArchList:
> +            EdkLogger.info('%-16s = %s' % ("Architecture(s)", ' '.join(self.ArchList)))
> +        EdkLogger.info('%-16s = %s' % ("Build target", self.BuildTarget))
> +        EdkLogger.info('%-16s = %s' % ("Toolchain", self.ToolChain))
> +
> +        EdkLogger.info('\n%-24s = %s' % ("Active Platform", self.Platform))
> +        if BuildModule:
> +            EdkLogger.info('%-24s = %s' % ("Active Module", BuildModule))
> +
> +        if self.FdfFile:
> +            EdkLogger.info('%-24s = %s' % ("Flash Image Definition", self.FdfFile))
> +
> +        EdkLogger.verbose("\nFLASH_DEFINITION = %s" % self.FdfFile)
> +
> +        if Progress:
> +            Progress.Start("\nProcessing meta-data")
>          #
> -        # Merge Arch
> +        # Mark now build in AutoGen Phase
>          #
> +        GlobalData.gAutoGenPhase = True
> +        self.ProcessModuleFromPdf()
> +        self.ProcessPcdType()
> +        self.ProcessMixedPcd()
> +        self.GetPcdsFromFDF()
> +        self.CollectAllPcds()
> +        self.GeneratePkgLevelHash()
> +        #
> +        # Check PCDs token value conflict in each DEC file.
> +        #
> +        self._CheckAllPcdsTokenValueConflict()
> +        #
> +        # Check PCD type and definition between DSC and DEC
> +        #
> +        self._CheckPcdDefineAndType()
> +
> +        self.CreateBuildOptionsFile()
> +        self.CreatePcdTokenNumberFile()
> +        self.CreateModuleHashInfo()
> +        GlobalData.gAutoGenPhase = False
> +
> +    #
> +    # Merge Arch
> +    #
> +    def MergeArch(self):
>          if not self.ArchList:
>              ArchList = set(self.Platform.SupArchList)
>          else:
>              ArchList = set(self.ArchList) & set(self.Platform.SupArchList)
>          if not ArchList:
> @@ -349,57 +394,49 @@ class WorkspaceAutoGen(AutoGen):
>              SkippedArchList = set(self.ArchList).symmetric_difference(set(self.Platform.SupArchList))
>              EdkLogger.verbose("\nArch [%s] is ignored because the platform supports [%s] only!"
>                                % (" ".join(SkippedArchList), " ".join(self.Platform.SupArchList)))
>          self.ArchList = tuple(ArchList)
>  
> -        # Validate build target
> +    # Validate build target
> +    def ValidateBuildTarget(self):
>          if self.BuildTarget not in self.Platform.BuildTargets:
>              EdkLogger.error("build", PARAMETER_INVALID,
>                              ExtraData="Build target [%s] is not supported by the platform. [Valid target: %s]"
>                                        % (self.BuildTarget, " ".join(self.Platform.BuildTargets)))
> -
> -
> -        # parse FDF file to get PCDs in it, if any
> +    @cached_property
> +    def FdfProfile(self):
>          if not self.FdfFile:
>              self.FdfFile = self.Platform.FlashDefinition
>  
> -        EdkLogger.info("")
> -        if self.ArchList:
> -            EdkLogger.info('%-16s = %s' % ("Architecture(s)", ' '.join(self.ArchList)))
> -        EdkLogger.info('%-16s = %s' % ("Build target", self.BuildTarget))
> -        EdkLogger.info('%-16s = %s' % ("Toolchain", self.ToolChain))
> -
> -        EdkLogger.info('\n%-24s = %s' % ("Active Platform", self.Platform))
> -        if BuildModule:
> -            EdkLogger.info('%-24s = %s' % ("Active Module", BuildModule))
> -
> +        FdfProfile = None
>          if self.FdfFile:
> -            EdkLogger.info('%-24s = %s' % ("Flash Image Definition", self.FdfFile))
> -
> -        EdkLogger.verbose("\nFLASH_DEFINITION = %s" % self.FdfFile)
> -
> -        if Progress:
> -            Progress.Start("\nProcessing meta-data")
> -
> -        if self.FdfFile:
> -            #
> -            # Mark now build in AutoGen Phase
> -            #
> -            GlobalData.gAutoGenPhase = True
>              Fdf = FdfParser(self.FdfFile.Path)
>              Fdf.ParseFile()
>              GlobalData.gFdfParser = Fdf
> -            GlobalData.gAutoGenPhase = False
> -            PcdSet = Fdf.Profile.PcdDict
>              if Fdf.CurrentFdName and Fdf.CurrentFdName in Fdf.Profile.FdDict:
>                  FdDict = Fdf.Profile.FdDict[Fdf.CurrentFdName]
>                  for FdRegion in FdDict.RegionList:
>                      if str(FdRegion.RegionType) is 'FILE' and self.Platform.VpdToolGuid in str(FdRegion.RegionDataList):
>                          if int(FdRegion.Offset) % 8 != 0:
>                              EdkLogger.error("build", FORMAT_INVALID, 'The VPD Base Address %s must be 8-byte aligned.' % (FdRegion.Offset))
> -            ModuleList = Fdf.Profile.InfList
> -            self.FdfProfile = Fdf.Profile
> +            FdfProfile = Fdf.Profile
> +        else:
> +            if self.FdTargetList:
> +                EdkLogger.info("No flash definition file found. FD [%s] will be ignored." % " ".join(self.FdTargetList))
> +                self.FdTargetList = []
> +            if self.FvTargetList:
> +                EdkLogger.info("No flash definition file found. FV [%s] will be ignored." % " ".join(self.FvTargetList))
> +                self.FvTargetList = []
> +            if self.CapTargetList:
> +                EdkLogger.info("No flash definition file found. Capsule [%s] will be ignored." % " ".join(self.CapTargetList))
> +                self.CapTargetList = []
> +
> +        return FdfProfile
> +
> +    def ProcessModuleFromPdf(self):
> +
> +        if self.FdfProfile:
>              for fvname in self.FvTargetList:
>                  if fvname.upper() not in self.FdfProfile.FvDict:
>                      EdkLogger.error("build", OPTION_VALUE_INVALID,
>                                      "No such an FV in FDF file: %s" % fvname)
>  
> @@ -407,64 +444,60 @@ class WorkspaceAutoGen(AutoGen):
>              # but the path (self.MetaFile.Path) is the real path
>              for key in self.FdfProfile.InfDict:
>                  if key == 'ArchTBD':
>                      MetaFile_cache = defaultdict(set)
>                      for Arch in self.ArchList:
> -                        Current_Platform_cache = self.BuildDatabase[self.MetaFile, Arch, Target, Toolchain]
> +                        Current_Platform_cache = self.BuildDatabase[self.MetaFile, Arch, self.BuildTarget, self.ToolChain]
>                          for Pkey in Current_Platform_cache.Modules:
>                              MetaFile_cache[Arch].add(Current_Platform_cache.Modules[Pkey].MetaFile)
>                      for Inf in self.FdfProfile.InfDict[key]:
>                          ModuleFile = PathClass(NormPath(Inf), GlobalData.gWorkspace, Arch)
>                          for Arch in self.ArchList:
>                              if ModuleFile in MetaFile_cache[Arch]:
>                                  break
>                          else:
> -                            ModuleData = self.BuildDatabase[ModuleFile, Arch, Target, Toolchain]
> +                            ModuleData = self.BuildDatabase[ModuleFile, Arch, self.BuildTarget, self.ToolChain]
>                              if not ModuleData.IsBinaryModule:
>                                  EdkLogger.error('build', PARSER_ERROR, "Module %s NOT found in DSC file; Is it really a binary module?" % ModuleFile)
>  
>                  else:
>                      for Arch in self.ArchList:
>                          if Arch == key:
> -                            Platform = self.BuildDatabase[self.MetaFile, Arch, Target, Toolchain]
> +                            Platform = self.BuildDatabase[self.MetaFile, Arch, self.BuildTarget, self.ToolChain]
>                              MetaFileList = set()
>                              for Pkey in Platform.Modules:
>                                  MetaFileList.add(Platform.Modules[Pkey].MetaFile)
>                              for Inf in self.FdfProfile.InfDict[key]:
>                                  ModuleFile = PathClass(NormPath(Inf), GlobalData.gWorkspace, Arch)
>                                  if ModuleFile in MetaFileList:
>                                      continue
> -                                ModuleData = self.BuildDatabase[ModuleFile, Arch, Target, Toolchain]
> +                                ModuleData = self.BuildDatabase[ModuleFile, Arch, self.BuildTarget, self.ToolChain]
>                                  if not ModuleData.IsBinaryModule:
>                                      EdkLogger.error('build', PARSER_ERROR, "Module %s NOT found in DSC file; Is it really a binary module?" % ModuleFile)
>  
> -        else:
> -            PcdSet = {}
> -            ModuleList = []
> -            self.FdfProfile = None
> -            if self.FdTargetList:
> -                EdkLogger.info("No flash definition file found. FD [%s] will be ignored." % " ".join(self.FdTargetList))
> -                self.FdTargetList = []
> -            if self.FvTargetList:
> -                EdkLogger.info("No flash definition file found. FV [%s] will be ignored." % " ".join(self.FvTargetList))
> -                self.FvTargetList = []
> -            if self.CapTargetList:
> -                EdkLogger.info("No flash definition file found. Capsule [%s] will be ignored." % " ".join(self.CapTargetList))
> -                self.CapTargetList = []
> -
> -        # apply SKU and inject PCDs from Flash Definition file
> +
> +
> +    # parse FDF file to get PCDs in it, if any
> +    def GetPcdsFromFDF(self):
> +
> +        if self.FdfProfile:
> +            PcdSet = self.FdfProfile.PcdDict
> +            # handle the mixed pcd in FDF file
> +            for key in PcdSet:
> +                if key in GlobalData.MixedPcd:
> +                    Value = PcdSet[key]
> +                    del PcdSet[key]
> +                    for item in GlobalData.MixedPcd[key]:
> +                        PcdSet[item] = Value
> +            self.VerifyPcdDeclearation(PcdSet)
> +
> +    def ProcessPcdType(self):
>          for Arch in self.ArchList:
> -            Platform = self.BuildDatabase[self.MetaFile, Arch, Target, Toolchain]
> -            PlatformPcds = Platform.Pcds
> -            self._GuidDict = Platform._GuidDict
> -            SourcePcdDict = {TAB_PCDS_DYNAMIC_EX:set(), TAB_PCDS_PATCHABLE_IN_MODULE:set(),TAB_PCDS_DYNAMIC:set(),TAB_PCDS_FIXED_AT_BUILD:set()}
> -            BinaryPcdDict = {TAB_PCDS_DYNAMIC_EX:set(), TAB_PCDS_PATCHABLE_IN_MODULE:set()}
> -            SourcePcdDict_Keys = SourcePcdDict.keys()
> -            BinaryPcdDict_Keys = BinaryPcdDict.keys()
> -
> +            Platform = self.BuildDatabase[self.MetaFile, Arch, self.BuildTarget, self.ToolChain]
> +            Platform.Pcds
>              # generate the SourcePcdDict and BinaryPcdDict
> -            PGen = PlatformAutoGen(self, self.MetaFile, Target, Toolchain, Arch)
> +            PGen = PlatformAutoGen(self, self.MetaFile, self.BuildTarget, self.ToolChain, Arch)
>              for BuildData in list(PGen.BuildDatabase._CACHE_.values()):
>                  if BuildData.Arch != Arch:
>                      continue
>                  if BuildData.MetaFile.Ext == '.inf':
>                      for key in BuildData.Pcds:
> @@ -483,11 +516,11 @@ class WorkspaceAutoGen(AutoGen):
>                                          BuildData.Pcds[key].Type = PcdInPlatform.Type
>                                          BuildData.Pcds[key].Pending = False
>                              else:
>                                  #Pcd used in Library, Pcd Type from reference module if Pcd Type is Pending
>                                  if BuildData.Pcds[key].Pending:
> -                                    MGen = ModuleAutoGen(self, BuildData.MetaFile, Target, Toolchain, Arch, self.MetaFile)
> +                                    MGen = ModuleAutoGen(self, BuildData.MetaFile, self.BuildTarget, self.ToolChain, Arch, self.MetaFile)
>                                      if MGen and MGen.IsLibrary:
>                                          if MGen in PGen.LibraryAutoGenList:
>                                              ReferenceModules = MGen.ReferenceModules
>                                              for ReferenceModule in ReferenceModules:
>                                                  if ReferenceModule.MetaFile in Platform.Modules:
> @@ -497,10 +530,24 @@ class WorkspaceAutoGen(AutoGen):
>                                                          if PcdInReferenceModule.Type:
>                                                              BuildData.Pcds[key].Type = PcdInReferenceModule.Type
>                                                              BuildData.Pcds[key].Pending = False
>                                                              break
>  
> +    def ProcessMixedPcd(self):
> +        for Arch in self.ArchList:
> +            SourcePcdDict = {TAB_PCDS_DYNAMIC_EX:set(), TAB_PCDS_PATCHABLE_IN_MODULE:set(),TAB_PCDS_DYNAMIC:set(),TAB_PCDS_FIXED_AT_BUILD:set()}
> +            BinaryPcdDict = {TAB_PCDS_DYNAMIC_EX:set(), TAB_PCDS_PATCHABLE_IN_MODULE:set()}
> +            SourcePcdDict_Keys = SourcePcdDict.keys()
> +            BinaryPcdDict_Keys = BinaryPcdDict.keys()
> +
> +            # generate the SourcePcdDict and BinaryPcdDict
> +            PGen = PlatformAutoGen(self, self.MetaFile, self.BuildTarget, self.ToolChain, Arch)
> +            for BuildData in list(PGen.BuildDatabase._CACHE_.values()):
> +                if BuildData.Arch != Arch:
> +                    continue
> +                if BuildData.MetaFile.Ext == '.inf':
> +                    for key in BuildData.Pcds:
>                          if TAB_PCDS_DYNAMIC_EX in BuildData.Pcds[key].Type:
>                              if BuildData.IsBinaryModule:
>                                  BinaryPcdDict[TAB_PCDS_DYNAMIC_EX].add((BuildData.Pcds[key].TokenCName, BuildData.Pcds[key].TokenSpaceGuidCName))
>                              else:
>                                  SourcePcdDict[TAB_PCDS_DYNAMIC_EX].add((BuildData.Pcds[key].TokenCName, BuildData.Pcds[key].TokenSpaceGuidCName))
> @@ -514,12 +561,11 @@ class WorkspaceAutoGen(AutoGen):
>  
>                          elif TAB_PCDS_DYNAMIC in BuildData.Pcds[key].Type:
>                              SourcePcdDict[TAB_PCDS_DYNAMIC].add((BuildData.Pcds[key].TokenCName, BuildData.Pcds[key].TokenSpaceGuidCName))
>                          elif TAB_PCDS_FIXED_AT_BUILD in BuildData.Pcds[key].Type:
>                              SourcePcdDict[TAB_PCDS_FIXED_AT_BUILD].add((BuildData.Pcds[key].TokenCName, BuildData.Pcds[key].TokenSpaceGuidCName))
> -                else:
> -                    pass
> +
>              #
>              # A PCD can only use one type for all source modules
>              #
>              for i in SourcePcdDict_Keys:
>                  for j in SourcePcdDict_Keys:
> @@ -588,27 +634,38 @@ class WorkspaceAutoGen(AutoGen):
>                                      del BuildData.Pcds[key]
>                                      BuildData.Pcds[newkey] = Value
>                                      break
>                              break
>  
> -            # handle the mixed pcd in FDF file
> -            for key in PcdSet:
> -                if key in GlobalData.MixedPcd:
> -                    Value = PcdSet[key]
> -                    del PcdSet[key]
> -                    for item in GlobalData.MixedPcd[key]:
> -                        PcdSet[item] = Value
> +    #Collect package set information from INF of FDF
> +    @cached_property
> +    def PkgSet(self):
> +        if not self.FdfFile:
> +            self.FdfFile = self.Platform.FlashDefinition
>  
> -            #Collect package set information from INF of FDF
> +        if self.FdfFile:
> +            ModuleList = self.FdfProfile.InfList
> +        else:
> +            ModuleList = []
> +        Pkgs = {}
> +        for Arch in self.ArchList:
> +            Platform = self.BuildDatabase[self.MetaFile, Arch, self.BuildTarget, self.ToolChain]
> +            PGen = PlatformAutoGen(self, self.MetaFile, self.BuildTarget, self.ToolChain, Arch)
>              PkgSet = set()
>              for Inf in ModuleList:
>                  ModuleFile = PathClass(NormPath(Inf), GlobalData.gWorkspace, Arch)
>                  if ModuleFile in Platform.Modules:
>                      continue
> -                ModuleData = self.BuildDatabase[ModuleFile, Arch, Target, Toolchain]
> +                ModuleData = self.BuildDatabase[ModuleFile, Arch, self.BuildTarget, self.ToolChain]
>                  PkgSet.update(ModuleData.Packages)
> -            Pkgs = list(PkgSet) + list(PGen.PackageList)
> +            Pkgs[Arch] = list(PkgSet) + list(PGen.PackageList)
> +        return Pkgs
> +
> +    def VerifyPcdDeclearation(self,PcdSet):
> +        for Arch in self.ArchList:
> +            Platform = self.BuildDatabase[self.MetaFile, Arch, self.BuildTarget, self.ToolChain]
> +            Pkgs = self.PkgSet[Arch]
>              DecPcds = set()
>              DecPcdsKey = set()
>              for Pkg in Pkgs:
>                  for Pcd in Pkg.Pcds:
>                      DecPcds.add((Pcd[0], Pcd[1]))
> @@ -636,37 +693,33 @@ class WorkspaceAutoGen(AutoGen):
>                                  PARSER_ERROR,
>                                  "Using Dynamic or DynamicEx type of PCD [%s.%s] in FDF file is not allowed." % (Guid, Name),
>                                  File = self.FdfProfile.PcdFileLineDict[Name, Guid, Fileds][0],
>                                  Line = self.FdfProfile.PcdFileLineDict[Name, Guid, Fileds][1]
>                          )
> +    def CollectAllPcds(self):
>  
> -            Pa = PlatformAutoGen(self, self.MetaFile, Target, Toolchain, Arch)
> +        for Arch in self.ArchList:
> +            Pa = PlatformAutoGen(self, self.MetaFile, self.BuildTarget, self.ToolChain, Arch)
>              #
>              # Explicitly collect platform's dynamic PCDs
>              #
>              Pa.CollectPlatformDynamicPcds()
>              Pa.CollectFixedAtBuildPcds()
>              self.AutoGenObjectList.append(Pa)
>  
> -            #
> -            # Generate Package level hash value
> -            #
> +    #
> +    # Generate Package level hash value
> +    #
> +    def GeneratePkgLevelHash(self):
> +        for Arch in self.ArchList:
>              GlobalData.gPackageHash = {}
>              if GlobalData.gUseHashCache:
> -                for Pkg in Pkgs:
> +                for Pkg in self.PkgSet[Arch]:
>                      self._GenPkgLevelHash(Pkg)
>  
> -        #
> -        # Check PCDs token value conflict in each DEC file.
> -        #
> -        self._CheckAllPcdsTokenValueConflict()
> -
> -        #
> -        # Check PCD type and definition between DSC and DEC
> -        #
> -        self._CheckPcdDefineAndType()
>  
> +    def CreateBuildOptionsFile(self):
>          #
>          # Create BuildOptions Macro & PCD metafile, also add the Active Platform and FDF file.
>          #
>          content = 'gCommandLineDefines: '
>          content += str(GlobalData.gCommandLineDefines)
> @@ -681,27 +734,31 @@ class WorkspaceAutoGen(AutoGen):
>              content += 'Flash Image Definition: '
>              content += str(self.FdfFile)
>              content += TAB_LINE_BREAK
>          SaveFileOnChange(os.path.join(self.BuildDir, 'BuildOptions'), content, False)
>  
> +    def CreatePcdTokenNumberFile(self):
>          #
>          # Create PcdToken Number file for Dynamic/DynamicEx Pcd.
>          #
>          PcdTokenNumber = 'PcdTokenNumber: '
> -        if Pa.PcdTokenNumber:
> -            if Pa.DynamicPcdList:
> -                for Pcd in Pa.DynamicPcdList:
> -                    PcdTokenNumber += TAB_LINE_BREAK
> -                    PcdTokenNumber += str((Pcd.TokenCName, Pcd.TokenSpaceGuidCName))
> -                    PcdTokenNumber += ' : '
> -                    PcdTokenNumber += str(Pa.PcdTokenNumber[Pcd.TokenCName, Pcd.TokenSpaceGuidCName])
> +        for Arch in self.ArchList:
> +            Pa = PlatformAutoGen(self, self.MetaFile, self.BuildTarget, self.ToolChain, Arch)
> +            if Pa.PcdTokenNumber:
> +                if Pa.DynamicPcdList:
> +                    for Pcd in Pa.DynamicPcdList:
> +                        PcdTokenNumber += TAB_LINE_BREAK
> +                        PcdTokenNumber += str((Pcd.TokenCName, Pcd.TokenSpaceGuidCName))
> +                        PcdTokenNumber += ' : '
> +                        PcdTokenNumber += str(Pa.PcdTokenNumber[Pcd.TokenCName, Pcd.TokenSpaceGuidCName])
>          SaveFileOnChange(os.path.join(self.BuildDir, 'PcdTokenNumber'), PcdTokenNumber, False)
>  
> +    def CreateModuleHashInfo(self):
>          #
>          # Get set of workspace metafiles
>          #
> -        AllWorkSpaceMetaFiles = self._GetMetaFiles(Target, Toolchain, Arch)
> +        AllWorkSpaceMetaFiles = self._GetMetaFiles(self.BuildTarget, self.ToolChain)
>  
>          #
>          # Retrieve latest modified time of all metafiles
>          #
>          SrcTimeStamp = 0
> @@ -759,11 +816,11 @@ class WorkspaceAutoGen(AutoGen):
>                          f.close()
>                          m.update(Content)
>          SaveFileOnChange(HashFile, m.hexdigest(), False)
>          GlobalData.gPackageHash[Pkg.PackageName] = m.hexdigest()
>  
> -    def _GetMetaFiles(self, Target, Toolchain, Arch):
> +    def _GetMetaFiles(self, Target, Toolchain):
>          AllWorkSpaceMetaFiles = set()
>          #
>          # add fdf
>          #
>          if self.FdfFile:
>