[PATCH 0/1] fix to bug 671

[PATCH 0/1] fix to bug 671

[Dexuan Cui]

The following changes since commit 46cf540e63a848512617b20fd8492f81bfb2f704:

  arch-armv7a.inc: fix armv7a-vfp-neon -> armv7a compat case (2011-08-01 16:49:11 +0100)

are available in the git repository at:
  git://git.pokylinux.org/poky-contrib dcui/bug-671
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=dcui/bug-671

Dexuan Cui (1):
  oe-init-build-env, scripts/oe-buildenv-internal: add error detecting
    for $BDIR

 oe-init-build-env            |    6 +++---
 scripts/oe-buildenv-internal |   13 +++++++++++--
 2 files changed, 14 insertions(+), 5 deletions(-)

-- 
1.7.6

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

[Dexuan Cui]

[YOCTO #671]

"readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
"readlink -f /tmp/non-existent-dir/" returns nothing, but according to
http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.

So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
a path with trailing slash here.

Moreever, I also add the detection of non-existent path, e.g.,
source oe-init-build-env /non-existent-dir/build
can be detected and we'll print an error msg.
And, if we get errors in oe-buildenv-internal, we should stop the script
and shouldn't further run.

Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
---
 oe-init-build-env            |    6 +++---
 scripts/oe-buildenv-internal |   13 +++++++++++--
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/oe-init-build-env b/oe-init-build-env
index 77332a7..cc30a3b 100755
--- a/oe-init-build-env
+++ b/oe-init-build-env
@@ -35,10 +35,10 @@ else
    fi
    OEROOT=`readlink -f "$OEROOT"`
    export OEROOT
-   . $OEROOT/scripts/oe-buildenv-internal
-   $OEROOT/scripts/oe-setup-builddir
+   . $OEROOT/scripts/oe-buildenv-internal && \
+        $OEROOT/scripts/oe-setup-builddir && \
+        [ -n "$BUILDDIR" ] && cd $BUILDDIR
    unset OEROOT
    unset BBPATH
-   [ -n "$BUILDDIR" ] && cd $BUILDDIR
 fi
 
diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
index c13fc40..117b0c5 100755
--- a/scripts/oe-buildenv-internal
+++ b/scripts/oe-buildenv-internal
@@ -21,7 +21,7 @@
 # It is assumed OEROOT is already defined when this is called
 if [ -z "$OEROOT" ]; then
     echo >&2 "Error: OEROOT is not defined!"
-    return
+    return 1
 fi
 
 if [ "x$BDIR" = "x" ]; then
@@ -29,6 +29,15 @@ if [ "x$BDIR" = "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
+            return 1
+        fi
     fi
 fi
 if expr "$BDIR" : '/.*' > /dev/null ; then
@@ -45,7 +54,7 @@ BUILDDIR=`readlink -f "$BUILDDIR"`
 
 if ! (test -d "$BITBAKEDIR"); then
     echo >&2 "Error: The bitbake directory ($BITBAKEDIR) does not exist!  Please ensure a copy of bitbake exists at this location"
-    return
+    return 1
 fi
 
 PATH="${OEROOT}/scripts:$BITBAKEDIR/bin/:$PATH"
-- 
1.7.6

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

[Richard Purdie]

On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
> [YOCTO #671]
> 
> "readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
> "readlink -f /tmp/non-existent-dir/" returns nothing, but according to
> http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
> hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
> and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.
> 
> So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
> a path with trailing slash here.
> 
> Moreever, I also add the detection of non-existent path, e.g.,
> source oe-init-build-env /non-existent-dir/build
> can be detected and we'll print an error msg.
> And, if we get errors in oe-buildenv-internal, we should stop the script
> and shouldn't further run.
> 
> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

Merged to master, thanks.

Richard

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

[Darren Hart]

On 08/02/2011 04:43 AM, Richard Purdie wrote:
> On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
>> [YOCTO #671]
>>
>> "readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
>> "readlink -f /tmp/non-existent-dir/" returns nothing, but according to
>> http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
>> hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
>> and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.
>>
>> So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
>> a path with trailing slash here.
>>
>> Moreever, I also add the detection of non-existent path, e.g.,
>> source oe-init-build-env /non-existent-dir/build
>> can be detected and we'll print an error msg.
>> And, if we get errors in oe-buildenv-internal, we should stop the script
>> and shouldn't further run.
>>
>> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
> 
> Merged to master, thanks.

For a patch to address a relatively benign bug I thought the standard
procedure would be for it to await feedback for more than 5 hours. I was
hoping to have an opportunity to review this fix as I was working with
the team in root causing the bug.

+ if [ -z "$BDIR" ]; then
+ if expr "$1" : '.*/$' >/dev/null; then
+ echo >&2 "Error: please remove any trailing / in the argument."

This portion of the patch is really not necessary. There is no
meaningful difference between the path with or without the trailing
slash, a much simpler and less noisy solution would be to simply strip
the trailing slash from the user input before doing the rest of the
input validation.

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

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

[Cui, Dexuan]

Darren Hart wrote on 2011-08-03:
> On 08/02/2011 04:43 AM, Richard Purdie wrote:
>> On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
>>> [YOCTO #671]
>> 
> For a patch to address a relatively benign bug I thought the standard
> procedure would be for it to await feedback for more than 5 hours. I
I agree. :-)

> was hoping to have an opportunity to review this fix as I was working
> with the team in root causing the bug.
> 
> + if [ -z "$BDIR" ]; then
> + if expr "$1" : '.*/$' >/dev/null; then echo >&2 "Error: please
> + remove any trailing / in the argument."
> 
> This portion of the patch is really not necessary. There is no
> meaningful difference between the path with or without the trailing
> slash, a much simpler and less noisy solution would be to simply strip
> the trailing slash from the user input before doing the rest of the input validation.
Hi Darren, thanks for the suggestion!
I considered the idea too, however, if we use the idea, it looks not that simple to gracefully and concisely handle the case if a user (by accident or by prank) passes / as $1 here, i.e., "readlink -f" would fail. So I didn't do that.

Thanks,
-- Dexuan

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

[Darren Hart]

On 08/02/2011 11:46 PM, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-03:
>> On 08/02/2011 04:43 AM, Richard Purdie wrote:
>>> On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
>>>> [YOCTO #671]
>>> 
>> For a patch to address a relatively benign bug I thought the
>> standard procedure would be for it to await feedback for more than
>> 5 hours. I
> I agree. :-)
> 
>> was hoping to have an opportunity to review this fix as I was
>> working with the team in root causing the bug.
>> 
>> + if [ -z "$BDIR" ]; then + if expr "$1" : '.*/$' >/dev/null; then
>> echo >&2 "Error: please + remove any trailing / in the argument."
>> 
>> This portion of the patch is really not necessary. There is no 
>> meaningful difference between the path with or without the
>> trailing slash, a much simpler and less noisy solution would be to
>> simply strip the trailing slash from the user input before doing
>> the rest of the input validation.
> Hi Darren, thanks for the suggestion! I considered the idea too,
> however, if we use the idea, it looks not that simple to gracefully
> and concisely handle the case if a user (by accident or by prank)
> passes / as $1 here, i.e., "readlink -f" would fail. So I didn't do
> that.

Hi Dexuan,

I had not considered that case, good catch. I can't think of a valid use
case for BDIR="/". Not only are write permissions unlikely, but the
build would conflict with /tmp as well.

if [ "$BDIR" == "/" ]; then
	echo "ERROR: / is not supported as a build directory."
	exit 1
fi
BDIR=${BDIR%/}

I would be happy with something like the above (untested). It seems a
lot more clear and direct to me.

In any case, I don't see any reason to bail out and ask the user to
remove a trailing slash. We should just do this and move on. There is no
semantic difference from the user's perspective, and the blame is to be
placed on readlink, not the user.

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

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

[Paul Eggleton]

On Wednesday 03 August 2011 14:50:45 Darren Hart wrote:
> if [ "$BDIR" == "/" ]; then
>         echo "ERROR: / is not supported as a build directory."
>         exit 1
> fi
> BDIR=${BDIR%/}

Works fine here - the only thing I'd suggest is use "=" instead of "==" as I 
think "==" is a bashism (at least dash doesn't like it - not that we support 
dash but we at least want the user to get past the setup script so they can 
get a proper error from sanity.bbclass).

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

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

[Phil Blundell]

On Wed, 2011-08-03 at 15:01 +0100, Paul Eggleton wrote:
> On Wednesday 03 August 2011 14:50:45 Darren Hart wrote:
> > if [ "$BDIR" == "/" ]; then
> >         echo "ERROR: / is not supported as a build directory."
> >         exit 1
> > fi
> > BDIR=${BDIR%/}
> 
> Works fine here - the only thing I'd suggest is use "=" instead of "==" as I 
> think "==" is a bashism 

Yeah, it is.

>not that we support dash but we at least want the user to get past the
>setup script so they can get a proper error from sanity.bbclass).

Has anybody ever tried to quantify how much work would be involved in
making OE work within the constraints of POSIX sh (i.e. work with dash)?
It does seem fairly obnoxious/embarrassing that you're obliged to
make /bin/sh be bash on a systemwide basis; I can't think offhand of any
other piece of software I use that has this kind of requirement.

p.

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

[Paul Eggleton]

On Wednesday 03 August 2011 15:11:07 Phil Blundell wrote:
> Has anybody ever tried to quantify how much work would be involved in
> making OE work within the constraints of POSIX sh (i.e. work with dash)?
> It does seem fairly obnoxious/embarrassing that you're obliged to
> make /bin/sh be bash on a systemwide basis; I can't think offhand of any
> other piece of software I use that has this kind of requirement.

Would that not entail fixing everything we build that contains shell scripts 
with bashisms that claim "#!/bin/sh"?

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

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

[Phil Blundell]

On Wed, 2011-08-03 at 15:21 +0100, Paul Eggleton wrote:
> On Wednesday 03 August 2011 15:11:07 Phil Blundell wrote:
> > Has anybody ever tried to quantify how much work would be involved in
> > making OE work within the constraints of POSIX sh (i.e. work with dash)?
> > It does seem fairly obnoxious/embarrassing that you're obliged to
> > make /bin/sh be bash on a systemwide basis; I can't think offhand of any
> > other piece of software I use that has this kind of requirement.
> 
> Would that not entail fixing everything we build that contains shell scripts 
> with bashisms that claim "#!/bin/sh"?

Yes, but anything that builds on current Ubuntu (etc) will presumably be
OK in that respect, and any shell scripts which are installed on the
target ought to be getting fixed anyway since bash is unlikely to be
available there.  If you assume that those two groups of things are
going to have to be solved anyway (by someone) then it isn't obvious to
me that the remaining problem set will be all that large.

If it were to become a real issue then one could write a class which
searched for shell scripts inside ${S} and reprocessed them to use
#!/bin/bash, or alternatively write an LD_PRELOAD sort of shim to detect
such scripts at exec() time and redirect them to bash rather than sh.

p.

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

[Cui, Dexuan]

Darren Hart wrote on 2011-08-03:
> On 08/02/2011 11:46 PM, Cui, Dexuan wrote:
>> Hi Darren, thanks for the suggestion! I considered the idea too,
>> however, if we use the idea, it looks not that simple to gracefully
>> and concisely handle the case if a user (by accident or by prank)
>> passes / as $1 here, i.e., "readlink -f" would fail. So I didn't do
>> that.
> 
> Hi Dexuan, 
> I had not considered that case, good catch. I can't think of a valid
> use case for BDIR="/". Not only are write permissions unlikely, but
Agree.

> the build would conflict with /tmp as well.
> 
> if [ "$BDIR" == "/" ]; then
> 	echo "ERROR: / is not supported as a build directory."
> 	exit 1
> fi
> BDIR=${BDIR%/}
Hi Darren,
This seems good to me.
Looks ${BDIR%/} can only remove one trailing slash. Do we need to consider more-than-one-slashes, e.g., $BDIR is /home/poky/build///? :-)   (We could use sed:  BDIR=`echo $BDIR | sed -re 's|/+$||'` , but I'm not sure if it deserves the complexity)
Darren, could you please help to make a patch? 
I really have few experience about how to validate a user's input. :-)

> I would be happy with something like the above (untested). It seems a
> lot more clear and direct to me.
> 
> In any case, I don't see any reason to bail out and ask the user to
> remove a trailing slash. We should just do this and move on. There is
> no semantic difference from the user's perspective, and the blame is
> to be placed on readlink, not the user.
I agree.

Thanks,
-- Dexuan

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

[Darren Hart]

On 08/03/2011 07:25 PM, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-03:
>> On 08/02/2011 11:46 PM, Cui, Dexuan wrote:
>>> Hi Darren, thanks for the suggestion! I considered the idea too,
>>> however, if we use the idea, it looks not that simple to gracefully
>>> and concisely handle the case if a user (by accident or by prank)
>>> passes / as $1 here, i.e., "readlink -f" would fail. So I didn't do
>>> that.
>>
>> Hi Dexuan, 
>> I had not considered that case, good catch. I can't think of a valid
>> use case for BDIR="/". Not only are write permissions unlikely, but
> Agree.
> 
>> the build would conflict with /tmp as well.
>>
>> if [ "$BDIR" == "/" ]; then
>> 	echo "ERROR: / is not supported as a build directory."
>> 	exit 1
>> fi
>> BDIR=${BDIR%/}
> Hi Darren,
> This seems good to me.
> Looks ${BDIR%/} can only remove one trailing slash. Do we need to consider more-than-one-slashes, e.g., $BDIR is /home/poky/build///? :-)   (We could use sed:  BDIR=`echo $BDIR | sed -re 's|/+$||'` , but I'm not sure if it deserves the complexity)
> Darren, could you please help to make a patch? 
> I really have few experience about how to validate a user's input. :-)

At some point I think it's fair for us to say "so don't do that" when
someone says "if I pass this random string of garbage to the script it
gives up and uses the current directory".

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 would suggest you include a patch to first revert the previous patch
that was applied to address this issue.

--
Darren

> 
>> I would be happy with something like the above (untested). It seems a
>> lot more clear and direct to me.
>>
>> In any case, I don't see any reason to bail out and ask the user to
>> remove a trailing slash. We should just do this and move on. There is
>> no semantic difference from the user's perspective, and the blame is
>> to be placed on readlink, not the user.
> I agree.
> 
> Thanks,
> -- Dexuan
> 
> 

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

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

[Cui, Dexuan]

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

    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?"
             return 1
         fi
     fi

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

[Darren Hart]

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

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

[Cui, Dexuan]

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>

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

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

[Phil Blundell]

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.

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

[Cui, Dexuan]

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

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

[Cui, Dexuan]

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

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

[Darren Hart]

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.

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

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

[Cui, Dexuan]

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>

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

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

[Darren Hart]

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

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

[Cui, Dexuan]

Darren Hart wrote on 2011-08-09:
> On 08/09/2011 07:04 AM, Cui, Dexuan wrote:
>> 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=dcu i/
>> bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47
>> 
>> -- 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, Darren!

Hi RP, could you please pull the patch? 
http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47

Thanks,
-- Dexuan

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

[Richard Purdie]

On Wed, 2011-08-10 at 11:18 +0800, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-09:
> > On 08/09/2011 07:04 AM, Cui, Dexuan wrote:
> >> 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=dcu i/
> >> bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47
> >> 
> >> -- 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, Darren!
> 
> Hi RP, could you please pull the patch? 
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47

Merged to master, thanks.

Richard

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

[Richard Purdie]

On Wed, 2011-08-10 at 11:18 +0800, Cui, Dexuan wrote:
> Darren Hart wrote on 2011-08-09:
> > On 08/09/2011 07:04 AM, Cui, Dexuan wrote:
> >> 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=dcu i/
> >> bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47
> >> 
> >> -- 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, Darren!
> 
> Hi RP, could you please pull the patch? 
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=dcui/bug-671&id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47

Merged to master, thanks.

Richard

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

[Richard Purdie]

On Tue, 2011-08-02 at 21:06 -0700, Darren Hart wrote:
> 
> On 08/02/2011 04:43 AM, Richard Purdie wrote:
> > On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
> >> [YOCTO #671]
> >>
> >> "readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
> >> "readlink -f /tmp/non-existent-dir/" returns nothing, but according to
> >> http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
> >> hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
> >> and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.
> >>
> >> So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
> >> a path with trailing slash here.
> >>
> >> Moreever, I also add the detection of non-existent path, e.g.,
> >> source oe-init-build-env /non-existent-dir/build
> >> can be detected and we'll print an error msg.
> >> And, if we get errors in oe-buildenv-internal, we should stop the script
> >> and shouldn't further run.
> >>
> >> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
> > 
> > Merged to master, thanks.
> 
> For a patch to address a relatively benign bug I thought the standard
> procedure would be for it to await feedback for more than 5 hours. I was
> hoping to have an opportunity to review this fix as I was working with
> the team in root causing the bug.

It is near impossible for me to tell who (if anyone) is working jointly
on an issue or expecting to review a patch. All I see are the complaints
when things don't merge promptly or something less than ideal merges too
soon (i.e. I can't win) :(.

If a group of people want to review and ack a patch before its accepted
can you please indicate this somewhere in the patch so I stand a chance
of figuring out what people are expecting me to do...

In this case the change isn't bad, there are just ways to improve it so
please send follow up patches.

Cheers,

Richard

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

[Darren Hart]

On 08/04/2011 05:07 AM, Richard Purdie wrote:
> On Tue, 2011-08-02 at 21:06 -0700, Darren Hart wrote:
>>
>> On 08/02/2011 04:43 AM, Richard Purdie wrote:
>>> On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
>>>> [YOCTO #671]
>>>>
>>>> "readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
>>>> "readlink -f /tmp/non-existent-dir/" returns nothing, but according to
>>>> http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
>>>> hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
>>>> and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.
>>>>
>>>> So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
>>>> a path with trailing slash here.
>>>>
>>>> Moreever, I also add the detection of non-existent path, e.g.,
>>>> source oe-init-build-env /non-existent-dir/build
>>>> can be detected and we'll print an error msg.
>>>> And, if we get errors in oe-buildenv-internal, we should stop the script
>>>> and shouldn't further run.
>>>>
>>>> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
>>>
>>> Merged to master, thanks.
>>
>> For a patch to address a relatively benign bug I thought the standard
>> procedure would be for it to await feedback for more than 5 hours. I was
>> hoping to have an opportunity to review this fix as I was working with
>> the team in root causing the bug.
> 
> It is near impossible for me to tell who (if anyone) is working jointly
> on an issue or expecting to review a patch. All I see are the complaints
> when things don't merge promptly or something less than ideal merges too
> soon (i.e. I can't win) :(.


In this case I was trying to refer back to what I had understood to be
the norm (waiting for 24 hours) to allow for feedback. I know it wasn't
a hard rule, but I didn't see any degree of urgency with this patch. If
your process is different than my understanding, please correct my
thinking so I know what to expect going forward. If not, then the above
is just meant as a friendly reminder that I, at least, am operating
under the assumption that patches will have a 24 hour review window
unless there is a pressing need to merge them sooner.


> If a group of people want to review and ack a patch before its accepted
> can you please indicate this somewhere in the patch so I stand a chance
> of figuring out what people are expecting me to do...


This is a critical part of the patch review process. 100% agreed. The
send-pull-requuest script knows to look for the CC: line below the
Signed-off-by for people whose input is relevant to the patch. Please
make use of this function to first notify them (people who helped on the
bug, recipe authors, maintainers, bug introducer, or people who have
recently modified the files in question) that a change is on the list
that may benefit from their review and second to notify the person
responsible for merging the patch that there are others involved who
should have an opportunity to provide feedback before the patch is merged.

Thanks,

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

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

[Richard Purdie]

On Thu, 2011-08-04 at 06:37 -0700, Darren Hart wrote:
> On 08/04/2011 05:07 AM, Richard Purdie wrote:
> > On Tue, 2011-08-02 at 21:06 -0700, Darren Hart wrote:
> >>
> >> On 08/02/2011 04:43 AM, Richard Purdie wrote:
> >>> On Tue, 2011-08-02 at 14:08 +0800, Dexuan Cui wrote:
> >>>> [YOCTO #671]
> >>>>
> >>>> "readlink -f" in Ubuntu 10.04 is buggy: it doesn't ignore a trailing / (e.g.,
> >>>> "readlink -f /tmp/non-existent-dir/" returns nothing, but according to
> >>>> http://www.gnu.org/s/coreutils/manual/coreutils.pdf it should do that --
> >>>> hence we get bug 671. It seems Ubuntu 10.10 or even later Ubuntu 11.04,
> >>>> and other Linux distributions(e.g., Open Suse 11.4) haven't such an issue.
> >>>>
> >>>> So I think we should detect this and ask Ubuntu 10.04 users to avoid supply
> >>>> a path with trailing slash here.
> >>>>
> >>>> Moreever, I also add the detection of non-existent path, e.g.,
> >>>> source oe-init-build-env /non-existent-dir/build
> >>>> can be detected and we'll print an error msg.
> >>>> And, if we get errors in oe-buildenv-internal, we should stop the script
> >>>> and shouldn't further run.
> >>>>
> >>>> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>
> >>>
> >>> Merged to master, thanks.
> >>
> >> For a patch to address a relatively benign bug I thought the standard
> >> procedure would be for it to await feedback for more than 5 hours. I was
> >> hoping to have an opportunity to review this fix as I was working with
> >> the team in root causing the bug.
> > 
> > It is near impossible for me to tell who (if anyone) is working jointly
> > on an issue or expecting to review a patch. All I see are the complaints
> > when things don't merge promptly or something less than ideal merges too
> > soon (i.e. I can't win) :(.
> 
> 
> In this case I was trying to refer back to what I had understood to be
> the norm (waiting for 24 hours) to allow for feedback. I know it wasn't
> a hard rule, but I didn't see any degree of urgency with this patch. If
> your process is different than my understanding, please correct my
> thinking so I know what to expect going forward. If not, then the above
> is just meant as a friendly reminder that I, at least, am operating
> under the assumption that patches will have a 24 hour review window
> unless there is a pressing need to merge them sooner.

Fair comment, its a 24 hour guideline and I thought that patch was safe
enough :/. I'll try and ensure I don't do that again.

Cheers,

Richard