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

login
register
mail settings
Submitter Dexuan Cui
Date Aug. 4, 2011, 2:49 p.m.
Message ID <1865303E0DED764181A9D882DEF65FB6B3046C9C11@shsmsx502.ccr.corp.intel.com>
Download mbox | patch
Permalink /patch/9311/
State New, archived
Headers show

Comments

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

Patch

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