diff mbox series

[2/2] make: install/uninstall tools symlinks to kmod

Message ID 20240126-master-v1-2-6257d039a30a@gmail.com (mailing list archive)
State New, archived
Headers show
Series Polish module_directory help string, install symlinks | expand

Commit Message

Emil Velikov via B4 Relay Jan. 26, 2024, 2:43 p.m. UTC
From: Emil Velikov <emil.velikov@collabora.com>

Currently we create symlinks like modprobe (pointing to kmod), during
the normal `make` build. Although those were never installed.

Add a few lines in the install-exec-hook, to ensure they're present at
`make install` time. Thus one can actually use those without additional
changes. As an added bonus, distributions can drop the similar hunk from
their packaging.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Out of curiosity: are there any plans about releasing v32? I'm
interested in the recent /usr/lib/modules (module_directory) patches.

Thanks o/
---
 Makefile.am | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Lucas De Marchi Jan. 29, 2024, 4:40 p.m. UTC | #1
On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.velikov@collabora.com>
>
>Currently we create symlinks like modprobe (pointing to kmod), during
>the normal `make` build. Although those were never installed.
>
>Add a few lines in the install-exec-hook, to ensure they're present at
>`make install` time. Thus one can actually use those without additional
>changes. As an added bonus, distributions can drop the similar hunk from
>their packaging.

It was a long time ago and my memory may be fading, but afair the fact
that distros were doing it was what prevented us from adding the
symlinks ourselves.... and then we never re-visited this.

I'll dig some history before applying to make sure I'm not forgetting
something.

>
>Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>---
>Out of curiosity: are there any plans about releasing v32? I'm
>interested in the recent /usr/lib/modules (module_directory) patches.


yes, I should be releasing that soon, probably this week or the next.

thanks
Lucas De Marchi
Emil Velikov Jan. 29, 2024, 5:23 p.m. UTC | #2
On Mon, 29 Jan 2024 at 16:40, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
> >From: Emil Velikov <emil.velikov@collabora.com>
> >
> >Currently we create symlinks like modprobe (pointing to kmod), during
> >the normal `make` build. Although those were never installed.
> >
> >Add a few lines in the install-exec-hook, to ensure they're present at
> >`make install` time. Thus one can actually use those without additional
> >changes. As an added bonus, distributions can drop the similar hunk from
> >their packaging.
>
> It was a long time ago and my memory may be fading, but afair the fact
> that distros were doing it was what prevented us from adding the
> symlinks ourselves.... and then we never re-visited this.
>
> I'll dig some history before applying to make sure I'm not forgetting
> something.
>
Ack, makes sense. Fwiw I've already opened a MR with the Arch team
highlighting these changes.

Sadly I don't have contact for other maintainers.

> >
> >Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >---
> >Out of curiosity: are there any plans about releasing v32? I'm
> >interested in the recent /usr/lib/modules (module_directory) patches.
>
>
> yes, I should be releasing that soon, probably this week or the next.
>

Thanks much appreciated.
Emil
Lucas De Marchi Jan. 29, 2024, 10:09 p.m. UTC | #3
On Mon, Jan 29, 2024 at 05:23:44PM +0000, Emil Velikov wrote:
>On Mon, 29 Jan 2024 at 16:40, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
>> >From: Emil Velikov <emil.velikov@collabora.com>
>> >
>> >Currently we create symlinks like modprobe (pointing to kmod), during
>> >the normal `make` build. Although those were never installed.
>> >
>> >Add a few lines in the install-exec-hook, to ensure they're present at
>> >`make install` time. Thus one can actually use those without additional
>> >changes. As an added bonus, distributions can drop the similar hunk from
>> >their packaging.
>>
>> It was a long time ago and my memory may be fading, but afair the fact
>> that distros were doing it was what prevented us from adding the
>> symlinks ourselves.... and then we never re-visited this.
>>
>> I'll dig some history before applying to make sure I'm not forgetting
>> something.
>>
>Ack, makes sense. Fwiw I've already opened a MR with the Arch team
>highlighting these changes.

