Laszlo, Wow. I thought I had tested, but clearly I missed that.
Do you think we just revert back the name change short term? I agree that mixing internal data structure names and names in the config file seems wrong, I don’t know the ROI for separation. Maybe use a dict to translate? -Jaben > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, May 04, 2018 4:15 AM > To: Carsey, Jaben <jaben.car...@intel.com> > Cc: edk2-devel@lists.01.org; Gao, Liming <liming....@intel.com> > Subject: Re: [edk2] [PATCH v1 17/27] BaseTools: DataType - cleanup list > constants > Importance: High > > Hi, > > On 04/20/18 17:51, Jaben Carsey wrote: > > remove unused ones > > convert lists used for membership testing to sets > > use shared ones not local ones > > > > Cc: Liming Gao <liming....@intel.com> > > Cc: Yonghong Zhu <yonghong....@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jaben Carsey <jaben.car...@intel.com> > > --- > > BaseTools/Source/Python/AutoGen/AutoGen.py | 32 ++++++++------ > --- > > BaseTools/Source/Python/AutoGen/GenC.py | 36 ++++++++-------- > ---- > > BaseTools/Source/Python/AutoGen/GenPcdDb.py | 4 +-- > > BaseTools/Source/Python/Common/DataType.py | 29 ++++++-------- > -- > > BaseTools/Source/Python/Ecc/Configuration.py | 2 +- > > BaseTools/Source/Python/Ecc/c.py | 2 +- > > BaseTools/Source/Python/GenFds/FdfParser.py | 2 +- > > BaseTools/Source/Python/GenFds/Ffs.py | 20 +---------- > > BaseTools/Source/Python/GenFds/OptRomInfStatement.py | 1 - > > BaseTools/Source/Python/Workspace/InfBuildData.py | 2 +- > > BaseTools/Source/Python/Workspace/MetaFileParser.py | 4 +-- > > BaseTools/Source/Python/build/build.py | 4 +-- > > 12 files changed, 53 insertions(+), 85 deletions(-) > > I think this patch (commit eece4292acc80) broke the Ecc tool: > > > diff --git a/BaseTools/Source/Python/Ecc/Configuration.py > b/BaseTools/Source/Python/Ecc/Configuration.py > > index b523858e1b1f..b5b583be8c4a 100644 > > --- a/BaseTools/Source/Python/Ecc/Configuration.py > > +++ b/BaseTools/Source/Python/Ecc/Configuration.py > > @@ -53,7 +53,7 @@ class Configuration(object): > > > > # List customized Modifer here, split with ',' > > # Defaultly use the definition in class DataType > > - self.ModifierList = MODIFIER_LIST > > + self.ModifierSet = MODIFIER_SET > > > > ## General Checking > > self.GeneralCheckAll = 0 > > When I run Ecc, it prints: > > BaseTools/Source/Python/Ecc/config.ini(44): error F004: Invalid > configuration option 'ModifierList' was found > > The error comes from "BaseTools/Source/Python/Ecc/Configuration.py": > > LineNo = 0 > for Line in open(Filepath, 'r'): > LineNo = LineNo + 1 > Line = CleanString(Line) > if Line != '': > List = GetSplitValueList(Line, TAB_EQUAL_SPLIT) > if List[0] not in self.__dict__: > ErrorMsg = "Invalid configuration option '%s' was found" > % List[0] > EdkLogger.error("Ecc", EdkLogger.ECC_ERROR, ErrorMsg, > File = > Filepath, Line = LineNo) > if List[0] == 'ModifierList': > List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) > if List[0] == 'MetaDataFileCheckPathOfGenerateFileList' and > List[1] > == "": > continue > if List[0] == 'SkipDirList': > List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) > if List[0] == 'SkipFileList': > List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) > if List[0] == 'BinaryExtList': > List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) > if List[0] == 'Copyright': > List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) > self.__dict__[List[0]] = List[1] > > Basically, the condition > > List[0] not in self.__dict__ > > expects that option names in the "config.ini" file match the attributes > / members of the Python object directly. Thus, if we rename a member, > such as from "ModifierList" to "ModifierSet", then all the config files > that used to contain "ModifierList" now have to rename their > "ModifierList" options to "ModifierSet" as well. > > This looks brittle to me; option names in config files are public, while > object member names are an implementation detail. They shouldn't be > coupled. > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
