[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Carlos Santos]

Prevent creating a dangling symlink when vim is not present on the host
machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
on the same directory, otherwise link to "../usr/bin/vim".

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/vim/vim.mk | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/package/vim/vim.mk b/package/vim/vim.mk
index dbf71c573f..ee0c8b61e4 100644
--- a/package/vim/vim.mk
+++ b/package/vim/vim.mk
@@ -63,9 +63,15 @@ define VIM_REMOVE_DOCS
 endef
 
 # Avoid oopses with vipw/vigr, lack of $EDITOR and 'vi' command expectation
+ifeq ($(BR2_ROOTFS_MERGED_USR),y)
 define VIM_INSTALL_VI_SYMLINK
-	ln -sf /usr/bin/vim $(TARGET_DIR)/bin/vi
+	ln -sf vim $(TARGET_DIR)/usr/bin/vi
 endef
+else
+define VIM_INSTALL_VI_SYMLINK
+	ln -sf ../usr/bin/vim $(TARGET_DIR)/bin/vi
+endef
+endif
 VIM_POST_INSTALL_TARGET_HOOKS += VIM_INSTALL_VI_SYMLINK
 
 ifeq ($(BR2_PACKAGE_VIM_RUNTIME),y)
-- 
2.14.4

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Thomas Petazzoni]

Hello,

On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
> Prevent creating a dangling symlink when vim is not present on the host
> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
> on the same directory, otherwise link to "../usr/bin/vim".
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>

Baruch had already sent a patch with the same title/intention, but it
is no longer in the pending state in patchwork.

Could you clarify what happened, and which of the two patches is
relevant ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Carlos Santos]

> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
> To: "DATACOM" <casantos@datacom.com.br>
> Cc: "buildroot" <buildroot@buildroot.org>, "Yann Morin" <yann.morin.1998@free.fr>, "ratbert90" <aduskett@gmail.com>,
> "Baruch Siach" <baruch@tkos.co.il>
> Sent: Wednesday, July 18, 2018 10:03:50 AM
> Subject: Re: [Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

> Hello,
> 
> On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
>> Prevent creating a dangling symlink when vim is not present on the host
>> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
>> on the same directory, otherwise link to "../usr/bin/vim".
>> 
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> 
> Baruch had already sent a patch with the same title/intention, but it
> is no longer in the pending state in patchwork.
> 
> Could you clarify what happened, and which of the two patches is
> relevant ?

Baruch's patch is for busybox and is under review. Both can be applied
independently.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Baruch Siach]

Hi Thomas,

Thomas Petazzoni writes:
> Hello,
>
> On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
>> Prevent creating a dangling symlink when vim is not present on the host
>> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
>> on the same directory, otherwise link to "../usr/bin/vim".
>>
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>
> Baruch had already sent a patch with the same title/intention, but it
> is no longer in the pending state in patchwork.
>
> Could you clarify what happened, and which of the two patches is
> relevant ?

My vim patch is at

  http://patchwork.ozlabs.org/patch/943314/

I marked it as Rejected following the comment of Arnout. Carlos' patch
works around the merged /usr issue by changing the symlink target for
merged /usr. In my opinion this solution is error prone. It would be
much easier to allow dangling symlinks in the target directory, and
tweak the busybox install.sh to cope with that. That's what my pending
busybox patch suggests.

  http://patchwork.ozlabs.org/patch/944884/

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Carlos Santos]

> From: "Baruch Siach" <baruch@tkos.co.il>
> To: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
> Cc: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>, "Yann Morin" <yann.morin.1998@free.fr>,
> "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>
> Sent: Wednesday, July 18, 2018 11:57:06 PM
> Subject: Re: [Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

> Hi Thomas,
> 
> Thomas Petazzoni writes:
>> Hello,
>>
>> On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
>>> Prevent creating a dangling symlink when vim is not present on the host
>>> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
>>> on the same directory, otherwise link to "../usr/bin/vim".
>>>
>>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>>
>> Baruch had already sent a patch with the same title/intention, but it
>> is no longer in the pending state in patchwork.
>>
>> Could you clarify what happened, and which of the two patches is
>> relevant ?
> 
> My vim patch is at
> 
>  http://patchwork.ozlabs.org/patch/943314/
> 
> I marked it as Rejected following the comment of Arnout. Carlos' patch
> works around the merged /usr issue by changing the symlink target for
> merged /usr. In my opinion this solution is error prone.

Why?

> It would be
> much easier to allow dangling symlinks in the target directory, and
> tweak the busybox install.sh to cope with that. That's what my pending
> busybox patch suggests.
> 
>  http://patchwork.ozlabs.org/patch/944884/

Both patches could be applied. They are independent from each other.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Baruch Siach]

Hi Carlos,

Carlos Santos writes:
>> From: "Baruch Siach" <baruch@tkos.co.il>
>> To: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
>> Cc: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>, "Yann Morin" <yann.morin.1998@free.fr>,
>> "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>
>> Sent: Wednesday, July 18, 2018 11:57:06 PM
>> Subject: Re: [Buildroot] [PATCH] vim: install /bin/vi as a relative symlink
>
>> Hi Thomas,
>> 
>> Thomas Petazzoni writes:
>>> Hello,
>>>
>>> On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
>>>> Prevent creating a dangling symlink when vim is not present on the host
>>>> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
>>>> on the same directory, otherwise link to "../usr/bin/vim".
>>>>
>>>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>>>
>>> Baruch had already sent a patch with the same title/intention, but it
>>> is no longer in the pending state in patchwork.
>>>
>>> Could you clarify what happened, and which of the two patches is
>>> relevant ?
>> 
>> My vim patch is at
>> 
>>  http://patchwork.ozlabs.org/patch/943314/
>> 
>> I marked it as Rejected following the comment of Arnout. Carlos' patch
>> works around the merged /usr issue by changing the symlink target for
>> merged /usr. In my opinion this solution is error prone.
>
> Why?

Because it is confusing, for me at least. Changes in the filesystem
layout adding or removing directory symlinks might break them.

>> It would be
>> much easier to allow dangling symlinks in the target directory, and
>> tweak the busybox install.sh to cope with that. That's what my pending
>> busybox patch suggests.
>> 
>>  http://patchwork.ozlabs.org/patch/944884/
>
> Both patches could be applied. They are independent from each other.

But if we allow dangling non-relative symlinks I don't think we want to
add this complexity to the vim package, or any other package that
installs similar relative symlinks.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Carlos Santos]

> From: "Baruch Siach" <baruch@tkos.co.il>
> To: "DATACOM" <casantos@datacom.com.br>
> Cc: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>, "buildroot" <buildroot@buildroot.org>, "Yann Morin"
> <yann.morin.1998@free.fr>, "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>
> Sent: Friday, July 20, 2018 2:58:27 AM
> Subject: Re: [Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

> Hi Carlos,
> 
> Carlos Santos writes:
>>> From: "Baruch Siach" <baruch@tkos.co.il>
>>> To: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
>>> Cc: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>,
>>> "Yann Morin" <yann.morin.1998@free.fr>,
>>> "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>
>>> Sent: Wednesday, July 18, 2018 11:57:06 PM
>>> Subject: Re: [Buildroot] [PATCH] vim: install /bin/vi as a relative symlink
>>
>>> Hi Thomas,
>>> 
>>> Thomas Petazzoni writes:
>>>> Hello,
>>>>
>>>> On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
>>>>> Prevent creating a dangling symlink when vim is not present on the host
>>>>> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
>>>>> on the same directory, otherwise link to "../usr/bin/vim".
>>>>>
>>>>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>>>>
>>>> Baruch had already sent a patch with the same title/intention, but it
>>>> is no longer in the pending state in patchwork.
>>>>
>>>> Could you clarify what happened, and which of the two patches is
>>>> relevant ?
>>> 
>>> My vim patch is at
>>> 
>>>  http://patchwork.ozlabs.org/patch/943314/
>>> 
>>> I marked it as Rejected following the comment of Arnout. Carlos' patch
>>> works around the merged /usr issue by changing the symlink target for
>>> merged /usr. In my opinion this solution is error prone.
>>
>> Why?
> 
> Because it is confusing, for me at least. Changes in the filesystem
> layout adding or removing directory symlinks might break them.

Anyone that removes the directory symlinks is looking for trouble.

>>> It would be
>>> much easier to allow dangling symlinks in the target directory, and
>>> tweak the busybox install.sh to cope with that. That's what my pending
>>> busybox patch suggests.
>>> 
>>>  http://patchwork.ozlabs.org/patch/944884/
>>
>> Both patches could be applied. They are independent from each other.
> 
> But if we allow dangling non-relative symlinks I don't think we want to
> add this complexity to the vim package, or any other package that
> installs similar relative symlinks.

The complexity is a consequence of the BR2_ROOTFS_MERGED_USR stuff.
We already need to deal with such situations in six packages. I'd
rather declare merged /usr as mandatory and send all those ifeq's
down the tubes.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Thomas Petazzoni]

Hello,

On Fri, 20 Jul 2018 08:42:36 -0300 (BRT), Carlos Santos wrote:

> > But if we allow dangling non-relative symlinks I don't think we want to
> > add this complexity to the vim package, or any other package that
> > installs similar relative symlinks.  
> 
> The complexity is a consequence of the BR2_ROOTFS_MERGED_USR stuff.
> We already need to deal with such situations in six packages. I'd
> rather declare merged /usr as mandatory and send all those ifeq's
> down the tubes.

Perhaps we could introduce one or two helper functions that hide what
BR2_ROOTFS_MERGED_USR is doing. I thought a few minutes about this and
couldn't find immediately a good semantic/naming, but perhaps this is
a direction that could be investigated ?

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Arnout Vandecappelle]

On 23-07-18 15:17, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri, 20 Jul 2018 08:42:36 -0300 (BRT), Carlos Santos wrote:
> 
>>> But if we allow dangling non-relative symlinks I don't think we want to
>>> add this complexity to the vim package, or any other package that
>>> installs similar relative symlinks.  
>>
>> The complexity is a consequence of the BR2_ROOTFS_MERGED_USR stuff.
>> We already need to deal with such situations in six packages. I'd
>> rather declare merged /usr as mandatory and send all those ifeq's
>> down the tubes.
> 
> Perhaps we could introduce one or two helper functions that hide what
> BR2_ROOTFS_MERGED_USR is doing. I thought a few minutes about this and
> couldn't find immediately a good semantic/naming, but perhaps this is
> a direction that could be investigated ?

 The thing is, I don't believe that relying on BR2_ROOTFS_MERGED_USR is the
right approach. I've had projects with a usr -> . symlink in a custom skeleton,
for instance. We could of course declare such a skeleton invalid, but I don't
think that it is checked currently. Also I can imagine that there could be other
custom skeleton layouts that could create problems.

 We already use ln --relative in a few places, isn't that the easiest solution?

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Thomas Petazzoni]

Hello,

On Tue, 24 Jul 2018 09:55:40 +0200, Arnout Vandecappelle wrote:

>  The thing is, I don't believe that relying on BR2_ROOTFS_MERGED_USR is the
> right approach. I've had projects with a usr -> . symlink in a custom skeleton,
> for instance. We could of course declare such a skeleton invalid, but I don't
> think that it is checked currently. Also I can imagine that there could be other
> custom skeleton layouts that could create problems.
> 
>  We already use ln --relative in a few places, isn't that the easiest solution?

ln --relative is not supported on old distros, we even have a patch on
systemd to remove its use of ln --relative.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Arnout Vandecappelle]

On 24-07-18 10:30, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 24 Jul 2018 09:55:40 +0200, Arnout Vandecappelle wrote:
> 
>>  The thing is, I don't believe that relying on BR2_ROOTFS_MERGED_USR is the
>> right approach. I've had projects with a usr -> . symlink in a custom skeleton,
>> for instance. We could of course declare such a skeleton invalid, but I don't
>> think that it is checked currently. Also I can imagine that there could be other
>> custom skeleton layouts that could create problems.
>>
>>  We already use ln --relative in a few places, isn't that the easiest solution?
> 
> ln --relative is not supported on old distros, we even have a patch on
> systemd to remove its use of ln --relative.

 Crap, yes, I had done a quick grep, found some hits, and didn't notice it were
patches *removing* the --relative.

 I would still very much prefer to create a relative symlink automagically. Some
ideas:

1. Install a proper host-coreutils if the system-provided one is too old
(similar to cmake). Meh.
2. Always make absolute symlinks (pointing to target dir) and at some point call
https://github.com/brandt/symlinks to convert absolute to relative.
3. Make an 'ln' wrapper that converts absolute to relative, using e.g. the shell
scriptlet from [1].

 Regards,
 Arnout


[1]
https://unix.stackexchange.com/questions/85060/getting-relative-links-between-two-paths/85068#85068



-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Thomas Petazzoni]

Hello,

On Thu, 19 Jul 2018 05:57:06 +0300, Baruch Siach wrote:

> My vim patch is at
> 
>   http://patchwork.ozlabs.org/patch/943314/
> 
> I marked it as Rejected following the comment of Arnout. Carlos' patch
> works around the merged /usr issue by changing the symlink target for
> merged /usr. In my opinion this solution is error prone. It would be
> much easier to allow dangling symlinks in the target directory, and
> tweak the busybox install.sh to cope with that. That's what my pending
> busybox patch suggests.
> 
>   http://patchwork.ozlabs.org/patch/944884/

Allowing dangling symlinks is indeed desirable, but I think it is also
nice if we have as few dangling symlinks are possible. It looks cleaner
to me. So like Carlos said, I believe both your patch fixing the
Busybox installation and Carlos patch adjusting vim are useful.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Thomas Petazzoni]

Hello,

On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
> Prevent creating a dangling symlink when vim is not present on the host
> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
> on the same directory, otherwise link to "../usr/bin/vim".
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
>  package/vim/vim.mk | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

I know there is some ongoing discussion on how to handle this better in
a more generic way, but until those discussions are resolved, I've
applied this patch to master.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] vim: install /bin/vi as a relative symlink

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > Hello,
 > On Wed, 18 Jul 2018 09:34:43 -0300, Carlos Santos wrote:
 >> Prevent creating a dangling symlink when vim is not present on the host
 >> machine. With BR2_ROOTFS_MERGED_USR, just link to "vim", since they are
 >> on the same directory, otherwise link to "../usr/bin/vim".
 >> 
 >> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
 >> ---
 >> package/vim/vim.mk | 8 +++++++-
 >> 1 file changed, 7 insertions(+), 1 deletion(-)

 > I know there is some ongoing discussion on how to handle this better in
 > a more generic way, but until those discussions are resolved, I've
 > applied this patch to master.

Committed to 2018.02.x and 2018.05.x, thanks.

-- 
Bye, Peter Korsgaard