Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 -- Dexuan commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Author: Dexuan Cui dexuan@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@intel.com Cc: Darren Hart dvh...@linux.intel.com Cc: Paul Eggleton paul.eggle...@linux.intel.com Looks good, Acked-by: Darren Hart dvh...@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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Merged to master, thanks. Richard ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 -- Dexuan commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Author: Dexuan Cui dexuan@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@intel.com Cc: Darren Hart dvh...@linux.intel.com Cc: Paul Eggleton paul.eggle...@linux.intel.com Looks good, Acked-by: Darren Hart dvh...@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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Merged to master, thanks. Richard ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@intel.com Date: Thu, 04 Aug 2011 06:53:20 + 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 buggys/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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Please review it. Thanks, -- Dexuan commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Author: Dexuan Cui dexuan@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@intel.com Cc: Darren Hart dvh...@linux.intel.com Cc: Paul Eggleton paul.eggle...@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@intel.com Date: Thu, 04 Aug 2011 06:53:20 + 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 buggys/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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Please review it. Thanks, -- Dexuan commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Author: Dexuan Cui dexuan@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@intel.com Cc: Darren Hart dvh...@linux.intel.com Cc: Paul Eggleton paul.eggle...@linux.intel.com Looks good, Acked-by: Darren Hart dvh...@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 -- Dexuan commit 2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Author: Dexuan Cui dexuan@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@intel.com Cc: Darren Hart dvh...@linux.intel.com Cc: Paul Eggleton paul.eggle...@linux.intel.com Looks good, Acked-by: Darren Hart dvh...@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-671id=2ece3a84d646bb4bf83f3c8aa3067cb99989da47 Thanks, -- Dexuan ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=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@intel.com Date: Thu, 04 Aug 2011 06:53:20 + 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@intel.com CC: Darren Hart dvh...@linux.intel.com CC: Paul Eggleton paul.eggle...@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 buggys/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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=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@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@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
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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-671id=593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Thanks! -- Dexuan commit 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Author: Dexuan Cui dexuan@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@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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. ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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.
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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.
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
[OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
[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@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
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@intel.com Merged to master, thanks. Richard ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Re: [OE-core] [PATCH 1/1] oe-init-build-env, scripts/oe-buildenv-internal: add error detecting for $BDIR
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@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 ___ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
