Re: [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR

[Darren Hart <dvhart@linux.intel.com>]

On 08/09/2011 07:04 AM, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-09:
>> On 08/08/2011 07:13 PM, Cui, Dexuan wrote:
>>> From 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Mon Sep 17 00:00:00 2001
>>> From: Dexuan Cui <dexuan.cui@intel.com> Date: Thu, 04 Aug 2011 06:53:20
>>> +0000 Subject: scripts/oe-buildenv-internal: improve the error
>>> detecting for $BDIR
>>>
>>> Thanks a lot to Darren Hart and Paul Eggleton's suggestions!
>>>
>>> A description of this improvement from Darren is:
>>> "
>>
>> Drop the above two lines, we don't need these in the permanent git
>> history :-) Simply adding a pair of CC lines below the Signed-off-by
>> for Paul and myself is sufficient to indicate our involvement. If a
>> significant portion of the patch had been authored by either Paul or
>> myself, then getting our permission to include our Signed-off-by would be appropriate.
>> In this case, a simple CC will suffice.
> Ok, got it.
> 
>>>> &2 "Error: / is not supported as a build directory."
>>
>> I understand wanting to use stderr, but I don't see the >&2 very often
>> in our shell scripts. Is this common practice? If so, it's fine, I'm just wondering.
> I'm not sure this is common practice.
> I'm just following the existing style in scripts/oe-buildenv-internal and
> scripts/oe-setup-builddir. :-)
> 
>>> + # buggy readlink of Ubuntu 10.04 that doesn't ignore
>>> + trailing slash
>>
>>              a buggy        s/of/in/
>> slashes
> Thanks for pointing my mistakes
> 
>>> +        # and hence "readlink -f new_dir_to_be_created/" returns empty.
>>> +        # See YOCTO #671 for details.
>>
>>
>> Please don't include references to bug numbers in the code. Imagine
>> what it would look like if we included every bug in the source! :-)
>> Reference the bug in the git commit comment header.
> OK, got it.
> 
>>
>>
>>> +        BDIR=`echo $BDIR | sed -re 's|/+$||'`
>>> +
>>> +        BDIR=`readlink -f "$BDIR"`
>>> +        if [ -z "$BDIR" ]; then
>>> +            PARENTDIR=`dirname "$1"`
>>> +            echo >&2 "Error: the directory $PARENTDIR does not exist?"
>>>              return 1
>>>          fi
>>>      fi
>>
>> With the trivial changes mentioned above, this looks good to me.
> Hi Darren, thanks a lot for all the suggestions! I appreciate your efforts to help to make the patch better.
> Below is the updated patch (also pasted at the end of the mail):
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47
> 
> Please review it.
> 
> Thanks,
> -- Dexuan
> 
> commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47
> Author: Dexuan Cui <dexuan.cui@intel.com>
> Date:   Thu Aug 4 14:53:20 2011 +0800
> 
>     scripts/oe-buildenv-internal: improve the error detecting for $BDIR
> 
>     The previous fix for a bug in Ubuntu 10.04 readlink,
>     be2a2764d8ceb398d81714661e6f199c8b11946c, notified the user when a trailing
>     slash was used. As there is no semantic difference, simply remove any
>     trailing slashes and proceed without nagging the user.
> 
>     See [YOCTO #671] for more details.
> 
>     Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
>     Cc: Darren Hart <dvhart@linux.intel.com>
>     Cc: Paul Eggleton <paul.eggleton@linux.intel.com>

Looks good,

Acked-by: Darren Hart <dvhart@linux.intel.com>

Thanks Dexuan

> diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
> index 117b0c5..61ac18c 100755
> --- a/scripts/oe-buildenv-internal
> +++ b/scripts/oe-buildenv-internal
> @@ -28,14 +28,21 @@ if [ "x$BDIR" = "x" ]; then
>      if [ "x$1" = "x" ]; then
>          BDIR="build"
>      else
> -        BDIR=`readlink -f "$1"`
> -        if [ -z "$BDIR"  ]; then
> -            if expr "$1" : '.*/$' >/dev/null; then
> -                echo >&2 "Error: please remove any trailing / in the argument."
> -            else
> -                PARENTDIR=`dirname "$1"`
> -                echo >&2 "Error: the directory $PARENTDIR doesn't exist?"
> -            fi
> +        BDIR="$1"
> +        if [ "$BDIR" = "/" ]; then
> +            echo >&2 "Error: / is not supported as a build directory."
> +            return 1
> +        fi
> +
> +        # Remove any possible trailing slashes. This is used to work around
> +        # buggy readlink in Ubuntu 10.04 that doesn't ignore trailing slashes
> +        # and hence "readlink -f new_dir_to_be_created/" returns empty.
> +        BDIR=`echo $BDIR | sed -re 's|/+$||'`
> +
> +        BDIR=`readlink -f "$BDIR"`
> +        if [ -z "$BDIR" ]; then
> +            PARENTDIR=`dirname "$1"`
> +            echo >&2 "Error: the directory $PARENTDIR does not exist?"
>              return 1
>          fi
>      fi

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel