public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools
@ 2019-06-21  1:58 Fan, ZhijuX
  2019-06-21  9:00 ` Leif Lindholm
  0 siblings, 1 reply; 4+ messages in thread
From: Fan, ZhijuX @ 2019-06-21  1:58 UTC (permalink / raw)
  To: devel@edk2.groups.io
  Cc: Gao, Liming, Feng, Bob C, Ard Biesheuvel, Leif Lindholm,
	Kinney, Michael D

[-- Attachment #1: Type: text/plain, Size: 9129 bytes --]

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1855

GenBiosId is a tool to generate the BIOS ID binary file which uses
the data from the configuration file.
This tool can be run in both Py2 and Py3.

This patch is going to add GenBiosId to Platform/Intel/Tools

Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
---
 Platform/Intel/Tools/GenBiosId/BiosId.env   |  27 +++++
 Platform/Intel/Tools/GenBiosId/GenBiosId.py | 180 ++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)
 create mode 100644 Platform/Intel/Tools/GenBiosId/BiosId.env
 create mode 100644 Platform/Intel/Tools/GenBiosId/GenBiosId.py

diff --git a/Platform/Intel/Tools/GenBiosId/BiosId.env b/Platform/Intel/Tools/GenBiosId/BiosId.env
new file mode 100644
index 0000000000..e1e913da76
--- /dev/null
+++ b/Platform/Intel/Tools/GenBiosId/BiosId.env
@@ -0,0 +1,27 @@
+## @file
+#  This file is used to define the BIOS ID parameters of the build.
+#  This file is processed by GenBiosId.
+#  Here, it is just a template and can be customized by user.
+#
+#  BIOS ID string format:
+#    $(BOARD_ID)$(BOARD_REV).$(BOARD_EXT).$(VERSION_MAJOR).$(BUILD_TYPE)$(VERSION_MINOR).YYMMDDHHMM
+#  All fields must have a fixed length. YYMMDDHHMM is UTC time.
+#    Example: "EMLATOR1.000.0001.D01.1906141517"
+#
+#  If DATE is specified for YYMMDD and TIME is specified for HHMM like below,
+#  GenBiosId will use the value of DATE and TIME to fill YYMMDDHHMM,
+#  otherwise GenBiosId will fill YYMMDDHHMM with current UTC time of the build machine.
+#    DATE          = 190614
+#    TIME          = 1517
+#
+#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+[config]
+BOARD_ID      = KBLRVP3
+BOARD_REV     = 1
+BOARD_EXT     = 000
+BUILD_TYPE    = D
+VERSION_MAJOR = 0001
+VERSION_MINOR = 01
diff --git a/Platform/Intel/Tools/GenBiosId/GenBiosId.py b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
new file mode 100644
index 0000000000..20fb7592b4
--- /dev/null
+++ b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
@@ -0,0 +1,180 @@
+## @file
+# Trim files preprocessed by compiler
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+##
+# Import Modules
+#
+import os
+import sys
+import time
+import logging
+import struct
+import datetime
+import argparse
+import platform
+
+try:
+    from configparser import ConfigParser
+except:
+    from ConfigParser import ConfigParser
+
+# Config message
+_BIOS_Signature = "$IBIOSI$"
+_SectionKeyName = '__name__'
+_SectionName = 'config'
+
+_ConfigItem = {
+    "BOARD_ID": {'Value': '', 'Length': 7},
+    "BOARD_REV": {'Value': '', 'Length': 1},
+    "BOARD_EXT": {'Value': '', 'Length': 3},
+    "BUILD_TYPE": {'Value': '', 'Length': 1},
+    "VERSION_MAJOR": {'Value': '0000', 'Length': 4},
+    "VERSION_MINOR": {'Value': '00', 'Length': 2},
+}
+
+# Version message
+__prog__ = 'GenBiosld'
+__description__ = 'Trim files preprocessed by compiler'
+__copyright__ = 'Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> '
+__version__ = '%s Version %s' % (__prog__, '0.1 ')
+
+# ExtraData message
+_Usage = "Usage: GenBiosId -i Configfile -o OutputFile [-ot OutputTextFile]"
+_ConfigSectionNotDefine = "Not support the config file format, need config section"
+_ErrorMessageTemplate = '\n\n%(tool)s...\n : error: %(msg)s\n\t%(extra)s'
+_ErrorLogger = logging.getLogger("tool_error")
+_ErrorFormatter = logging.Formatter("%(message)s")
+_ConfigLenInvalid = "Config item %s length is invalid"
+_ConfigItemInvalid = "Item %s is invalid"
+
+# Error message
+INFO = 20
+ERRORCODE = 50
+OPTION_MISSING = 'Missing option'
+FORMAT_INVALID = 'Invalid syntax/format'
+FILE_NOT_FOUND = 'File/directory not found in workspace'
+FORMAT_UNKNOWN_ERROR = 'Unknown error in syntax/format'
+FORMAT_NOT_SUPPORTED = 'Not supported syntax/format'
+
+
+def SetEdkLogger():
+    _ErrorLogger.setLevel(INFO)
+    _ErrorCh = logging.StreamHandler(sys.stderr)
+    _ErrorCh.setFormatter(_ErrorFormatter)
+    _ErrorLogger.addHandler(_ErrorCh)
+    return _ErrorLogger
+
+
+# Output the error message and exit the tool
+def EdkLogger(ToolName, Message, ExtraData):
+    _ErrorLogger = SetEdkLogger()
+    TemplateDict = {"tool": ToolName, "msg": Message, "extra": ExtraData}
+    LogText = _ErrorMessageTemplate % TemplateDict
+    _ErrorLogger.log(ERRORCODE, LogText)
+    sys.exit()
+
+
+# Open the file in the correct way
+def FileOpen(FileName, Mode, Buffer=-1):
+    def LongFilePath(FileName):
+        FileName = os.path.normpath(FileName)
+        if platform.system() == 'Windows':
+            if FileName.startswith('\\\\?\\'):
+                return FileName
+            if FileName.startswith('\\\\'):
+                return '\\\\?\\UNC\\' + FileName[2:]
+            if os.path.isabs(FileName):
+                return '\\\\?\\' + FileName
+        return FileName
+
+    return open(LongFilePath(FileName), Mode, Buffer)
+
+
+# Parse command line options
+def MyOptionParser():
+    parser = argparse.ArgumentParser(prog=__prog__,
+                                     description=__description__ + __copyright__ + _Usage,
+                                     conflict_handler='resolve')
+    parser.add_argument('-v', '--version', action='version', version=__version__,
+                        help="show program's version number and exit")
+    parser.add_argument('-i', '--int', metavar='FILENAME', dest='InputFile', help="Input Config file")
+    parser.add_argument('-o', '--out', metavar='FILENAME', dest='OutputFile', help="Output file")
+    parser.add_argument('-ot', '--text', metavar='FILENAME', dest='OutputTextFile', help="Output Text file")
+    Options = parser.parse_args()
+    return Options
+
+
+# Check the Tool for missing variables
+def CheckOptions(Options):
+    if len(sys.argv) != 5 and not (len(sys.argv) == 7 and Options.OutputTextFile):
+        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
+    elif not Options.InputFile or not Options.OutputFile:
+        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
+    InputFile = Options.InputFile
+    OutputFile = Options.OutputFile
+    OutputTextFile = Options.OutputTextFile
+    if not os.path.exists(InputFile):
+        EdkLogger("GenBiosId", FILE_NOT_FOUND, ExtraData="Input file not found")
+    return InputFile, OutputFile, OutputTextFile
+
+
+# Parse the input file and extract the information
+def ParserInputFile(InputFile):
+    cf = ConfigParser()
+    cf.optionxform = str
+    cf.read(InputFile)
+    if _SectionName not in cf._sections:
+        EdkLogger("GenBiosId", FORMAT_NOT_SUPPORTED, ExtraData=_ConfigSectionNotDefine)
+    for Item in cf._sections[_SectionName]:
+        if Item == _SectionKeyName:
+            continue
+        if Item not in _ConfigItem:
+            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigItemInvalid % Item)
+        _ConfigItem[Item]['Value'] = cf._sections[_SectionName][Item]
+        if len(_ConfigItem[Item]['Value']) != _ConfigItem[Item]['Length']:
+            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigLenInvalid % Item)
+    for Item in _ConfigItem:
+        if not _ConfigItem[Item]['Value']:
+            EdkLogger("GenBiosId", FORMAT_UNKNOWN_ERROR, ExtraData="Item %s is missing" % Item)
+    utcnow = datetime.datetime.utcnow()
+    TimeStamp = time.strftime("%y%m%d%H%M", utcnow.timetuple())
+
+    Id_Str = _ConfigItem['BOARD_ID']['Value'] + _ConfigItem['BOARD_REV']['Value'] + '.' + _ConfigItem['BOARD_EXT'][
+        'Value'] + '.' + _ConfigItem['VERSION_MAJOR']['Value'] + \
+             '.' + _ConfigItem["BUILD_TYPE"]['Value'] + _ConfigItem['VERSION_MINOR']['Value'] + '.' + TimeStamp
+    return Id_Str
+
+
+# Output information to a file
+def PrintOutputFile(OutputFile, OutputTextFile, Id_Str):
+    with FileOpen(OutputFile, 'wb') as FdOut:
+        for i in _BIOS_Signature:
+            FdOut.write(struct.pack('B', ord(i)))
+
+        for i in Id_Str:
+            FdOut.write(struct.pack('H', ord(i)))
+
+        FdOut.write(struct.pack('H', 0x00))
+    if OutputTextFile:
+        with FileOpen(OutputTextFile, 'w') as FdOut:
+            FdOut.write(Id_Str)
+
+
+# Tool entrance method
+def Main():
+    Options = MyOptionParser()
+    InputFile, OutputFile, OutputTextFile = CheckOptions(Options)
+    Id_Str = ParserInputFile(InputFile)
+    PrintOutputFile(OutputFile, OutputTextFile, Id_Str)
+    return 0
+
+
+if __name__ == '__main__':
+    r = Main()
+    ## 0-127 is a safe return range, and 1 is a standard default error
+    if r < 0 or r > 127: r = 1
+    sys.exit(r)
-- 
2.14.1.windows.1


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 7192 bytes --]

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools
  2019-06-21  1:58 [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools Fan, ZhijuX
@ 2019-06-21  9:00 ` Leif Lindholm
  2019-06-21  9:47   ` [edk2-devel] " Fan, ZhijuX
  0 siblings, 1 reply; 4+ messages in thread
From: Leif Lindholm @ 2019-06-21  9:00 UTC (permalink / raw)
  To: Fan, ZhijuX
  Cc: devel@edk2.groups.io, Gao, Liming, Feng, Bob C, Ard Biesheuvel,
	Kinney, Michael D

On Fri, Jun 21, 2019 at 01:58:17AM +0000, Fan, ZhijuX wrote:
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1855
> 
> GenBiosId is a tool to generate the BIOS ID binary file which uses
> the data from the configuration file.
> This tool can be run in both Py2 and Py3.
> 
> This patch is going to add GenBiosId to Platform/Intel/Tools
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  Platform/Intel/Tools/GenBiosId/BiosId.env   |  27 +++++
>  Platform/Intel/Tools/GenBiosId/GenBiosId.py | 180 ++++++++++++++++++++++++++++
>  2 files changed, 207 insertions(+)
>  create mode 100644 Platform/Intel/Tools/GenBiosId/BiosId.env
>  create mode 100644 Platform/Intel/Tools/GenBiosId/GenBiosId.py
> 
> diff --git a/Platform/Intel/Tools/GenBiosId/BiosId.env b/Platform/Intel/Tools/GenBiosId/BiosId.env
> new file mode 100644
> index 0000000000..e1e913da76
> --- /dev/null
> +++ b/Platform/Intel/Tools/GenBiosId/BiosId.env
> @@ -0,0 +1,27 @@
> +## @file
> +#  This file is used to define the BIOS ID parameters of the build.
> +#  This file is processed by GenBiosId.
> +#  Here, it is just a template and can be customized by user.
> +#
> +#  BIOS ID string format:
> +#    $(BOARD_ID)$(BOARD_REV).$(BOARD_EXT).$(VERSION_MAJOR).$(BUILD_TYPE)$(VERSION_MINOR).YYMMDDHHMM
> +#  All fields must have a fixed length. YYMMDDHHMM is UTC time.
> +#    Example: "EMLATOR1.000.0001.D01.1906141517"
> +#
> +#  If DATE is specified for YYMMDD and TIME is specified for HHMM like below,
> +#  GenBiosId will use the value of DATE and TIME to fill YYMMDDHHMM,
> +#  otherwise GenBiosId will fill YYMMDDHHMM with current UTC time of the build machine.
> +#    DATE          = 190614
> +#    TIME          = 1517
> +#
> +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +[config]
> +BOARD_ID      = KBLRVP3
> +BOARD_REV     = 1
> +BOARD_EXT     = 000
> +BUILD_TYPE    = D
> +VERSION_MAJOR = 0001
> +VERSION_MINOR = 01
> diff --git a/Platform/Intel/Tools/GenBiosId/GenBiosId.py b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> new file mode 100644
> index 0000000000..20fb7592b4
> --- /dev/null
> +++ b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> @@ -0,0 +1,180 @@
> +## @file
> +# Trim files preprocessed by compiler
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +##
> +# Import Modules
> +#
> +import os
> +import sys
> +import time
> +import logging
> +import struct
> +import datetime
> +import argparse
> +import platform
> +
> +try:
> +    from configparser import ConfigParser
> +except:
> +    from ConfigParser import ConfigParser
> +
> +# Config message
> +_BIOS_Signature = "$IBIOSI$"
> +_SectionKeyName = '__name__'
> +_SectionName = 'config'
> +
> +_ConfigItem = {
> +    "BOARD_ID": {'Value': '', 'Length': 7},
> +    "BOARD_REV": {'Value': '', 'Length': 1},
> +    "BOARD_EXT": {'Value': '', 'Length': 3},
> +    "BUILD_TYPE": {'Value': '', 'Length': 1},
> +    "VERSION_MAJOR": {'Value': '0000', 'Length': 4},
> +    "VERSION_MINOR": {'Value': '00', 'Length': 2},
> +}
> +
> +# Version message
> +__prog__ = 'GenBiosld'
> +__description__ = 'Trim files preprocessed by compiler'
> +__copyright__ = 'Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> '
> +__version__ = '%s Version %s' % (__prog__, '0.1 ')
> +
> +# ExtraData message
> +_Usage = "Usage: GenBiosId -i Configfile -o OutputFile [-ot OutputTextFile]"
> +_ConfigSectionNotDefine = "Not support the config file format, need config section"
> +_ErrorMessageTemplate = '\n\n%(tool)s...\n : error: %(msg)s\n\t%(extra)s'
> +_ErrorLogger = logging.getLogger("tool_error")
> +_ErrorFormatter = logging.Formatter("%(message)s")
> +_ConfigLenInvalid = "Config item %s length is invalid"
> +_ConfigItemInvalid = "Item %s is invalid"
> +
> +# Error message
> +INFO = 20
> +ERRORCODE = 50
> +OPTION_MISSING = 'Missing option'
> +FORMAT_INVALID = 'Invalid syntax/format'
> +FILE_NOT_FOUND = 'File/directory not found in workspace'
> +FORMAT_UNKNOWN_ERROR = 'Unknown error in syntax/format'
> +FORMAT_NOT_SUPPORTED = 'Not supported syntax/format'
> +
> +
> +def SetEdkLogger():
> +    _ErrorLogger.setLevel(INFO)
> +    _ErrorCh = logging.StreamHandler(sys.stderr)
> +    _ErrorCh.setFormatter(_ErrorFormatter)
> +    _ErrorLogger.addHandler(_ErrorCh)
> +    return _ErrorLogger
> +
> +
> +# Output the error message and exit the tool
> +def EdkLogger(ToolName, Message, ExtraData):
> +    _ErrorLogger = SetEdkLogger()
> +    TemplateDict = {"tool": ToolName, "msg": Message, "extra": ExtraData}
> +    LogText = _ErrorMessageTemplate % TemplateDict
> +    _ErrorLogger.log(ERRORCODE, LogText)
> +    sys.exit()

Should the above two functions be in a common module in edk2?

> +
> +
> +# Open the file in the correct way
> +def FileOpen(FileName, Mode, Buffer=-1):
> +    def LongFilePath(FileName):
> +        FileName = os.path.normpath(FileName)
> +        if platform.system() == 'Windows':
> +            if FileName.startswith('\\\\?\\'):
> +                return FileName
> +            if FileName.startswith('\\\\'):
> +                return '\\\\?\\UNC\\' + FileName[2:]
> +            if os.path.isabs(FileName):
> +                return '\\\\?\\' + FileName
> +        return FileName
> +
> +    return open(LongFilePath(FileName), Mode, Buffer)
> +
> +
> +# Parse command line options
> +def MyOptionParser():
> +    parser = argparse.ArgumentParser(prog=__prog__,
> +                                     description=__description__ + __copyright__ + _Usage,
> +                                     conflict_handler='resolve')
> +    parser.add_argument('-v', '--version', action='version', version=__version__,
> +                        help="show program's version number and exit")
> +    parser.add_argument('-i', '--int', metavar='FILENAME', dest='InputFile', help="Input Config file")
> +    parser.add_argument('-o', '--out', metavar='FILENAME', dest='OutputFile', help="Output file")
> +    parser.add_argument('-ot', '--text', metavar='FILENAME', dest='OutputTextFile', help="Output Text file")

'-ot' is long for a short argument. Could it just be '-t', which also
matches '--text' better?

> +    Options = parser.parse_args()
> +    return Options
> +
> +
> +# Check the Tool for missing variables
> +def CheckOptions(Options):
> +    if len(sys.argv) != 5 and not (len(sys.argv) == 7 and Options.OutputTextFile):
> +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> +    elif not Options.InputFile or not Options.OutputFile:
> +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)

Can we use argparse to validate the arguments instead - that's what
it's for?

> +    InputFile = Options.InputFile
> +    OutputFile = Options.OutputFile
> +    OutputTextFile = Options.OutputTextFile
> +    if not os.path.exists(InputFile):
> +        EdkLogger("GenBiosId", FILE_NOT_FOUND, ExtraData="Input file not found")

https://stackoverflow.com/questions/37471636/python-argument-parsing-validation-best-practices
gives an example on how to make argparse verify that files exist.

> +    return InputFile, OutputFile, OutputTextFile
> +
> +
> +# Parse the input file and extract the information
> +def ParserInputFile(InputFile):
> +    cf = ConfigParser()
> +    cf.optionxform = str
> +    cf.read(InputFile)
> +    if _SectionName not in cf._sections:
> +        EdkLogger("GenBiosId", FORMAT_NOT_SUPPORTED, ExtraData=_ConfigSectionNotDefine)
> +    for Item in cf._sections[_SectionName]:
> +        if Item == _SectionKeyName:
> +            continue
> +        if Item not in _ConfigItem:
> +            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigItemInvalid % Item)
> +        _ConfigItem[Item]['Value'] = cf._sections[_SectionName][Item]
> +        if len(_ConfigItem[Item]['Value']) != _ConfigItem[Item]['Length']:
> +            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigLenInvalid % Item)
> +    for Item in _ConfigItem:
> +        if not _ConfigItem[Item]['Value']:
> +            EdkLogger("GenBiosId", FORMAT_UNKNOWN_ERROR, ExtraData="Item %s is missing" % Item)
> +    utcnow = datetime.datetime.utcnow()
> +    TimeStamp = time.strftime("%y%m%d%H%M", utcnow.timetuple())
> +
> +    Id_Str = _ConfigItem['BOARD_ID']['Value'] + _ConfigItem['BOARD_REV']['Value'] + '.' + _ConfigItem['BOARD_EXT'][
> +        'Value'] + '.' + _ConfigItem['VERSION_MAJOR']['Value'] + \
> +             '.' + _ConfigItem["BUILD_TYPE"]['Value'] + _ConfigItem['VERSION_MINOR']['Value'] + '.' + TimeStamp
> +    return Id_Str
> +
> +
> +# Output information to a file
> +def PrintOutputFile(OutputFile, OutputTextFile, Id_Str):
> +    with FileOpen(OutputFile, 'wb') as FdOut:
> +        for i in _BIOS_Signature:
> +            FdOut.write(struct.pack('B', ord(i)))
> +
> +        for i in Id_Str:
> +            FdOut.write(struct.pack('H', ord(i)))
> +
> +        FdOut.write(struct.pack('H', 0x00))
> +    if OutputTextFile:
> +        with FileOpen(OutputTextFile, 'w') as FdOut:
> +            FdOut.write(Id_Str)
> +
> +
> +# Tool entrance method
> +def Main():
> +    Options = MyOptionParser()
> +    InputFile, OutputFile, OutputTextFile = CheckOptions(Options)
> +    Id_Str = ParserInputFile(InputFile)
> +    PrintOutputFile(OutputFile, OutputTextFile, Id_Str)
> +    return 0
> +
> +
> +if __name__ == '__main__':
> +    r = Main()
> +    ## 0-127 is a safe return range, and 1 is a standard default error
> +    if r < 0 or r > 127: r = 1

What is the point of this dance? Main always returns 0 (which does not
sound ideal).

> +    sys.exit(r)

Implications for asynchronous i/o?

/
    Leif

> -- 
> 2.14.1.windows.1
> 



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools
  2019-06-21  9:00 ` Leif Lindholm
