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

Submitted by Dexuan Cui on Aug. 4, 2011, 2:49 p.m.

Details

Message ID 1865303E0DED764181A9D882DEF65FB6B3046C9C11@shsmsx502.ccr.corp.intel.com
State New, archived
Headers show

Commit Message

Dexuan Cui Aug. 4, 2011, 2:49 p.m.
Darren Hart wrote on 2011-08-04:
> On 08/04/2011 12:37 AM, Cui, Dexuan wrote:
> 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! I'll use the description.

>> 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.
The latest manual of readlink says readlink should ignore trailing slash.

> Also, it is a good idea to avoid contractions in printed error messages.
> 
> 	echo >&2 "Error: the directory $PARENTDIR does not exist."
Ok, I'll change "doesn't" to "does not".

> Otherwise, this looks good to me.
Please review the new patch (I paste it at the end of the mail, too)
http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb

Thanks!
-- Dexuan

commit 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb
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

    Thanks a lot to Darren Hart and Paul Eggleton's suggestions!

    A description of this improvement from Darren is:
    "
    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.
    "

    Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

Patch hide | download patch | download mbox

diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
index 117b0c5..9988c9f 100755
--- a/scripts/oe-buildenv-internal
+++ b/scripts/oe-buildenv-internal
@@ -28,14 +28,22 @@  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 possible trailing slash. This is used to work around
+        # buggy readlink of Ubuntu 10.04 that doesn't ignore trailing slash
+        # and hence "readlink -f new_dir_to_be_created/" returns empty.
+        # See YOCTO #671 for details.
+        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

Comments

Phil Blundell Aug. 4, 2011, 2:53 p.m.
On Thu, 2011-08-04 at 22:49 +0800, Cui, Dexuan wrote:
> +        BDIR=`readlink -f "$BDIR"`
> +        if [ -z "$BDIR" ]; then
> +            PARENTDIR=`dirname "$1"`
> +            echo >&2 "Error: the directory $PARENTDIR does not exist?"
>              return 1
>          fi
>      fi

Just out of curiosity, could you not just do "mkdir -p $BDIR" and avoid
this whole set of complicated tests?  Or is there some reason why it's
actually important to know whether the parent directory existed already?

p.
Dexuan Cui Aug. 4, 2011, 3:14 p.m.
Phil Blundell wrote on 2011-08-04:
> On Thu, 2011-08-04 at 22:49 +0800, Cui, Dexuan wrote:
>> +        BDIR=`readlink -f "$BDIR"`
>> +        if [ -z "$BDIR" ]; then
>> +            PARENTDIR=`dirname "$1"`
>> +            echo >&2 "Error: the directory $PARENTDIR does not exist?"
>>              return 1
>>          fi
>>      fi
> 
> Just out of curiosity, could you not just do "mkdir -p $BDIR" and
> avoid this whole set of complicated tests?  Or is there some reason
> why it's actually important to know whether the parent directory existed already?
Hi Phil,
Actually in scripts/oe-setup-builddir, we do have a line
mkdir -p $BUILDDIR/conf .

The issue is: "readlink -f not_existent_dir/build" returns empty, so BUILDDIR would be assigned with `pwd` and this is not expected.

I don't really know why the test "readlink -f" is here -- "readlink -f" is used 3 times in scripts/oe-buildenv-internal. Maybe RP knows the history? I also think we can drop the tests "readlink -f" since we use "mkdir -p"?

Thanks,
-- Dexuan
Dexuan Cui Aug. 9, 2011, 2:13 a.m.
Cui, Dexuan wrote on 2011-08-04:
> Darren Hart wrote on 2011-08-04:
>> On 08/04/2011 12:37 AM, Cui, Dexuan wrote: 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! I'll use the description.
> 
>>> diff --git a/scripts/oe-buildenv-internal
>> 
>> 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.
> The latest manual of readlink says readlink should ignore trailing slash.
> 
>> Also, it is a good idea to avoid contractions in printed error messages.
>> 
>> 	echo >&2 "Error: the directory $PARENTDIR does not exist."
> Ok, I'll change "doesn't" to "does not".
> 
>> Otherwise, this looks good to me.
> Please review the new patch (I paste it at the end of the mail, too)
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb
Hi Darren,
Could you please comment on this new version of patch?
I sent it out for several days ago. Maybe it was drowned in the mailing list.

Thanks,
-- Dexuan
Darren Hart Aug. 9, 2011, 4:35 a.m.
On 08/08/2011 07:13 PM, Cui, Dexuan wrote:
> Cui, Dexuan wrote on 2011-08-04:
>> Darren Hart wrote on 2011-08-04:
>>> On 08/04/2011 12:37 AM, Cui, Dexuan wrote: 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! I'll use the description.
>>
>>>> diff --git a/scripts/oe-buildenv-internal
>>>
>>> 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.
>> The latest manual of readlink says readlink should ignore trailing slash.
>>
>>> Also, it is a good idea to avoid contractions in printed error messages.
>>>
>>> 	echo >&2 "Error: the directory $PARENTDIR does not exist."
>> Ok, I'll change "doesn't" to "does not".
>>
>>> Otherwise, this looks good to me.
>> Please review the new patch (I paste it at the end of the mail, too)
>> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb
> Hi Darren,
> Could you please comment on this new version of patch?
> I sent it out for several days ago. Maybe it was drowned in the mailing list.

Hi Dexuan, sorry for the delay, I have been on jury duty for a week, just getting back now.

> 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.

> 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.
> "
> 
> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

CC: Darren Hart <dvhart@linux.intel.com>
CC: Paul Eggleton <paul.eggleton@linux.intel.com>


> ---
> diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
> index 117b0c5..9988c9f 100755
> --- a/scripts/oe-buildenv-internal
> +++ b/scripts/oe-buildenv-internal
> @@ -28,14 +28,22 @@ 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."

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.

> +            return 1
> +        fi
> +
> +        # Remove possible trailing slash. This is used to work around

                                      slashes

> +        # buggy readlink of Ubuntu 10.04 that doesn't ignore trailing slash

             a buggy        s/of/in/                                     slashes

> +        # 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.


> +        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.