From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web12.13676.1597236343577448253 for ; Wed, 12 Aug 2020 05:45:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=l7iJqDFH; spf=pass (domain: nuviainc.com, ip: 209.85.128.66, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f66.google.com with SMTP id k8so1813277wma.2 for ; Wed, 12 Aug 2020 05:45:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=eTdir48xCEz/67QYVod3n++mwCwg1haAJzzpzHA/yUA=; b=l7iJqDFHAKheSuTkFC+H+HIWHAcC3q/cJnwgkneJ4US+fucf4O1RY9WvpZvEAuXdvj WoSn8zkwI+TPf+0rFbcHgslUkD/JIL3VFk0jV4m+BBS2GOE+uruLxt6NtSZhfFkIfRl+ ldOn5JjJfxXk1HqXWvYI2Eav5EonfH9nr7bhPyx9CCyTTWpAfzV4kbB9EvBB37YGRpo3 QyW7drG1lCMHvkx9wpQ/RDrtZAq7DEyLiztW+J6/SMBtlpXEVLLBVlD7ioGjbDrw93J4 JroMw7xvQBBRQ7TCb70eZEHUOPV3rdzuojyY8eHEEdqsrSieqbTUNuScFKBfK8xNaNDo hBcg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=eTdir48xCEz/67QYVod3n++mwCwg1haAJzzpzHA/yUA=; b=FBBdLiGvr4uI2QN0jwDzkpkfwJ3gAT4yte387sJ56/SY5GZ2nxolxyHf0dqrMuzAUo TrT4XgvTI30jhP64cXtyCK6EvjG54wRE1KUJr6V8Lb9CoiVS3KBhypBoQimTSIMXyxfo qHJc5+xb/4IHyRVkJ4dvzfaaWWeJYhdwiiSp1apMSTdP1GdcF99xHkKLe13Md0ROia3+ Z4uhNJwy1nQYckLBnwohw9f8GYsT2GdrFFgotHd2Acbxge9yxH/SeJ02xM2MXCPD1EhN S1LU1AaEiM1plsbBXFYdqG8gOgRBMylRDpBIlp9QVahVamEZeaZZCefX0SF1Icg//qEB 8j6g== X-Gm-Message-State: AOAM530+nOg4+fY7s2WS8yvnJyvGkNA48zCzc3jyt3R3E/KWNbgHazwz A3mE17XY3T+/wHmuD4mgiChPKg== X-Google-Smtp-Source: ABdhPJwzvYf5zhwgHToU4MKAWPxeUJrX8OuewSIrOr7yUD2j3J9/fh46KIhwkLkHWcFyLs9c0msRhw== X-Received: by 2002:a1c:cc05:: with SMTP id h5mr7086301wmb.129.1597236342051; Wed, 12 Aug 2020 05:45:42 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id t17sm3389437wmj.34.2020.08.12.05.45.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Aug 2020 05:45:41 -0700 (PDT) Date: Wed, 12 Aug 2020 13:45:39 +0100 From: "Leif Lindholm" To: "Pankaj Bansal (OSS)" Cc: Meenakshi Aggarwal , "devel@edk2.groups.io" , Ard Biesheuvel Subject: Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print Message-ID: <20200812124539.GR23500@vanye> References: <20200708051933.8123-1-pankaj.bansal@oss.nxp.com> <20200708051933.8123-4-pankaj.bansal@oss.nxp.com> <20200805144441.GO31778@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 $ . /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