@ 2019-06-21  9:47   ` Fan, ZhijuX
  2019-06-21 10:26     ` Leif Lindholm
  0 siblings, 1 reply; 4+ messages in thread
From: Fan, ZhijuX @ 2019-06-21  9:47 UTC (permalink / raw)
  To: devel@edk2.groups.io, leif.lindholm@linaro.org
  Cc: Gao, Liming, Feng, Bob C, Ard Biesheuvel, Kinney, Michael D

Hi:

Thank you for your comments. The output 1 was designed as a standard error.
Function EdkLogger ()       sys.exit(1)  "1" missing while writing the script




Any question, please let me know. Thanks.

Best Regards
Fan Zhiju



> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Friday, June 21, 2019 5:01 PM
> To: Fan, ZhijuX <zhijux.fan@intel.com>
> Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [edk2-platform patch 1/2] Platform/Intel:Add
> GenBiosId into edk2-platforms/Platform/Intel/Tools
> 
> On Fri, Jun 21, 2019 at 01:58:17AM +0000, Fan, ZhijuX wrote:
> > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1855
> >
> > GenBiosId is a tool to generate the BIOS ID binary file which uses the
> > data from the configuration file.
> > This tool can be run in both Py2 and Py3.
> >
> > This patch is going to add GenBiosId to Platform/Intel/Tools
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> > ---
> >  Platform/Intel/Tools/GenBiosId/BiosId.env   |  27 +++++
> >  Platform/Intel/Tools/GenBiosId/GenBiosId.py | 180
> > ++++++++++++++++++++++++++++
> >  2 files changed, 207 insertions(+)
> >  create mode 100644 Platform/Intel/Tools/GenBiosId/BiosId.env
> >  create mode 100644 Platform/Intel/Tools/GenBiosId/GenBiosId.py
> >
> > diff --git a/Platform/Intel/Tools/GenBiosId/BiosId.env
> > b/Platform/Intel/Tools/GenBiosId/BiosId.env
> > new file mode 100644
> > index 0000000000..e1e913da76
> > --- /dev/null
> > +++ b/Platform/Intel/Tools/GenBiosId/BiosId.env
> > @@ -0,0 +1,27 @@
> > +## @file
> > +#  This file is used to define the BIOS ID parameters of the build.
> > +#  This file is processed by GenBiosId.
> > +#  Here, it is just a template and can be customized by user.
> > +#
> > +#  BIOS ID string format:
> > +#
> $(BOARD_ID)$(BOARD_REV).$(BOARD_EXT).$(VERSION_MAJOR).$(BUILD_T
> YPE)$(VERSION_MINOR).YYMMDDHHMM
> > +#  All fields must have a fixed length. YYMMDDHHMM is UTC time.
> > +#    Example: "EMLATOR1.000.0001.D01.1906141517"
> > +#
> > +#  If DATE is specified for YYMMDD and TIME is specified for HHMM
> > +like below, #  GenBiosId will use the value of DATE and TIME to fill
> > +YYMMDDHHMM, #  otherwise GenBiosId will fill YYMMDDHHMM with
> current UTC time of the build machine.
> > +#    DATE          = 190614
> > +#    TIME          = 1517
> > +#
> > +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> #
> > +SPDX-License-Identifier: BSD-2-Clause-Patent # ## [config]
> > +BOARD_ID      = KBLRVP3
> > +BOARD_REV     = 1
> > +BOARD_EXT     = 000
> > +BUILD_TYPE    = D
> > +VERSION_MAJOR = 0001
> > +VERSION_MINOR = 01
> > diff --git a/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > new file mode 100644
> > index 0000000000..20fb7592b4
> > --- /dev/null
> > +++ b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > @@ -0,0 +1,180 @@
> > +## @file
> > +# Trim files preprocessed by compiler # # Copyright (c) 2019, Intel
> > +Corporation. All rights reserved.<BR> # SPDX-License-Identifier:
> > +BSD-2-Clause-Patent #
> > +
> > +##
> > +# Import Modules
> > +#
> > +import os
> > +import sys
> > +import time
> > +import logging
> > +import struct
> > +import datetime
> > +import argparse
> > +import platform
> > +
> > +try:
> > +    from configparser import ConfigParser
> > +except:
> > +    from ConfigParser import ConfigParser
> > +
> > +# Config message
> > +_BIOS_Signature = "$IBIOSI$"
> > +_SectionKeyName = '__name__'
> > +_SectionName = 'config'
> > +
> > +_ConfigItem = {
> > +    "BOARD_ID": {'Value': '', 'Length': 7},
> > +    "BOARD_REV": {'Value': '', 'Length': 1},
> > +    "BOARD_EXT": {'Value': '', 'Length': 3},
> > +    "BUILD_TYPE": {'Value': '', 'Length': 1},
> > +    "VERSION_MAJOR": {'Value': '0000', 'Length': 4},
> > +    "VERSION_MINOR": {'Value': '00', 'Length': 2}, }
> > +
> > +# Version message
> > +__prog__ = 'GenBiosld'
> > +__description__ = 'Trim files preprocessed by compiler'
> > +__copyright__ = 'Copyright (c) 2019, Intel Corporation. All rights
> reserved.<BR> '
> > +__version__ = '%s Version %s' % (__prog__, '0.1 ')
> > +
> > +# ExtraData message
> > +_Usage = "Usage: GenBiosId -i Configfile -o OutputFile [-ot
> OutputTextFile]"
> > +_ConfigSectionNotDefine = "Not support the config file format, need
> config section"
> > +_ErrorMessageTemplate = '\n\n%(tool)s...\n :
> error: %(msg)s\n\t%(extra)s'
> > +_ErrorLogger = logging.getLogger("tool_error") _ErrorFormatter =
> > +logging.Formatter("%(message)s") _ConfigLenInvalid = "Config item %s
> > +length is invalid"
> > +_ConfigItemInvalid = "Item %s is invalid"
> > +
> > +# Error message
> > +INFO = 20
> > +ERRORCODE = 50
> > +OPTION_MISSING = 'Missing option'
> > +FORMAT_INVALID = 'Invalid syntax/format'
> > +FILE_NOT_FOUND = 'File/directory not found in workspace'
> > +FORMAT_UNKNOWN_ERROR = 'Unknown error in syntax/format'
> > +FORMAT_NOT_SUPPORTED = 'Not supported syntax/format'
> > +
> > +
> > +def SetEdkLogger():
> > +    _ErrorLogger.setLevel(INFO)
> > +    _ErrorCh = logging.StreamHandler(sys.stderr)
> > +    _ErrorCh.setFormatter(_ErrorFormatter)
> > +    _ErrorLogger.addHandler(_ErrorCh)
> > +    return _ErrorLogger
> > +
> > +
> > +# Output the error message and exit the tool def EdkLogger(ToolName,
> > +Message, ExtraData):
> > +    _ErrorLogger = SetEdkLogger()
> > +    TemplateDict = {"tool": ToolName, "msg": Message, "extra": ExtraData}
> > +    LogText = _ErrorMessageTemplate % TemplateDict
> > +    _ErrorLogger.log(ERRORCODE, LogText)
> > +    sys.exit()
> 
> Should the above two functions be in a common module in edk2?
> 
> > +
> > +
> > +# Open the file in the correct way
> > +def FileOpen(FileName, Mode, Buffer=-1):
> > +    def LongFilePath(FileName):
> > +        FileName = os.path.normpath(FileName)
> > +        if platform.system() == 'Windows':
> > +            if FileName.startswith('\\\\?\\'):
> > +                return FileName
> > +            if FileName.startswith('\\\\'):
> > +                return '\\\\?\\UNC\\' + FileName[2:]
> > +            if os.path.isabs(FileName):
> > +                return '\\\\?\\' + FileName
> > +        return FileName
> > +
> > +    return open(LongFilePath(FileName), Mode, Buffer)
> > +
> > +
> > +# Parse command line options
> > +def MyOptionParser():
> > +    parser = argparse.ArgumentParser(prog=__prog__,
> > +                                     description=__description__ + __copyright__ + _Usage,
> > +                                     conflict_handler='resolve')
> > +    parser.add_argument('-v', '--version', action='version',
> version=__version__,
> > +                        help="show program's version number and exit")
> > +    parser.add_argument('-i', '--int', metavar='FILENAME', dest='InputFile',
> help="Input Config file")
> > +    parser.add_argument('-o', '--out', metavar='FILENAME',
> dest='OutputFile', help="Output file")
> > +    parser.add_argument('-ot', '--text', metavar='FILENAME',
> > +dest='OutputTextFile', help="Output Text file")
> 
> '-ot' is long for a short argument. Could it just be '-t', which also matches '--
> text' better?
> 
> > +    Options = parser.parse_args()
> > +    return Options
> > +
> > +
> > +# Check the Tool for missing variables def CheckOptions(Options):
> > +    if len(sys.argv) != 5 and not (len(sys.argv) == 7 and
> Options.OutputTextFile):
> > +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> > +    elif not Options.InputFile or not Options.OutputFile:
> > +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> 
> Can we use argparse to validate the arguments instead - that's what it's for?
> 
> > +    InputFile = Options.InputFile
> > +    OutputFile = Options.OutputFile
> > +    OutputTextFile = Options.OutputTextFile
> > +    if not os.path.exists(InputFile):
> > +        EdkLogger("GenBiosId", FILE_NOT_FOUND, ExtraData="Input file
> > + not found")
> 
> https://stackoverflow.com/questions/37471636/python-argument-parsing-
> validation-best-practices
> gives an example on how to make argparse verify that files exist.
> 
> > +    return InputFile, OutputFile, OutputTextFile
> > +
> > +
> > +# Parse the input file and extract the information def
> > +ParserInputFile(InputFile):
> > +    cf = ConfigParser()
> > +    cf.optionxform = str
> > +    cf.read(InputFile)
> > +    if _SectionName not in cf._sections:
> > +        EdkLogger("GenBiosId", FORMAT_NOT_SUPPORTED,
> ExtraData=_ConfigSectionNotDefine)
> > +    for Item in cf._sections[_SectionName]:
> > +        if Item == _SectionKeyName:
> > +            continue
> > +        if Item not in _ConfigItem:
> > +            EdkLogger("GenBiosId", FORMAT_INVALID,
> ExtraData=_ConfigItemInvalid % Item)
> > +        _ConfigItem[Item]['Value'] = cf._sections[_SectionName][Item]
> > +        if len(_ConfigItem[Item]['Value']) != _ConfigItem[Item]['Length']:
> > +            EdkLogger("GenBiosId", FORMAT_INVALID,
> ExtraData=_ConfigLenInvalid % Item)
> > +    for Item in _ConfigItem:
> > +        if not _ConfigItem[Item]['Value']:
> > +            EdkLogger("GenBiosId", FORMAT_UNKNOWN_ERROR,
> ExtraData="Item %s is missing" % Item)
> > +    utcnow = datetime.datetime.utcnow()
> > +    TimeStamp = time.strftime("%y%m%d%H%M", utcnow.timetuple())
> > +
> > +    Id_Str = _ConfigItem['BOARD_ID']['Value'] +
> _ConfigItem['BOARD_REV']['Value'] + '.' + _ConfigItem['BOARD_EXT'][
> > +        'Value'] + '.' + _ConfigItem['VERSION_MAJOR']['Value'] + \
> > +             '.' + _ConfigItem["BUILD_TYPE"]['Value'] +
> _ConfigItem['VERSION_MINOR']['Value'] + '.' + TimeStamp
> > +    return Id_Str
> > +
> > +
> > +# Output information to a file
> > +def PrintOutputFile(OutputFile, OutputTextFile, Id_Str):
> > +    with FileOpen(OutputFile, 'wb') as FdOut:
> > +        for i in _BIOS_Signature:
> > +            FdOut.write(struct.pack('B', ord(i)))
> > +
> > +        for i in Id_Str:
> > +            FdOut.write(struct.pack('H', ord(i)))
> > +
> > +        FdOut.write(struct.pack('H', 0x00))
> > +    if OutputTextFile:
> > +        with FileOpen(OutputTextFile, 'w') as FdOut:
> > +            FdOut.write(Id_Str)
> > +
> > +
> > +# Tool entrance method
> > +def Main():
> > +    Options = MyOptionParser()
> > +    InputFile, OutputFile, OutputTextFile = CheckOptions(Options)
> > +    Id_Str = ParserInputFile(InputFile)
> > +    PrintOutputFile(OutputFile, OutputTextFile, Id_Str)
> > +    return 0
> > +
> > +
> > +if __name__ == '__main__':
> > +    r = Main()
> > +    ## 0-127 is a safe return range, and 1 is a standard default error
> > +    if r < 0 or r > 127: r = 1
> 
> What is the point of this dance? Main always returns 0 (which does not sound
> ideal).
> 
> > +    sys.exit(r)
> 
> Implications for asynchronous i/o?
> 
> /
>     Leif
> 
> > --
> > 2.14.1.windows.1
> >
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools
  2019-06-21  9:47   ` [edk2-devel] " Fan, ZhijuX
@ 2019-06-21 10:26     ` Leif Lindholm
  0 siblings, 0 replies; 4+ messages in thread
From: Leif Lindholm @ 2019-06-21 10:26 UTC (permalink / raw)
  To: Fan, ZhijuX
  Cc: devel@edk2.groups.io, Gao, Liming, Feng, Bob C, Ard Biesheuvel,
	Kinney, Michael D

On Fri, Jun 21, 2019 at 09:47:37AM +0000, Fan, ZhijuX wrote:
> Hi:
> 
> Thank you for your comments. The output 1 was designed as a standard error.
> Function EdkLogger ()       sys.exit(1)  "1" missing while writing the script
> 
> 
> 
> 
> Any question, please let me know. Thanks.
> 
> Best Regards
> Fan Zhiju
> 
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Leif Lindholm
> > Sent: Friday, June 21, 2019 5:01 PM
> > To: Fan, ZhijuX <zhijux.fan@intel.com>
> > Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Feng, Bob C
> > <bob.c.feng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] [edk2-platform patch 1/2] Platform/Intel:Add
> > GenBiosId into edk2-platforms/Platform/Intel/Tools
> > 
> > On Fri, Jun 21, 2019 at 01:58:17AM +0000, Fan, ZhijuX wrote:
> > > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1855
> > >
> > > GenBiosId is a tool to generate the BIOS ID binary file which uses the
> > > data from the configuration file.
> > > This tool can be run in both Py2 and Py3.
> > >
> > > This patch is going to add GenBiosId to Platform/Intel/Tools
> > >
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> > > ---
> > >  Platform/Intel/Tools/GenBiosId/BiosId.env   |  27 +++++
> > >  Platform/Intel/Tools/GenBiosId/GenBiosId.py | 180
> > > ++++++++++++++++++++++++++++
> > >  2 files changed, 207 insertions(+)
> > >  create mode 100644 Platform/Intel/Tools/GenBiosId/BiosId.env
> > >  create mode 100644 Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > >
> > > diff --git a/Platform/Intel/Tools/GenBiosId/BiosId.env
> > > b/Platform/Intel/Tools/GenBiosId/BiosId.env
> > > new file mode 100644
> > > index 0000000000..e1e913da76
> > > --- /dev/null
> > > +++ b/Platform/Intel/Tools/GenBiosId/BiosId.env
> > > @@ -0,0 +1,27 @@
> > > +## @file
> > > +#  This file is used to define the BIOS ID parameters of the build.
> > > +#  This file is processed by GenBiosId.
> > > +#  Here, it is just a template and can be customized by user.
> > > +#
> > > +#  BIOS ID string format:
> > > +#
> > $(BOARD_ID)$(BOARD_REV).$(BOARD_EXT).$(VERSION_MAJOR).$(BUILD_T
> > YPE)$(VERSION_MINOR).YYMMDDHHMM
> > > +#  All fields must have a fixed length. YYMMDDHHMM is UTC time.
> > > +#    Example: "EMLATOR1.000.0001.D01.1906141517"
> > > +#
> > > +#  If DATE is specified for YYMMDD and TIME is specified for HHMM
> > > +like below, #  GenBiosId will use the value of DATE and TIME to fill
> > > +YYMMDDHHMM, #  otherwise GenBiosId will fill YYMMDDHHMM with
> > current UTC time of the build machine.
> > > +#    DATE          = 190614
> > > +#    TIME          = 1517
> > > +#
> > > +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> #
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent # ## [config]
> > > +BOARD_ID      = KBLRVP3
> > > +BOARD_REV     = 1
> > > +BOARD_EXT     = 000
> > > +BUILD_TYPE    = D
> > > +VERSION_MAJOR = 0001
> > > +VERSION_MINOR = 01
> > > diff --git a/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > > b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > > new file mode 100644
> > > index 0000000000..20fb7592b4
> > > --- /dev/null
> > > +++ b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> > > @@ -0,0 +1,180 @@
> > > +## @file
> > > +# Trim files preprocessed by compiler # # Copyright (c) 2019, Intel
> > > +Corporation. All rights reserved.<BR> # SPDX-License-Identifier:
> > > +BSD-2-Clause-Patent #
> > > +
> > > +##
> > > +# Import Modules
> > > +#
> > > +import os
> > > +import sys
> > > +import time
> > > +import logging
> > > +import struct
> > > +import datetime
> > > +import argparse
> > > +import platform
> > > +
> > > +try:
> > > +    from configparser import ConfigParser
> > > +except:
> > > +    from ConfigParser import ConfigParser
> > > +
> > > +# Config message
> > > +_BIOS_Signature = "$IBIOSI$"
> > > +_SectionKeyName = '__name__'
> > > +_SectionName = 'config'
> > > +
> > > +_ConfigItem = {
> > > +    "BOARD_ID": {'Value': '', 'Length': 7},
> > > +    "BOARD_REV": {'Value': '', 'Length': 1},
> > > +    "BOARD_EXT": {'Value': '', 'Length': 3},
> > > +    "BUILD_TYPE": {'Value': '', 'Length': 1},
> > > +    "VERSION_MAJOR": {'Value': '0000', 'Length': 4},
> > > +    "VERSION_MINOR": {'Value': '00', 'Length': 2}, }
> > > +
> > > +# Version message
> > > +__prog__ = 'GenBiosld'
> > > +__description__ = 'Trim files preprocessed by compiler'
> > > +__copyright__ = 'Copyright (c) 2019, Intel Corporation. All rights
> > reserved.<BR> '
> > > +__version__ = '%s Version %s' % (__prog__, '0.1 ')
> > > +
> > > +# ExtraData message
> > > +_Usage = "Usage: GenBiosId -i Configfile -o OutputFile [-ot
> > OutputTextFile]"
> > > +_ConfigSectionNotDefine = "Not support the config file format, need
> > config section"
> > > +_ErrorMessageTemplate = '\n\n%(tool)s...\n :
> > error: %(msg)s\n\t%(extra)s'
> > > +_ErrorLogger = logging.getLogger("tool_error") _ErrorFormatter =
> > > +logging.Formatter("%(message)s") _ConfigLenInvalid = "Config item %s
> > > +length is invalid"
> > > +_ConfigItemInvalid = "Item %s is invalid"
> > > +
> > > +# Error message
> > > +INFO = 20
> > > +ERRORCODE = 50
> > > +OPTION_MISSING = 'Missing option'
> > > +FORMAT_INVALID = 'Invalid syntax/format'
> > > +FILE_NOT_FOUND = 'File/directory not found in workspace'
> > > +FORMAT_UNKNOWN_ERROR = 'Unknown error in syntax/format'
> > > +FORMAT_NOT_SUPPORTED = 'Not supported syntax/format'
> > > +
> > > +
> > > +def SetEdkLogger():
> > > +    _ErrorLogger.setLevel(INFO)
> > > +    _ErrorCh = logging.StreamHandler(sys.stderr)
> > > +    _ErrorCh.setFormatter(_ErrorFormatter)
> > > +    _ErrorLogger.addHandler(_ErrorCh)
> > > +    return _ErrorLogger
> > > +
> > > +
> > > +# Output the error message and exit the tool def EdkLogger(ToolName,
> > > +Message, ExtraData):
> > > +    _ErrorLogger = SetEdkLogger()
> > > +    TemplateDict = {"tool": ToolName, "msg": Message, "extra": ExtraData}
> > > +    LogText = _ErrorMessageTemplate % TemplateDict
> > > +    _ErrorLogger.log(ERRORCODE, LogText)
> > > +    sys.exit()
> > 
> > Should the above two functions be in a common module in edk2?
> > 
> > > +
> > > +
> > > +# Open the file in the correct way
> > > +def FileOpen(FileName, Mode, Buffer=-1):
> > > +    def LongFilePath(FileName):
> > > +        FileName = os.path.normpath(FileName)
> > > +        if platform.system() == 'Windows':
> > > +            if FileName.startswith('\\\\?\\'):
> > > +                return FileName
> > > +            if FileName.startswith('\\\\'):
> > > +                return '\\\\?\\UNC\\' + FileName[2:]
> > > +            if os.path.isabs(FileName):
> > > +                return '\\\\?\\' + FileName
> > > +        return FileName
> > > +
> > > +    return open(LongFilePath(FileName), Mode, Buffer)
> > > +
> > > +
> > > +# Parse command line options
> > > +def MyOptionParser():
> > > +    parser = argparse.ArgumentParser(prog=__prog__,
> > > +                                     description=__description__ + __copyright__ + _Usage,
> > > +                                     conflict_handler='resolve')
> > > +    parser.add_argument('-v', '--version', action='version',
> > version=__version__,
> > > +                        help="show program's version number and exit")
> > > +    parser.add_argument('-i', '--int', metavar='FILENAME', dest='InputFile',
> > help="Input Config file")
> > > +    parser.add_argument('-o', '--out', metavar='FILENAME',
> > dest='OutputFile', help="Output file")
> > > +    parser.add_argument('-ot', '--text', metavar='FILENAME',
> > > +dest='OutputTextFile', help="Output Text file")
> > 
> > '-ot' is long for a short argument. Could it just be '-t', which also matches '--
> > text' better?
> > 
> > > +    Options = parser.parse_args()
> > > +    return Options
> > > +
> > > +
> > > +# Check the Tool for missing variables def CheckOptions(Options):
> > > +    if len(sys.argv) != 5 and not (len(sys.argv) == 7 and
> > Options.OutputTextFile):
> > > +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> > > +    elif not Options.InputFile or not Options.OutputFile:
> > > +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> > 
> > Can we use argparse to validate the arguments instead - that's what it's for?
> > 
> > > +    InputFile = Options.InputFile
> > > +    OutputFile = Options.OutputFile
> > > +    OutputTextFile = Options.OutputTextFile
> > > +    if not os.path.exists(InputFile):
> > > +        EdkLogger("GenBiosId", FILE_NOT_FOUND, ExtraData="Input file
> > > + not found")
> > 
> > https://stackoverflow.com/questions/37471636/python-argument-parsing-
> > validation-best-practices
> > gives an example on how to make argparse verify that files exist.
> > 
> > > +    return InputFile, OutputFile, OutputTextFile
> > > +
> > > +
> > > +# Parse the input file and extract the information def
> > > +ParserInputFile(InputFile):
> > > +    cf = ConfigParser()
> > > +    cf.optionxform = str
> > > +    cf.read(InputFile)
> > > +    if _SectionName not in cf._sections:
> > > +        EdkLogger("GenBiosId", FORMAT_NOT_SUPPORTED,
> > ExtraData=_ConfigSectionNotDefine)
> > > +    for Item in cf._sections[_SectionName]:
> > > +        if Item == _SectionKeyName:
> > > +            continue
> > > +        if Item not in _ConfigItem:
> > > +            EdkLogger("GenBiosId", FORMAT_INVALID,
> > ExtraData=_ConfigItemInvalid % Item)
> > > +        _ConfigItem[Item]['Value'] = cf._sections[_SectionName][Item]
> > > +        if len(_ConfigItem[Item]['Value']) != _ConfigItem[Item]['Length']:
> > > +            EdkLogger("GenBiosId", FORMAT_INVALID,
> > ExtraData=_ConfigLenInvalid % Item)
> > > +    for Item in _ConfigItem:
> > > +        if not _ConfigItem[Item]['Value']:
> > > +            EdkLogger("GenBiosId", FORMAT_UNKNOWN_ERROR,
> > ExtraData="Item %s is missing" % Item)
> > > +    utcnow = datetime.datetime.utcnow()
> > > +    TimeStamp = time.strftime("%y%m%d%H%M", utcnow.timetuple())
> > > +
> > > +    Id_Str = _ConfigItem['BOARD_ID']['Value'] +
> > _ConfigItem['BOARD_REV']['Value'] + '.' + _ConfigItem['BOARD_EXT'][
> > > +        'Value'] + '.' + _ConfigItem['VERSION_MAJOR']['Value'] + \
> > > +             '.' + _ConfigItem["BUILD_TYPE"]['Value'] +
> > _ConfigItem['VERSION_MINOR']['Value'] + '.' + TimeStamp
> > > +    return Id_Str
> > > +
> > > +
> > > +# Output information to a file
> > > +def PrintOutputFile(OutputFile, OutputTextFile, Id_Str):
> > > +    with FileOpen(OutputFile, 'wb') as FdOut:
> > > +        for i in _BIOS_Signature:
> > > +            FdOut.write(struct.pack('B', ord(i)))
> > > +
> > > +        for i in Id_Str:
> > > +            FdOut.write(struct.pack('H', ord(i)))
> > > +
> > > +        FdOut.write(struct.pack('H', 0x00))
> > > +    if OutputTextFile:
> > > +        with FileOpen(OutputTextFile, 'w') as FdOut:
> > > +            FdOut.write(Id_Str)
> > > +
> > > +
> > > +# Tool entrance method
> > > +def Main():
> > > +    Options = MyOptionParser()
> > > +    InputFile, OutputFile, OutputTextFile = CheckOptions(Options)
> > > +    Id_Str = ParserInputFile(InputFile)
> > > +    PrintOutputFile(OutputFile, OutputTextFile, Id_Str)
> > > +    return 0
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > +    r = Main()
> > > +    ## 0-127 is a safe return range, and 1 is a standard default error
> > > +    if r < 0 or r > 127: r = 1
> > 
> > What is the point of this dance? Main always returns 0 (which does not sound
> > ideal).
> > 
> > > +    sys.exit(r)
> > 
> > Implications for asynchronous i/o?
> > 
> > /
> >     Leif
> > 
> > > --
> > > 2.14.1.windows.1
> > >
> > 
> > 
> > 
> > 
> 

Can you please respond inline, next to the points being raised.

We are already communicating in what is not either of our first
language, about a non-human language. We do not need to increase the
difficulty level.

/
    Leif

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-06-21 10:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-21  1:58 [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools Fan, ZhijuX
2019-06-21  9:00 ` Leif Lindholm
2019-06-21  9:47   ` [edk2-devel] " Fan, ZhijuX
2019-06-21 10:26     ` Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox