llvm dev - Jul 2015 - [LLVMdev] [3.7 Release] RC1 has been tagged, Testing Phase I begins

On Sun, Jul 19, 2015 at 8:32 AM, Dimitry Andric <dimitry at andric.com>
wrote:
> On 19 Jul 2015, at 01:44, Dimitry Andric <dimitry at andric.com>
wrote:
> ...
>> Hm, strangely enough, this version of the script does not go further
than the Phase 2 installation, and does not run any tests?  This used to work
fine for the release_36 branch.
>>
>> I think it is because of the "set -o pipefail" which was
introduced, but I don't yet understand why this causes the Phase 2
installation to appear to fail, as there is no visible error.  I will
investigate, or work around it by removing the pipefail option again.
>
> It appears to be caused by the clean_RPATH() function, which has this line:
>
>       rpath=`objdump -x $Candidate | grep 'RPATH' | sed -e's/^
*RPATH *//'`
>
> If the objdump'd file does not have any RPATH, the whole statement will
fail, since set -o pipefail is in effect.  This terminates the script, since set
-e is also in effect.
Nice find!
>
> The line can be replaced with this instead:
>
>       rpath=`objdump -x $Candidate | sed -ne'/RPATH/{s/^ *RPATH
*//;p;}'`
How about just guarding it with an if statement? I'm worried about
adding non-obvious commands to the script, both for making it harder
to understand and for portability.

Thanks,
Hans

On 20 Jul 2015, at 23:34, Hans Wennborg <hans at chromium.org>
wrote:
> 
> On Sun, Jul 19, 2015 at 8:32 AM, Dimitry Andric <dimitry at
andric.com> wrote:
...
>> The line can be replaced with this instead:
>> 
>>      rpath=`objdump -x $Candidate | sed -ne'/RPATH/{s/^ *RPATH
*//;p;}'`
> 
> How about just guarding it with an if statement? I'm worried about
> adding non-obvious commands to the script, both for making it harder
> to understand and for portability.
It should be portable enough, as this works with with BSD and GNU sed,
but it may indeed be harder to understand.  So how about this:

Index: test-release.sh
==================================================================---
test-release.sh     (revision 242721)
+++ test-release.sh     (working copy)
@@ -359,10 +359,12 @@ function clean_RPATH() {
   local InstallPath="$1"
   for Candidate in `find $InstallPath/{bin,lib} -type f`; do
     if file $Candidate | grep ELF | egrep 'executable|shared object'
> /dev/null 2>&1 ; then
-      rpath=`objdump -x $Candidate | grep 'RPATH' | sed -e's/^
*RPATH *//'`
-      if [ -n "$rpath" ]; then
-        newrpath=`echo $rpath | sed -e's/.*\(\$ORIGIN[^:]*\).*/\1/'`
-        chrpath -r $newrpath $Candidate 2>&1 > /dev/null 2>&1
+      if rpath=`objdump -x $Candidate | grep 'RPATH'` ; then
+        rpath=`echo $rpath | sed -e's/^ *RPATH *//'`
+        if [ -n "$rpath" ]; then
+          newrpath=`echo $rpath | sed -e's/.*\(\$ORIGIN[^:]*\).*/\1/'`
+          chrpath -r $newrpath $Candidate 2>&1 > /dev/null
2>&1
+        fi
       fi
     fi
   done

-Dimitry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 194 bytes
Desc: Message signed with OpenPGP using GPGMail
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150721/b981615d/attachment.sig>

On Mon, Jul 20, 2015 at 3:15 PM, Dimitry Andric <dimitry at andric.com>
wrote:
> On 20 Jul 2015, at 23:34, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Sun, Jul 19, 2015 at 8:32 AM, Dimitry Andric <dimitry at
andric.com> wrote:
> ...
>>> The line can be replaced with this instead:
>>>
>>>      rpath=`objdump -x $Candidate | sed -ne'/RPATH/{s/^ *RPATH
*//;p;}'`
>>
>> How about just guarding it with an if statement? I'm worried about
>> adding non-obvious commands to the script, both for making it harder
>> to understand and for portability.
>
> It should be portable enough, as this works with with BSD and GNU sed,
> but it may indeed be harder to understand.  So how about this:
>
> Index: test-release.sh
> ==================================================================> ---
test-release.sh     (revision 242721)
> +++ test-release.sh     (working copy)
> @@ -359,10 +359,12 @@ function clean_RPATH() {
>    local InstallPath="$1"
>    for Candidate in `find $InstallPath/{bin,lib} -type f`; do
>      if file $Candidate | grep ELF | egrep 'executable|shared
object' > /dev/null 2>&1 ; then
> -      rpath=`objdump -x $Candidate | grep 'RPATH' | sed -e's/^
*RPATH *//'`
> -      if [ -n "$rpath" ]; then
> -        newrpath=`echo $rpath | sed
-e's/.*\(\$ORIGIN[^:]*\).*/\1/'`
> -        chrpath -r $newrpath $Candidate 2>&1 > /dev/null
2>&1
> +      if rpath=`objdump -x $Candidate | grep 'RPATH'` ; then
> +        rpath=`echo $rpath | sed -e's/^ *RPATH *//'`
> +        if [ -n "$rpath" ]; then
> +          newrpath=`echo $rpath | sed
-e's/.*\(\$ORIGIN[^:]*\).*/\1/'`
> +          chrpath -r $newrpath $Candidate 2>&1 > /dev/null
2>&1
> +        fi
>        fi
>      fi
>    done
Looks great, please commit.

Thanks,
Hans