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....@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....@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 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
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