no wonder my memory faded as it was almost 12 years ago and during  kmod 3 ~ 5.


00fc926 build-sys: create symlinks instead of building separate tools

	when we started using symlinks to a single kmod binary

7bbf523 build-sys: create symlinks if we are installing tools
12fd9cd build-sys: forcefully create links

	when we started creating the symlinks

fe8b067 build-sys: do not create symlinks by default

	stop creating the symlinks since it was not working across
	distros and creating more problems than solving

Now that distros configure all the paths through configure options,
we can probably re-attempt this.

Cc'ing some pkg maintainers


Lucas De Marchi

>
>Sadly I don't have contact for other maintainers.
>
>> >
>> >Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> >---
>> >Out of curiosity: are there any plans about releasing v32? I'm
>> >interested in the recent /usr/lib/modules (module_directory) patches.
>>
>>
>> yes, I should be releasing that soon, probably this week or the next.
>>
>
>Thanks much appreciated.
>Emil
Emil Velikov Jan. 31, 2024, 1:45 p.m. UTC | #4
On Mon, 29 Jan 2024 at 22:09, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Mon, Jan 29, 2024 at 05:23:44PM +0000, Emil Velikov wrote:
> >On Mon, 29 Jan 2024 at 16:40, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >>
> >> On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
> >> >From: Emil Velikov <emil.velikov@collabora.com>
> >> >
> >> >Currently we create symlinks like modprobe (pointing to kmod), during
> >> >the normal `make` build. Although those were never installed.
> >> >
> >> >Add a few lines in the install-exec-hook, to ensure they're present at
> >> >`make install` time. Thus one can actually use those without additional
> >> >changes. As an added bonus, distributions can drop the similar hunk from
> >> >their packaging.
> >>
> >> It was a long time ago and my memory may be fading, but afair the fact
> >> that distros were doing it was what prevented us from adding the
> >> symlinks ourselves.... and then we never re-visited this.
> >>
> >> I'll dig some history before applying to make sure I'm not forgetting
> >> something.
> >>
> >Ack, makes sense. Fwiw I've already opened a MR with the Arch team
> >highlighting these changes.
>
> no wonder my memory faded as it was almost 12 years ago and during  kmod 3 ~ 5.
>
>
> 00fc926 build-sys: create symlinks instead of building separate tools
>
>         when we started using symlinks to a single kmod binary
>
> 7bbf523 build-sys: create symlinks if we are installing tools
> 12fd9cd build-sys: forcefully create links
>
>         when we started creating the symlinks
>
> fe8b067 build-sys: do not create symlinks by default
>
>         stop creating the symlinks since it was not working across
>         distros and creating more problems than solving
>
> Now that distros configure all the paths through configure options,
> we can probably re-attempt this.
>
> Cc'ing some pkg maintainers
>

Thanks for the information. Curiosity got the best of me, so I had a
look across few distros:

Arch - installs kmod in /usr/bin, symlinks are in /usr/bin.

Debian - installs kmod in /usr/bin and symlinks are split across
/usr/bin and /usr/sbin. Cannot find any references if they're aiming
to merge /usr/bin and /usr/sbin.

Fedora - installs kmod in /usr/bin and symlinks are split across
/usr/bin and /usr/sbin. Fedora 40 is aiming to "merge" /usr/bin and
/usr/sbin.

