| Message ID | 20180718123443.7242-1-casantos@datacom.com.br |
|---|---|
| State | Accepted |
| Headers | show |
| Series | vim: install /bin/vi as a relative symlink | expand |
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
> 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.
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@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
> 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.
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
> 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.
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
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
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
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
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
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" == 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.
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)
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(-)