Re: [PATCH v1 17/27] BaseTools: DataType - cleanup list constants

["Carsey, Jaben" <jaben.carsey@intel.com>]

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:lersek@redhat.com]
> Sent: Friday, May 04, 2018 4:15 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@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.gao@intel.com>
> > Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jaben Carsey <jaben.carsey@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