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. 2, 2011, 6:08 a.m.
Message ID <6def4624e63c8c7cf439dff32cb155dd2bba0ebe.1312265186.git.dexuan.cui@intel.com>
Download mbox | patch
Permalink /patch/9041/
State New, archived
Headers show

Comments

Dexuan Cui - Aug. 2, 2011, 6:08 a.m.
[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(-)
Richard Purdie - Aug. 2, 2011, 11:43 a.m.
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
Darren Hart - Aug. 3, 2011, 4:06 a.m.
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.
Dexuan Cui - Aug. 3, 2011, 6:46 a.m.
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
Darren Hart - Aug. 3, 2011, 1:50 p.m.
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.
Paul Eggleton - Aug. 3, 2011, 2:01 p.m.
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
Phil Blundell - Aug. 3, 2011, 2:11 p.m.
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.
Paul Eggleton - Aug. 3, 2011, 2:21 p.m.
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
Phil Blundell - Aug. 3, 2011, 2:25 p.m.
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.
Dexuan Cui - Aug. 4, 2011, 2:25 a.m.
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
Darren Hart - Aug. 4, 2011, 6 a.m.
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
> 
>
Richard Purdie - Aug. 4, 2011, 12:07 p.m.
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
Darren Hart - Aug. 4, 2011, 1:37 p.m.
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,
Richard Purdie - Aug. 4, 2011, 2:19 p.m.
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

Patch

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"