From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Egger <chegger@amazon.de>
Subject: Re: [PATCH] tools/pygrub: do not override pygrub with a
 symbolic link
Date: Mon, 22 Apr 2013 15:07:47 +0200
Message-ID: <51753623.9010401@amazon.de>
References: <517523FA.7050303@amazon.de>
	<1366631923.22143.47.camel@zakaz.uk.xensource.com>
	<51752C82.7010308@amazon.de>
	<1366634055.22143.64.camel@zakaz.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1366634055.22143.64.camel@zakaz.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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