Gentoo - installs kmod in /bin and symlinks are split across /bin and
/sbin. Gentoo has merged /usr (/{,s}bin being a symlink to
/usr/{.s}bin) and merged bin (/usr/sbin is a symlink to /usr/bin in
some instances.
The https://wiki.gentoo.org/wiki/Merge-usr wiki explains the tool
aiming with merge conversions, although it seems to be optional albeit
highly(?) recommended.

TlDr: Distro variations still exist. Arch is fine, Fedora 40 should be
OK. Debian will need an in-package tweak. Gentoo will be fine for most
or at least some instances.

Personally (Arch user here) it makes sense to land and release this,
alongside the /usr/lib/modules support. Although if you think it makes
sense to defer for a later date, that's fine with me.

Regards,
Emil
Marco d'Itri Jan. 31, 2024, 1:52 p.m. UTC | #5
On Jan 31, Emil Velikov <emil.l.velikov@gmail.com> wrote:

> Debian - installs kmod in /usr/bin and symlinks are split across
> /usr/bin and /usr/sbin. Cannot find any references if they're aiming
> to merge /usr/bin and /usr/sbin.
There are no such plans at this point.
I definitely will not be pushing for that, but somebody else might.
Lucas De Marchi Feb. 2, 2024, 6:53 p.m. UTC | #6
On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
>From: Emil Velikov <emil.velikov@collabora.com>
>
>Currently we create symlinks like modprobe (pointing to kmod), during
>the normal `make` build. Although those were never installed.
>
>Add a few lines in the install-exec-hook, to ensure they're present at
>`make install` time. Thus one can actually use those without additional
>changes. As an added bonus, distributions can drop the similar hunk from
>their packaging.
>
>Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>---
>Out of curiosity: are there any plans about releasing v32? I'm
>interested in the recent /usr/lib/modules (module_directory) patches.
>
>Thanks o/
>---
> Makefile.am | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/Makefile.am b/Makefile.am
>index 4062d81..a22d1b1 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -111,9 +111,19 @@ install-exec-hook:
> 		ln -sf $$so_img_rel_target_prefix$(rootlibdir)/$$so_img_name $(DESTDIR)$(libdir)/libkmod.so && \
> 		mv $(DESTDIR)$(libdir)/libkmod.so.* $(DESTDIR)$(rootlibdir); \
> 	fi
>+if BUILD_TOOLS
>+	for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
>+		$(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \

I was about to apply this, but then noticed a problem: I think we should
use a relative symlink here.

$ DESTDIR=/tmp/inst make install
$ ls -l /tmp/inst/usr/bin
total 700
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 depmod -> /usr/bin/kmod
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 insmod -> /usr/bin/kmod
-rwxr-xr-x 1 ldmartin ldmartin 715432 Feb  2 12:44 kmod
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 lsmod -> /usr/bin/kmod
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modinfo -> /usr/bin/kmod
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modprobe -> /usr/bin/kmod
lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 rmmod -> /usr/bin/kmod

should had been e.g. depmod -> ./kmod

Simplest fix without resorting to calculating the shortest symlink is to
assume: the symlinks should be in the same dir as kmod, just like if
they were not symlinks.

diff --git a/Makefile.am b/Makefile.am
index 39a46f4..6df2f60 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -113,7 +113,7 @@ install-exec-hook:
         fi
  if BUILD_TOOLS
         for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
-               $(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
+               $(LN_S) ./kmod $(DESTDIR)$(bindir)/$$tool; \
         done
  endif

does that seem ok squashed on your patch?

thanks
Lucas De Marchi
Emil Velikov Feb. 5, 2024, 12:37 p.m. UTC | #7
Hey Lucas,

On Fri, 2 Feb 2024 at 18:53, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
> >From: Emil Velikov <emil.velikov@collabora.com>
> >
> >Currently we create symlinks like modprobe (pointing to kmod), during
> >the normal `make` build. Although those were never installed.
> >
> >Add a few lines in the install-exec-hook, to ensure they're present at
> >`make install` time. Thus one can actually use those without additional
> >changes. As an added bonus, distributions can drop the similar hunk from
> >their packaging.
> >
> >Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >---
> >Out of curiosity: are there any plans about releasing v32? I'm
> >interested in the recent /usr/lib/modules (module_directory) patches.
> >
> >Thanks o/
> >---
> > Makefile.am | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> >diff --git a/Makefile.am b/Makefile.am
> >index 4062d81..a22d1b1 100644
> >--- a/Makefile.am
> >+++ b/Makefile.am
> >@@ -111,9 +111,19 @@ install-exec-hook:
> >               ln -sf $$so_img_rel_target_prefix$(rootlibdir)/$$so_img_name $(DESTDIR)$(libdir)/libkmod.so && \
> >               mv $(DESTDIR)$(libdir)/libkmod.so.* $(DESTDIR)$(rootlibdir); \
> >       fi
> >+if BUILD_TOOLS
> >+      for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
> >+              $(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
>
> I was about to apply this, but then noticed a problem: I think we should
> use a relative symlink here.
>
> $ DESTDIR=/tmp/inst make install
> $ ls -l /tmp/inst/usr/bin
> total 700
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 depmod -> /usr/bin/kmod
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 insmod -> /usr/bin/kmod
> -rwxr-xr-x 1 ldmartin ldmartin 715432 Feb  2 12:44 kmod
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 lsmod -> /usr/bin/kmod
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modinfo -> /usr/bin/kmod
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modprobe -> /usr/bin/kmod
> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 rmmod -> /usr/bin/kmod
>
> should had been e.g. depmod -> ./kmod
>
> Simplest fix without resorting to calculating the shortest symlink is to
> assume: the symlinks should be in the same dir as kmod, just like if
> they were not symlinks.
>

I'm not sure I follow, can you elaborate what is the issue?

Are you trying to use/run files installed in DESTDIR directly? If so,
that won't work for a few reasons:
 - kmod does not link to the in-DESTDIR libkmod.so, admittedly one can
workaround it with LD_PRELOAD/LD_LIBRARY_PATH
 - kmod tries to open the depmod config files in the normal
non-DESTDIR locations

> diff --git a/Makefile.am b/Makefile.am
> index 39a46f4..6df2f60 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -113,7 +113,7 @@ install-exec-hook:
>          fi
>   if BUILD_TOOLS
>          for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
> -               $(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
> +               $(LN_S) ./kmod $(DESTDIR)$(bindir)/$$tool; \
>          done
>   endif
>
> does that seem ok squashed on your patch?
>

I'm not a huge fan of using relative symlinks, especially if the tool
is run as root. In my experience that makes things harder to audit and
prevent accidental breakages.

As an example, my Arch box has the following:

 - /usr/bin/init -> ../lib/systemd/systemd
 - /usr/bin/ld.so -> ../lib/ld-linux-x86-64.so.2
Hmm should probably see if we can change these and how many things will break

 - /usr/bin/lirc-setup -> ../lib/python3.11/site-packages/lirc-setup/lirc-setup
Modern practises are to have a shim in /usr/bin/

 - /usr/bin/slapacl -> ../lib/slapd
 - /usr/bin/slapadd -> ../lib/slapd
 - moar slapd
Hmm what is openldap doing on this system again


In other words - I'd love it if we don't use relative symlinks if
there are other options.

Thanks,
Emil
Lucas De Marchi Feb. 5, 2024, 2:30 p.m. UTC | #8
On Mon, Feb 05, 2024 at 12:37:42PM +0000, Emil Velikov wrote:
>Hey Lucas,
>
>On Fri, 2 Feb 2024 at 18:53, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Fri, Jan 26, 2024 at 02:43:51PM +0000, Emil Velikov via B4 Relay wrote:
>> >From: Emil Velikov <emil.velikov@collabora.com>
>> >
>> >Currently we create symlinks like modprobe (pointing to kmod), during
>> >the normal `make` build. Although those were never installed.
>> >
>> >Add a few lines in the install-exec-hook, to ensure they're present at
>> >`make install` time. Thus one can actually use those without additional
>> >changes. As an added bonus, distributions can drop the similar hunk from
>> >their packaging.
>> >
>> >Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> >---
>> >Out of curiosity: are there any plans about releasing v32? I'm
>> >interested in the recent /usr/lib/modules (module_directory) patches.
>> >
>> >Thanks o/
>> >---
>> > Makefile.am | 10 ++++++++++
>> > 1 file changed, 10 insertions(+)
>> >
>> >diff --git a/Makefile.am b/Makefile.am
>> >index 4062d81..a22d1b1 100644
>> >--- a/Makefile.am
>> >+++ b/Makefile.am
>> >@@ -111,9 +111,19 @@ install-exec-hook:
>> >               ln -sf $$so_img_rel_target_prefix$(rootlibdir)/$$so_img_name $(DESTDIR)$(libdir)/libkmod.so && \
>> >               mv $(DESTDIR)$(libdir)/libkmod.so.* $(DESTDIR)$(rootlibdir); \
>> >       fi
>> >+if BUILD_TOOLS
>> >+      for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
>> >+              $(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
>>
>> I was about to apply this, but then noticed a problem: I think we should
>> use a relative symlink here.
>>
>> $ DESTDIR=/tmp/inst make install
>> $ ls -l /tmp/inst/usr/bin
>> total 700
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 depmod -> /usr/bin/kmod
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 insmod -> /usr/bin/kmod
>> -rwxr-xr-x 1 ldmartin ldmartin 715432 Feb  2 12:44 kmod
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 lsmod -> /usr/bin/kmod
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modinfo -> /usr/bin/kmod
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 modprobe -> /usr/bin/kmod
>> lrwxrwxrwx 1 ldmartin ldmartin     13 Feb  2 12:44 rmmod -> /usr/bin/kmod
>>
>> should had been e.g. depmod -> ./kmod
>>
>> Simplest fix without resorting to calculating the shortest symlink is to
>> assume: the symlinks should be in the same dir as kmod, just like if
>> they were not symlinks.
>>
>
>I'm not sure I follow, can you elaborate what is the issue?
>
>Are you trying to use/run files installed in DESTDIR directly? If so,
>that won't work for a few reasons:

no, those would usually be done by setting prefix and sysconfdir

> - kmod does not link to the in-DESTDIR libkmod.so, admittedly one can
>workaround it with LD_PRELOAD/LD_LIBRARY_PATH
> - kmod tries to open the depmod config files in the normal
>non-DESTDIR locations
>
>> diff --git a/Makefile.am b/Makefile.am
>> index 39a46f4..6df2f60 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -113,7 +113,7 @@ install-exec-hook:
>>          fi
>>   if BUILD_TOOLS
>>          for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
>> -               $(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
>> +               $(LN_S) ./kmod $(DESTDIR)$(bindir)/$$tool; \
>>          done
>>   endif
>>
>> does that seem ok squashed on your patch?
>>
>
>I'm not a huge fan of using relative symlinks, especially if the tool
>is run as root. In my experience that makes things harder to audit and
>prevent accidental breakages.

I'm completely in the opposite camp. Relative symlinks actually make
sure the thing you are running is what you are expecting. Nothing should
really point outside of $prefix expecting that is mounted on /.

Several years back there was also the issue with packaging, which would
complain when symlinks pointed outside what was being packaged.  It is
dangerous when using absolute symlinks because if the tool used to copy
follows the symlinks, it ends up with the wrong binary, copying the host
bin rather than what was just built.

Lucas De Marchi

>
>As an example, my Arch box has the following:
>
> - /usr/bin/init -> ../lib/systemd/systemd
> - /usr/bin/ld.so -> ../lib/ld-linux-x86-64.so.2
>Hmm should probably see if we can change these and how many things will break
>
> - /usr/bin/lirc-setup -> ../lib/python3.11/site-packages/lirc-setup/lirc-setup
>Modern practises are to have a shim in /usr/bin/
>
> - /usr/bin/slapacl -> ../lib/slapd
> - /usr/bin/slapadd -> ../lib/slapd
> - moar slapd
>Hmm what is openldap doing on this system again
>
>
>In other words - I'd love it if we don't use relative symlinks if
>there are other options.
>
>Thanks,
>Emil
Emil Velikov Feb. 5, 2024, 5:50 p.m. UTC | #9
On Mon, 5 Feb 2024 at 14:30, Lucas De Marchi <lucas.demarchi@intel.com> wrote:

[snip]

> >I'm not a huge fan of using relative symlinks, especially if the tool> >is run as root. In my experience that makes things harder to audit and
> >prevent accidental breakages.
>
> I'm completely in the opposite camp. Relative symlinks actually make
> sure the thing you are running is what you are expecting. Nothing should
> really point outside of $prefix expecting that is mounted on /.
>

That is true and I fully agree. Yet the contents of DESTDIR are not
meant to be run as-is - it's used for "staging" [1].

[1] https://www.gnu.org/software/make/manual/html_node/DESTDIR.html

> Several years back there was also the issue with packaging, which would
> complain when symlinks pointed outside what was being packaged.  It is
> dangerous when using absolute symlinks because if the tool used to copy
> follows the symlinks, it ends up with the wrong binary, copying the host
> bin rather than what was just built.
>

That sounds like a horrible bug, which can easily break your system
regardless of the project.

Would you consider dropping the leading `./` aka can we use `$(LN_S)
kmod $(DESTDIR)$(bindir)/$$tool;`?
Seems to be prevailing on my system with over 90% instances.

Thanks,
Emil
Lucas De Marchi Feb. 6, 2024, 4:15 p.m. UTC | #10
On Mon, Feb 05, 2024 at 05:50:54PM +0000, Emil Velikov wrote:
>On Mon, 5 Feb 2024 at 14:30, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>[snip]
>
>> >I'm not a huge fan of using relative symlinks, especially if the tool> >is run as root. In my experience that makes things harder to audit and
>> >prevent accidental breakages.
>>
>> I'm completely in the opposite camp. Relative symlinks actually make
>> sure the thing you are running is what you are expecting. Nothing should
>> really point outside of $prefix expecting that is mounted on /.
>>
>
>That is true and I fully agree. Yet the contents of DESTDIR are not
>meant to be run as-is - it's used for "staging" [1].
>
>[1] https://www.gnu.org/software/make/manual/html_node/DESTDIR.html
>
>> Several years back there was also the issue with packaging, which would
>> complain when symlinks pointed outside what was being packaged.  It is
>> dangerous when using absolute symlinks because if the tool used to copy
>> follows the symlinks, it ends up with the wrong binary, copying the host
>> bin rather than what was just built.
>>
>
>That sounds like a horrible bug, which can easily break your system
>regardless of the project.
>
>Would you consider dropping the leading `./` aka can we use `$(LN_S)
>kmod $(DESTDIR)$(bindir)/$$tool;`?
>Seems to be prevailing on my system with over 90% instances.

seems good to me. I will squash that and push.

thanks
Lucas De Marchi


>
>Thanks,
>Emil
Emil Velikov Feb. 14, 2024, 4:22 p.m. UTC | #11
On Tue, 6 Feb 2024 at 16:15, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 05:50:54PM +0000, Emil Velikov wrote:
> >On Mon, 5 Feb 2024 at 14:30, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >
> >[snip]
> >
> >> >I'm not a huge fan of using relative symlinks, especially if the tool> >is run as root. In my experience that makes things harder to audit and
> >> >prevent accidental breakages.
> >>
> >> I'm completely in the opposite camp. Relative symlinks actually make
> >> sure the thing you are running is what you are expecting. Nothing should
> >> really point outside of $prefix expecting that is mounted on /.
> >>
> >
> >That is true and I fully agree. Yet the contents of DESTDIR are not
> >meant to be run as-is - it's used for "staging" [1].
> >
> >[1] https://www.gnu.org/software/make/manual/html_node/DESTDIR.html
> >
> >> Several years back there was also the issue with packaging, which would
> >> complain when symlinks pointed outside what was being packaged.  It is
> >> dangerous when using absolute symlinks because if the tool used to copy
> >> follows the symlinks, it ends up with the wrong binary, copying the host
> >> bin rather than what was just built.
> >>
> >
> >That sounds like a horrible bug, which can easily break your system
> >regardless of the project.
> >
> >Would you consider dropping the leading `./` aka can we use `$(LN_S)
> >kmod $(DESTDIR)$(bindir)/$$tool;`?
> >Seems to be prevailing on my system with over 90% instances.
>
> seems good to me. I will squash that and push.
>

Respectful poke?

Thanks
Emil
Lucas De Marchi Feb. 20, 2024, 8:11 a.m. UTC | #12
On Wed, Feb 14, 2024 at 04:22:44PM +0000, Emil Velikov wrote:
>On Tue, 6 Feb 2024 at 16:15, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>
>> On Mon, Feb 05, 2024 at 05:50:54PM +0000, Emil Velikov wrote:
>> >On Mon, 5 Feb 2024 at 14:30, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >
>> >[snip]
>> >
>> >> >I'm not a huge fan of using relative symlinks, especially if the tool> >is run as root. In my experience that makes things harder to audit and
>> >> >prevent accidental breakages.
>> >>
>> >> I'm completely in the opposite camp. Relative symlinks actually make
>> >> sure the thing you are running is what you are expecting. Nothing should
>> >> really point outside of $prefix expecting that is mounted on /.
>> >>
>> >
>> >That is true and I fully agree. Yet the contents of DESTDIR are not
>> >meant to be run as-is - it's used for "staging" [1].
>> >
>> >[1] https://www.gnu.org/software/make/manual/html_node/DESTDIR.html
>> >
>> >> Several years back there was also the issue with packaging, which would
>> >> complain when symlinks pointed outside what was being packaged.  It is
>> >> dangerous when using absolute symlinks because if the tool used to copy
>> >> follows the symlinks, it ends up with the wrong binary, copying the host
>> >> bin rather than what was just built.
>> >>
>> >
>> >That sounds like a horrible bug, which can easily break your system
>> >regardless of the project.
>> >
>> >Would you consider dropping the leading `./` aka can we use `$(LN_S)
>> >kmod $(DESTDIR)$(bindir)/$$tool;`?
>> >Seems to be prevailing on my system with over 90% instances.
>>
>> seems good to me. I will squash that and push.

sorry I went on vacations and didn't realize I hadn't pushed.
Now it's in and with the other patches fixing make distcheck
we are clear for a release.  I will prep that this week.

thanks
Lucas De Marchi

>>
>
>Respectful poke?
>
>Thanks
>Emil
diff mbox series

Patch

diff --git a/Makefile.am b/Makefile.am
index 4062d81..a22d1b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -111,9 +111,19 @@  install-exec-hook:
 		ln -sf $$so_img_rel_target_prefix$(rootlibdir)/$$so_img_name $(DESTDIR)$(libdir)/libkmod.so && \
 		mv $(DESTDIR)$(libdir)/libkmod.so.* $(DESTDIR)$(rootlibdir); \
 	fi
+if BUILD_TOOLS
+	for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
+		$(LN_S) $(bindir)/kmod $(DESTDIR)$(bindir)/$$tool; \
+	done
+endif
 
 uninstall-hook:
 	rm -f $(DESTDIR)$(rootlibdir)/libkmod.so*
+if BUILD_TOOLS
+	for tool in insmod lsmod rmmod depmod modprobe modinfo; do \
+		rm -f $(DESTDIR)$(bindir)/$$tool; \
+	done
+endif
 
 if BUILD_TOOLS
 bin_PROGRAMS = tools/kmod