Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Jul 08, 2020 at 00:19:33 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> This patch adds the Support for printing the git commit information
> in linux build environment.
> 
> Ideal place of retrieving this information should be python script in
> BaseTools.
> 
> A Feature request for the same has been created:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2838
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dsc.inc                           |  3 ++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf   |  5 +++
>  Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf |  5 +++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c     | 17 +++++++++
>  Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c   | 17 +++++++++
>  Silicon/NXP/set_firmware_ver.sh                          | 36 ++++++++++++++++++++
>  6 files changed, 83 insertions(+)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
> index 06ee012c227a..a0762a6ef61d 100644
> --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> @@ -224,6 +224,9 @@ [PcdsDynamicHii.common.DEFAULT]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
>  
>  [PcdsFixedAtBuild.common]
> +  !ifdef $(FIRMWARE_VER)
> +    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"
> +  !endif
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> index f5dbd1349dc5..69f884af9e34 100644
> --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>  
>  [Packages]
>    ArmPkg/ArmPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Silicon/NXP/Chassis2/Chassis2.dec
>    Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
>    IoAccessLib
>    IoLib
>    PcdLib
> +  PrintLib
>    SerialPortLib
>  
>  [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>  
>  [FeaturePcd]
>    gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> index 75b68cc4ca2d..632acc52b20a 100644
> --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>  
>  [Packages]
>    ArmPkg/ArmPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Silicon/NXP/Chassis3V2/Chassis3V2.dec
>    Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
>    IoAccessLib
>    IoLib
>    PcdLib
> +  PrintLib
>    SerialPortLib
>  
>  [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>  
>  [FeaturePcd]
>    gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> index 91b19f832f00..bc782e7e3873 100644
> --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> @@ -12,6 +12,7 @@
>  #include <Library/IoAccessLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/SerialPortLib.h>
>  
>  /**
> @@ -89,10 +90,26 @@ ChassisInit (
>    VOID
>    )
>  {
> +  CHAR8                         Buffer[100];
> +  UINTN                         CharCount;
> +
>    //
>    // Early init serial Port to get board information.
>    //
>    SerialPortInitialize ();
>  
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer),
> +                "UEFI firmware built at %a on %a. version:\n\r",
> +                __TIME__, __DATE__
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);

Drop space before Buffer.

> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer), "%s\n\r",
> +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> +
>    SmmuInit ();
>  }
> diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> index 30f8f945b233..6d546f4754f9 100644
> --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> @@ -12,6 +12,7 @@
>  #include <Library/IoAccessLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/SerialPortLib.h>
>  
>  /**
> @@ -64,8 +65,24 @@ ChassisInit (
>    VOID
>    )
>  {
> +  CHAR8                         Buffer[100];
> +  UINTN                         CharCount;
> +
>    //
>    // Early init serial Port to get board information.
>    //
>    SerialPortInitialize ();
> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer),
> +                "UEFI firmware built at %a on %a. version:\n\r",
> +                __TIME__, __DATE__
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);

Drop space before Buffer.

I will note here that you are adding identical content to two
different ChassisLib implementations.

This is a strong indicator that the abstraction level is currently
incorrect and should be revisited.

> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer), "%s\n\r",
> +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
>  }
> diff --git a/Silicon/NXP/set_firmware_ver.sh b/Silicon/NXP/set_firmware_ver.sh

This script doesn't *set* anything (which is good, since I would have
rejected that outright), so the name should reflect the actual function.

> new file mode 100755
> index 000000000000..ba2336ad23dc
> --- /dev/null
> +++ b/Silicon/NXP/set_firmware_ver.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +#
> +# Copyright 2020 NXP
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +# parse PACKAGES_PATH and set FIRMWARE_VER based on that

This too should reflect actual function.

> +
> +get_git_version()
> +{
> +  command -v git>/dev/null 2>&1
> +  if [ $? -eq 0 ] && [ -n "$1" ]
> +  then
> +    head_or_tag=`git -C $1 describe --tag --dirty --broken --always 2>/dev/null`
> +    printf $head_or_tag
> +  fi
> +}
> +
> +if [ -n "$FIRMWARE_VER" ]; then
> +  echo "Warning FIRMWARE_VER=$FIRMWARE_VER is already set"
> +  echo "Please retry after 'unset FIRMWARE_VER' command"
> +fi

Just do
FIRMWARE_VER=
before the first code in the script.

> +
> +directories=$(echo $PACKAGES_PATH | tr ":" "\n")

Could set IFS=: instead, it would have the same effect.

> +for dir in $directories; do
> +  if [ -n "$FIRMWARE_VER" ]; then
> +    FIRMWARE_VER="$FIRMWARE_VER;$(basename $dir):$(get_git_version $dir)"
> +  else
> +    FIRMWARE_VER=$(basename $dir):$(get_git_version $dir)
> +  fi
> +done
> +
> +echo "FIRMWARE_VER=$FIRMWARE_VER"
> +export FIRMWARE_VER=$FIRMWARE_VER

What is this line supposed to achieve?

/
    Leif

> +
> +echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
> -- 
> 2.17.1
>