* [PATCH] tools/pygrub: do not override pygrub with a symbolic link
@ 2013-04-22 11:50 Christoph Egger
2013-04-22 18:34 ` Matt Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Egger @ 2013-04-22 11:50 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR) and $(PRIVATE_BINDIR) are the same.
Signed-off-by: Christoph Egger <chegger@amazon.de>
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); \
fi
.PHONY: clean
[-- Attachment #2: patch_pygrub.diff --]
[-- Type: text/plain, Size: 919 bytes --]
commit 78128508937af187889806b9be3de0d49b8915fa
Author: Christoph Egger <chegger@amazon.de>
Date: Fri Feb 8 12:44:12 2013 +0000
tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
and $(PRIVATE_BINDIR) are the same.
Signed-off-by: Christoph Egger <chegger@amazon.de>
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); \
fi
.PHONY: clean
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 11:50 [PATCH] tools/pygrub: do not override pygrub with a symbolic link Christoph Egger
@ 2013-04-22 18:34 ` Matt Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Matt Wilson @ 2013-04-22 18:34 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
On Mon, Apr 22, 2013 at 01:50:03PM +0200, Christoph Egger wrote:
> tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR) and $(PRIVATE_BINDIR) are the same.
>
> Signed-off-by: Christoph Egger <chegger@amazon.de>
Reviewed-by: Matt Wilson <msw@amazon.com>
> 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); \
> fi
>
> .PHONY: clean
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] tools/pygrub: do not override pygrub with a symbolic link
@ 2013-04-22 11:50 Christoph Egger
2013-04-22 11:58 ` Ian Campbell
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Egger @ 2013-04-22 11:50 UTC (permalink / raw)
To: xen-devel, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR) and $(PRIVATE_BINDIR) are the same.
Signed-off-by: Christoph Egger <chegger@amazon.de>
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); \
fi
.PHONY: clean
[-- Attachment #2: patch_pygrub.diff --]
[-- Type: text/plain, Size: 919 bytes --]
commit 78128508937af187889806b9be3de0d49b8915fa
Author: Christoph Egger <chegger@amazon.de>
Date: Fri Feb 8 12:44:12 2013 +0000
tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
and $(PRIVATE_BINDIR) are the same.
Signed-off-by: Christoph Egger <chegger@amazon.de>
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); \
fi
.PHONY: clean
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 11:50 Christoph Egger
@ 2013-04-22 11:58 ` Ian Campbell
2013-04-22 12:26 ` Christoph Egger
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-04-22 11:58 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
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?
> Signed-off-by: Christoph Egger <chegger@amazon.de>
> 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.
Ian.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 11:58 ` Ian Campbell
@ 2013-04-22 12:26 ` Christoph Egger
2013-04-22 12:34 ` Ian Campbell
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Egger @ 2013-04-22 12:26 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
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.
>
>> Signed-off-by: Christoph Egger <chegger@amazon.de>
>> 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.
Christoph
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 12:26 ` Christoph Egger
@ 2013-04-22 12:34 ` Ian Campbell
2013-04-22 13:07 ` Christoph Egger
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-04-22 12:34 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
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@amazon.de>
> >> 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.
The code inside the if shouldn't even be run in the case you are trying
to fix.
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.
Ian.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 12:34 ` Ian Campbell
@ 2013-04-22 13:07 ` Christoph Egger
2013-04-22 14:31 ` Ian Campbell
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Egger @ 2013-04-22 13:07 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
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@amazon.de>
>>>> 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:
if [ $(BINDIR) != $(PRIVATE_BINDIR) -a \
"`readlink -f $(DESTDIR)/$(BINDIR)`" != \
"`readlink -f $(PRIVATE_BINDIR)`" ]; then \
ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
Christoph
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 13:07 ` Christoph Egger
@ 2013-04-22 14:31 ` Ian Campbell
2013-04-22 14:48 ` Christoph Egger
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-04-22 14:31 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
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@amazon.de>
> >>>> 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.
>
> if [ $(BINDIR) != $(PRIVATE_BINDIR) -a \
> "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
> "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
> ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
>
> Christoph
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 14:31 ` Ian Campbell
@ 2013-04-22 14:48 ` Christoph Egger
2013-04-22 18:36 ` Matt Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Egger @ 2013-04-22 14:48 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 3618 bytes --]
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@amazon.de>
>>>>>> 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@amazon.de>
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 #2: patch_pygrub.diff --]
[-- Type: text/plain, Size: 913 bytes --]
commit 80034731fee32b4c4c1c5c326a265e6f75469a8f
Author: Christoph Egger <chegger@amazon.de>
Date: Fri Feb 8 12:44:12 2013 +0000
tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
and $(PRIVATE_BINDIR) are the same.
Signed-off-by: Christoph Egger <chegger@amazon.de>
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
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 14:48 ` Christoph Egger
@ 2013-04-22 18:36 ` Matt Wilson
2013-04-24 11:56 ` Ian Campbell
0 siblings, 1 reply; 11+ messages in thread
From: Matt Wilson @ 2013-04-22 18:36 UTC (permalink / raw)
To: Christoph Egger; +Cc: Ian Campbell, xen-devel
On Mon, Apr 22, 2013 at 04:48:01PM +0200, Christoph Egger wrote:
>
> tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
> and $(PRIVATE_BINDIR) are the same.
>
> Signed-off-by: Christoph Egger <chegger@amazon.de>
This one looks good to me too.
Reviewed-by: Matt Wilson <msw@amazon.com>
> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/pygrub: do not override pygrub with a symbolic link
2013-04-22 18:36 ` Matt Wilson
@ 2013-04-24 11:56 ` Ian Campbell
0 siblings, 0 replies; 11+ messages in thread
From: Ian Campbell @ 2013-04-24 11:56 UTC (permalink / raw)
To: Matt Wilson; +Cc: Christoph Egger, xen-devel
On Mon, 2013-04-22 at 19:36 +0100, Matt Wilson wrote:
> On Mon, Apr 22, 2013 at 04:48:01PM +0200, Christoph Egger wrote:
> >
> > tools/pygrub: Do not override pygrub with a symbolic link if $(BINDIR)
> > and $(PRIVATE_BINDIR) are the same.
> >
> > Signed-off-by: Christoph Egger <chegger@amazon.de>
>
> This one looks good to me too.
>
> Reviewed-by: Matt Wilson <msw@amazon.com>
Thanks, also acked + applied. I rewrote the commit message as follows so
that the initial summary line was < 80 characters
tools/pygrub: Fix install when $(BINDIR) and $(PRIVATE_BINDIR) are the same
Do not override pygrub with a symbolic link in this case.
Signed-off-by: Christoph Egger <chegger@amazon.de>
Reviewed-by: Matt Wilson <msw@amazon.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
[ ijc -- reworded summary to fit on one line ]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-04-24 11:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-22 11:50 [PATCH] tools/pygrub: do not override pygrub with a symbolic link Christoph Egger
2013-04-22 18:34 ` Matt Wilson
-- strict thread matches above, loose matches on Subject: below --
2013-04-22 11:50 Christoph Egger
2013-04-22 11:58 ` Ian Campbell
2013-04-22 12:26 ` Christoph Egger
2013-04-22 12:34 ` Ian Campbell
2013-04-22 13:07 ` Christoph Egger
2013-04-22 14:31 ` Ian Campbell
2013-04-22 14:48 ` Christoph Egger
2013-04-22 18:36 ` Matt Wilson
2013-04-24 11:56 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.