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

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

Hi Pankaj,

Apologies for slow response - having a bit of a heatwave and no AC...

On Sat, Aug 08, 2020 at 02:39:32 +0000, Pankaj Bansal (OSS) wrote:
> > 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.
> 
> I am preparing the patches to get the core/cluster info in NXP SOCs.
> I will move the version print in that code.

Works for me.

> > > +
> > > +  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.
> 
> it *does* set the FIRMWARE_VER environment variable.
> FIRMWARE_VER is just like WORKSPACE and PACKAGES_PATH.

1) OK, so the expectation is that this script should be sourced?
   This is not mentioned in the commit message, in comments in the
   script itself, and it does not warn you about that if you simply
   run it.
2) It's not like WORKSPACE or PACKAGES_PATH. Nothing in the EDK2
   BuildTools environment looks at FIRMWARE_VER.
3) It relies on PACKAGES_PATH being set in the environment.

So if I try to deduce the undocumented intended build steps, this
patch intends for the user to execute
$ . <edk2-platforms>/Silicon/NXP/set_firmware_ver.sh
which then prints for me that I should
"build edk2 firmware with -D FIRMWARE_VER=$FIRMWARE_VER"

This is not something that is of any use outside of your own build
infrastructure, so I would suggest you keep it downstream.

Having a generic (and portable) way of printing what you're currently
building isn't a bad idea however. If you were willing to create a
script for BaseTools based on roughly the below mechanics:

---

from __future__ import print_function
import os
import git

PACKAGES = os.environ['PACKAGES_PATH'].split(':')
# Trim empty entries caused by leading/trailing delimiter
try:
    while True:
        PACKAGES.remove('')
except ValueError:
    pass

for package in PACKAGES:
    REPO = git.Repo(path=package)
    HEAD = REPO.head
    SHA = REPO.git.rev_parse(HEAD, short=12)
    if REPO.is_dirty():
        STATE = 'dirty'
    else:
        STATE = 'clean'
    print('%s: %s-%s' % (os.path.basename(package), SHA, STATE))

---

I think that would be a lot more useful. If you were to add some
BinWrappers as well, you could then add in your build instructions to
use -D FIRMWARE_VER="`commandname`"

/
    Leif