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

[Laszlo Ersek <lersek@redhat.com>]

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