Re: [Xen-devel] [PATCH] tools/pygrub: do not override pygrub with a symbolic link

[Christoph Egger <chegger@xxxxxxxxx>]

On 22.04.13 16:31, Ian Campbell wrote:
> On Mon, 2013-04-22 at 14:07 +0100, Christoph Egger wrote:
>> On 22.04.13 14:34, Ian Campbell wrote:
>>> On Mon, 2013-04-22 at 13:26 +0100, Christoph Egger wrote:
>>>> On 22.04.13 13:58, Ian Campbell wrote:
>>>>> On Mon, 2013-04-22 at 12:50 +0100, Christoph Egger wrote:
>>>>>> tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
>>>>>> and $(PRIVATE_BINDIR) are the same.
>>>>>
>>>>> More properly this is "fix the if condition we use to decide when to
>>>>> create the symlink", since it is already the intention to not override
>>>>> if they are the same  but the condition is written wrong since it
>>>>> includes DESTDIR on one side but not the other, which defeats the check.
>>>>>
>>>>> FWIW I would niuke the DESTDIR on the left rather than adding it on the
>>>>> right, it might even fit on one line then?
>>>>
>>>> Does not work. See comment below.
>>>
>>> OK, that makes sense I guess.
>>>
>>>>>> Signed-off-by: Christoph Egger <chegger@xxxxxxxxx>
>>>>>> diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
>>>>>> index 039f7f7..c3b34d7 100644
>>>>>> --- a/tools/pygrub/Makefile
>>>>>> +++ b/tools/pygrub/Makefile
>>>>>> @@ -15,8 +15,8 @@ install: all
>>>>>>                  --install-scripts=$(PRIVATE_BINDIR) --force
>>>>>>          $(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
>>>>>>          set -e; if [ "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
>>>>>> -                     "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
>>>>>> -            ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
>>>>>> +                     "`readlink -f $(DESTDIR)/$(PRIVATE_BINDIR)`" ]; 
>>>>>> then \
>>>>>> +            ln -sf $(DESTDIR)/$(PRIVATE_BINDIR)/pygrub 
>>>>>> $(DESTDIR)/$(BINDIR); \
>>>>>
>>>>> I don't think this change to the first path passed to ln is correct
>>>>> since this will become the content of the symlink, which doesn't want to
>>>>> include DESTDIR.
>>>>
>>>> What actually happens when $(BINDIR) and $(PRIVATE_BINDIR) are equal
>>>> is that the pygrub python script does not exist. pygrub is a symbolic
>>>> link to a file that doesn't exist instead.
>>>>
>>>> With this patch you will actually install the pygrub python script
>>>> rather a symbolic link.
>>>
>>> But you have broken the case where BINDIR and PRIVATE_BINDIR differ
>>> because /usr/bin/pygrub will be a symlink
>>> to /tmp/destdir.XYZ/usr/lib/xen/bin/pygrub and not
>>> to /usr/lib/xen/bin/pygrub.
>>
>> I see.
>>
>>>
>>> The code inside the if shouldn't even be run in the case you are trying
>>> to fix.
>>
>> Right. Comparing $(DESTDIR)/$(BINDIR) with $(PRIVATE_BINDIR) is
>> always different. Also when comparing $(BINDIR) with
>> $(DESTDIR)/$(PRIVATE_BINDIR) is always different
>> and the code inside the if runs.
>>
>> This works for me and should be fine for you:
> 
> I think so too.

Great. Attached this new version.


    tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
    and $(PRIVATE_BINDIR) are the same.
    
    Signed-off-by: Christoph Egger <chegger@xxxxxxxxx>

diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
index 039f7f7..0191638 100644
--- a/tools/pygrub/Makefile
+++ b/tools/pygrub/Makefile
@@ -14,7 +14,8 @@ install: all
                $(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
                --install-scripts=$(PRIVATE_BINDIR) --force
        $(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
-       set -e; if [ "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
+       set -e; if [ $(BINDIR) != $(PRIVATE_BINDIR) -a \
+                    "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
                     "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
            ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
        fi


Christoph

Attachment: patch_pygrub.diff
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel