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/04/2011 12:37 AM, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-04:
>> As for a patch, I'm on Jury duty all this week and only get to a very
>> small percentage of my tasks. I would appreciate if you would try to
>> put one together using the above source snippet, with the suggested
>> changes from Paul of course. Then just send it to the list with Paul
>> and myself on CC. We'll review and provided additional Acked-by's to
>> confirm we're all happy with the end result.
> I made a patch according to your and Paul's suggestions.
> Please review the patch (I also pasted at the end of this mail):
> http://git.pokylinux.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=13cd1538bc5be078039be2054f117610e2425951
> Please note I use sed to remove any trailing slash since ${BDIR%/} can only remove one trailing slash and this can cause issue, e.g., if $1 is /tmp/build_new//, *on Ubuntu 10.04*, we would get a weird msg "Error: the directory /tmp doesn't exist?"
> 
>> I would suggest you include a patch to first revert the previous patch
>> that was applied to address this issue.
> I'm trying to patch the first patch to save a revert commit... :-)
> 
> Thanks,
> -- Dexuan
> 
> commit 13cd1538bc5be078039be2054f117610e2425951
> 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
> 

Please remember to include a description of the problem and the approach
taken to fix it. This eliminates wasted time trying to decipher it later
or by people unfamiliar with the history. This is important even for
simple changes. In this case something like:

"
The previous fix for a bug in Ubuntu 10.04 readlink, $COMMIT_ID,
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.
"


>     Thanks a lot to Darren Hart and Paul Eggleton's sugestion!
> 
>     Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
> 
> diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
> index 117b0c5..4a44174 100755
> --- a/scripts/oe-buildenv-internal
> +++ b/scripts/oe-buildenv-internal
> @@ -28,14 +28,16 @@ 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
> +        BDIR=`echo $BDIR | sed -re 's|/+$||'`
> +        BDIR=`readlink -f "$BDIR"`
> +        if [ -z "$BDIR" ]; then
> +            PARENTDIR=`dirname "$1"`
> +            echo >&2 "Error: the directory $PARENTDIR doesn't exist?"

This shouldn't be a question. If the documented behavior of readlink is
to return empty when the path doesn't exist, then assume this to be the
case. Also, it is a good idea to avoid contractions in printed error
messages.

	echo >&2 "Error: the directory $PARENTDIR does not exist."

Otherwise, this looks good to me.

Thanks Dexuan!

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