fix to bug 671

[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 <[email protected]>

Quoted 17 lines · 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.

Quoted 1 line

Signed-off-by Dexuan Cui <[email protected]>

Merged to master, thanks.

Quoted 3 lines · Dexuan Cui

Quoted 1 line

[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

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 wrote on 2011-08-03:

Quoted 7 lines · Richard Purdie

Quoted 3 lines · Dexuan Cui

Quoted 1 line

[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. :-)

Quoted 11 lines

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.

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

Quoted 5 lines · 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

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.

Quoted 5 lines · 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"?

Quoted 5 lines

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 wrote on 2011-08-03:

Quoted 10 lines · Dexuan

Quoted 5 lines

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.

Quoted 7 lines

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. :-)

Quoted 8 lines

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

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 Hart wrote on 2011-08-04:

Quoted 6 lines

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?"

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

scripts/oe-buildenv-internal +0 -8

index 117b0c5..4a44174 100755
@@ -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

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. "

Quoted 14 lines

(quoted diff) +0 -0

b/scripts/oe-buildenv-internal index 117b0c5..4a44174 100755

scripts/oe-buildenv-internal +0 -1

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

Quoted 1 line

&2 "Error: please remove any trailing / in the argument." -

else - PARENTDIR=dirname "$1" - echo

Quoted 1 line

&2 "Error: the directory $PARENTDIR doesn't exist?" - fi +

      BDIR="$1" +        if [ "$BDIR" = "/" ]; then +            echo

Quoted 5 lines

Quoted 1 line

&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.

scripts/oe-buildenv-internal if [ "x$BDIR" = "x" ]; then · line 49

Quoted 7 lines

scripts/oe-buildenv-internal +4 -0

+        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?

Darren Hart wrote on 2011-08-04:

Quoted 10 lines

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.

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +0 -0

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.

Quoted 3 lines

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".

Quoted 1 line

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

Cui, Dexuan wrote on 2011-08-04:

Quoted 23 lines

Darren Hart wrote on 2011-08-04:

Quoted 10 lines

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.

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +0 -0

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.

Quoted 3 lines

Also, it is a good idea to avoid contractions in printed error messages.

echo >&2 "Error: the directory $PARENTDIR does not exist."

Quoted 5 lines

Ok, I'll change "doesn't" to "does not".

Quoted 1 line

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.

From 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Mon Sep 17 00:00:00 2001
From: Dexuan Cui <[email protected]>
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:
"

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 <[email protected]>

---

scripts/oe-buildenv-internal +0 -1

index 117b0c5..9988c9f 100755
@@ -28,14 +28,22 @@ if [ "x$BDIR" = "x" ]; then
     if [ "x$1" = "x" ]; then
         BDIR="build"
     else
-        BDIR=`readlink -f "$1"`

Quoted 9 lines

From 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Mon Sep 17 00:00:00 2001
From: Dexuan Cui <[email protected]> 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.

Quoted 1 line

Quoted 1 line

&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.

Quoted 2 lines

+ # buggy readlink of Ubuntu 10.04 that doesn't ignore
+ trailing slash

a buggy s/of/in/
slashes

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Review

Quoted 11 lines

(quoted diff) +6 -0

+        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 wrote on 2011-08-09:

Quoted 11 lines · Dexuan

Quoted 9 lines

From 593e3506f4d4d9f6030100eac8c8e0af7f8ce3eb Mon Sep 17 00:00:00 2001
From: Dexuan Cui <[email protected]> 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:
"

Quoted 6 lines

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.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

&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. :-)

Quoted 5 lines

Quoted 2 lines

+ # buggy readlink of Ubuntu 10.04 that doesn't ignore
+ trailing slash

a buggy s/of/in/
slashes

Thanks for pointing my mistakes

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Quoted 12 lines

scripts/oe-buildenv-internal if [ "x$BDIR" = "x" ]; then · line 48

Quoted 11 lines

scripts/oe-buildenv-internal +6 -0

+        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

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Cc Darren Hart <[email protected]>

Cc Paul Eggleton <[email protected]>

scripts/oe-buildenv-internal +15 -8

index 117b0c5..61ac18c 100755
@@ -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

Quoted 22 lines

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 <[email protected]>
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.

Quoted 3 lines

Signed-off-by Dexuan Cui <[email protected]>

Cc Darren Hart <[email protected]>

Cc Paul Eggleton <[email protected]>

Looks good,

Acked-by Darren Hart <[email protected]>

Darren Hart wrote on 2011-08-09:

Quoted 27 lines · Dexuan

Quoted 25 lines

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 <[email protected]>
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 <[email protected]>

Cc Darren Hart <[email protected]>

Cc Paul Eggleton <[email protected]>

Quoted 3 lines

Looks good,

Acked-by Darren Hart <[email protected]>

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

Darren Hart wrote on 2011-08-09:

Quoted 27 lines · Dexuan

Quoted 25 lines

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 <[email protected]>
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 <[email protected]>

Cc Darren Hart <[email protected]>

Cc Paul Eggleton <[email protected]>

Quoted 3 lines

Looks good,

Acked-by Darren Hart <[email protected]>

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

Quoted 18 lines · Richard Purdie

Quoted 17 lines · 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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Dexuan Cui <[email protected]>

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.

Quoted 20 lines · Darren Hart

Quoted 18 lines · Richard Purdie

Quoted 17 lines · 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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Dexuan Cui <[email protected]>

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

Quoted 30 lines · Richard Purdie

Quoted 20 lines · Darren Hart

Quoted 18 lines · Richard Purdie

Quoted 17 lines · 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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Dexuan Cui <[email protected]>

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.

Quoted 4 lines

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.