Re: [edk2] [PATCH] BuildEnv: remove useless check before setting $WORKSPACE

Subject: Re: [edk2] [PATCH] BuildEnv: remove useless check before setting $WORKSPACE

From: Laszlo Ersek <lersek@redhat.com>

To: Paolo Bonzini <pbonzini@redhat.com>, edk2-devel@lists.sourceforge.net, edk2-buildtools-devel@lists.sourceforge.net

Date: 2014-06-24 21:33:06

On 06/24/14 11:55, Paolo Bonzini wrote:
> As long as $EDK_TOOLS_PATH is properly set, the BaseTools/ directory
> is not necessary in the workspace.  The BuildEnv file itself suggests
> setting the variable if BaseTools/ is not available.
> 
> However, this only works if the user also sets $WORKSPACE.  Otherwise,
> BuildEnv refuses to set WORKSPACE itself and does not even try to use
> the preset $EDK_TOOLS_PATH.  Remove the check that fails, as it does
> not have any practical benefit.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 
> 
> --- a/BaseTools/BuildEnv
> +++ b/BaseTools/BuildEnv
> @@ -23,14 +23,6 @@
>      return 0
>    fi
>  
> -  if [ ! ${BASH_SOURCE[0]} -ef ./BaseTools/BuildEnv ]
> -  then
> -    echo Run this script from the base of your tree.  For example:
> -    echo "  cd /Path/To/Edk/Root"
> -    echo "  . BaseTools/BuildEnv"
> -    return 1
> -  fi
> -
>    #
>    # Set $WORKSPACE
>    #
> 

The calls go like

ScriptMain
  SetWorkspace
  SetEdkToolsPath

"SetEdkToolsPath" prints the message you refer to (as last resort):

--------
Unable to determine EDK_TOOLS_PATH

You may need to download the 'BaseTools' from buildtools.tianocore.org.
After downloading, either create a symbolic link to the source at
$WORKSPACE/Conf/BaseToolsSource, or set the EDK_TOOLS_PATH environment
variable.
--------

The code being removed checks if "./BaseTools/BuildEnv" is the same
script file (by device and inode numbers) as the one executing (whether
forked or sourced). This seems to be the polar opposite of what
EDK_TOOLS_PATH is supposed to enable -- an out-of-WORKSPACE (== $PWD)
BaseTools subdir. I checked the commit log but still can't figure out
why this check was added in the first place.

If the SetWorkspace function only wants to ensure that it is called from
the root of an edk2 clone (ie. a WORKSPACE candidate), then it should
check eg. for the existence of "./MdePkg/MdePkg.dec", rather than
depending on an embedded BaseTools (which defeats EDK_TOOLS_PATH).

If that's the case, then I think the following patch would be preferable:

--------
diff --git a/BaseTools/BuildEnv b/BaseTools/BuildEnv
index cccc753..d85c967 100755
--- a/BaseTools/BuildEnv
+++ b/BaseTools/BuildEnv
@@ -23,7 +23,7 @@ SetWorkspace() {
     return 0
   fi

-  if [ ! ${BASH_SOURCE[0]} -ef ./BaseTools/BuildEnv ]
+  if [ ! -e MdePkg/MdePkg.dec ]
   then
     echo Run this script from the base of your tree.  For example:
     echo "  cd /Path/To/Edk/Root"
--------

Ie. the current code does have one practical benefit -- a basic sanity
check that you are standing in an edk2 root dir -- but the
implementation incurs an unnecessary dependency on BaseTools. Let's keep
the intent, just improve the implementation.

What do others think?

Thanks,
Laszlo

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel