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