On 7/30/19 4:10 AM, Feng, Bob C wrote: > Hi Phil, > > Thanks for your comments. I agree it will be easier to review if this patch > is split into multiple smaller ones. > Since the later patches in Multiple-process autogen patch series are based on > this patch. It will be bigger effort to recreate this whole patch series. So > I'd like to prefer not to change...
The tradeoff is which will have to do the biggest effort, and how many times, 1 for the commiter or N times for the N reviewers... > I'm willing to answer the questions for this patch. > > Thanks, > Bob > > -----Original Message----- > From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com] > Sent: Monday, July 29, 2019 11:03 PM > To: devel@edk2.groups.io; Feng, Bob C <bob.c.f...@intel.com> > Cc: Gao, Liming <liming....@intel.com> > Subject: Re: [edk2-devel] [Patch 02/11] BaseTools: Split > WorkspaceAutoGen._InitWorker into multiple functions > > 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....@intel.com> >> Signed-off-by: Bob Feng <bob.c.f...@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 This line is odd, it seems an incorrect copy/paste. Did you mean: PlatformPcds = Platform.Pcds Instead? >> # 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: >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44619): https://edk2.groups.io/g/devel/message/44619 Mute This Topic: https://groups.io/mt/32640228/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
