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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

> 
> if [ $(BINDIR) != $(PRIVATE_BINDIR) -a \
>      "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
>      "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
>    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
> 
> Christoph
> 



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