| Message ID | CAOcYpt2ah7+P2ZJyzN+i0DjxE+x9LiDZPWwg6zqMxzi-x7BhKQ@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
David, All, On 2015-10-20 20:44 +0200, David Kosir spake thusly: > From de239fceb033d278ef70924bcea5c0a18b1f930d Mon Sep 17 00:00:00 2001 > From: David Kosir <david.kosir@bylapis.com> > Date: Tue, 20 Oct 2015 19:45:28 +0200 > Subject: [PATCH 1/1] Add more options to eudev package. > > Making possible to install only in /, avoiding /usr. > Also, make opinional to use kmod. Thanks for your patch. However, I have a few comments. First, your patch does two changes: - make kmod optional - change prefix to / or /usr Therefore, it should have been split in two patches, each doing exactly one separate semantic change. > Signed-off-by: David Kosir <david.kosir@bylapis.com> > --- > package/eudev/Config.in | 12 +++++++++++- > package/eudev/eudev.mk | 16 +++++++++++++--- > 2 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/package/eudev/Config.in b/package/eudev/Config.in > index 76df409..2474444 100644 > --- a/package/eudev/Config.in > +++ b/package/eudev/Config.in > @@ -7,7 +7,6 @@ config BR2_PACKAGE_EUDEV > select BR2_PACKAGE_HAS_UDEV > select BR2_PACKAGE_UTIL_LINUX > select BR2_PACKAGE_UTIL_LINUX_LIBBLKID > - select BR2_PACKAGE_KMOD Indeed, kmod seems to now be optional. > help > Userspace device daemon. This is a standalone version, > independent of systemd. It is a fork maintained by Gentoo. > @@ -22,6 +21,12 @@ if BR2_PACKAGE_EUDEV > config BR2_PACKAGE_PROVIDES_UDEV > default "eudev" > > +config BR2_PACKAGE_EUDEV_AVOID_USR > + bool "don't install to /usr" > + help > + Avoid installing to /usr, use (e)prefix=/ > + Useful when having separate /usr partition. We recently committed an option to merge / and /usr. How does your change compare with that option? See: http://git.buildroot.org/buildroot/commit/?id=c5bd8af65e50a51735eb112fed9cbe6337f14e06 > config BR2_PACKAGE_EUDEV_RULES_GEN > bool "enable rules generator" > help > @@ -33,6 +38,11 @@ config BR2_PACKAGE_EUDEV_ENABLE_HWDB > help > Enables hardware database installation to /etc/udev/hwdb.d > > +config BR2_PACKAGE_EUDEV_ENABLE_KMOD > + bool "enable kernel module support" > + select BR2_PACKAGE_KMOD > + help > + Enable loadable kernel modules support (kmod) In this case, what we usually do is: - we do not add such option above; - we enable the support if the corresponding package is enabled. Which in this case would translate to a simple change in the .mk : ifeq ($(BR2_PACKAGE_KMOD),y) EUDEV_DEPENDENCIES += kmod EUDEV_CONF_OPTS += --enable-libkmod else EUDEV_CONF_OPTS += --disable-libkmod endif And that's all (for kmod). Care to split the patch in two and resend that? Also, please explain a bit more the / vs. /usr thingy. In the meantime, I marked this patch as "cjhamges requested" in our patchwork. Thanks! :-) Regards, Yann E. MORIN.
Yann, First of all, thanks for comments. >> Userspace device daemon. This is a standalone version, >> independent of systemd. It is a fork maintained by Gentoo. >> @@ -22,6 +21,12 @@ if BR2_PACKAGE_EUDEV >> config BR2_PACKAGE_PROVIDES_UDEV >> default "eudev" >> >> +config BR2_PACKAGE_EUDEV_AVOID_USR >> + bool "don't install to /usr" >> + help >> + Avoid installing to /usr, use (e)prefix=/ >> + Useful when having separate /usr partition. > > We recently committed an option to merge / and /usr. How does your > change compare with that option? > > See: > http://git.buildroot.org/buildroot/commit/?id=c5bd8af65e50a51735eb112fed9cbe6337f14e06 My idea was a bit different but my explanation was poor, sorry. I have separate /usr partition which mounts from nfs server, so I need to avoid all /usr paths for base programs and libraries. Actually, I was using mdev before udev and busybox had option for not use /usr (called CONFIG_INSTALL_NO_USR) so my idea worked. I wanted to achieve that with udev too, so I needed to implement something like that. BTW, my patch from first email wasn't good, I made mistake and sent first version which didn't build. I will fix it. >> @@ -33,6 +38,11 @@ config BR2_PACKAGE_EUDEV_ENABLE_HWDB >> help >> Enables hardware database installation to /etc/udev/hwdb.d >> >> +config BR2_PACKAGE_EUDEV_ENABLE_KMOD >> + bool "enable kernel module support" >> + select BR2_PACKAGE_KMOD >> + help >> + Enable loadable kernel modules support (kmod) > > In this case, what we usually do is: > - we do not add such option above; > - we enable the support if the corresponding package is enabled. > > Which in this case would translate to a simple change in the .mk : > > ifeq ($(BR2_PACKAGE_KMOD),y) > EUDEV_DEPENDENCIES += kmod > EUDEV_CONF_OPTS += --enable-libkmod > else > EUDEV_CONF_OPTS += --disable-libkmod > endif > > And that's all (for kmod). I understand it, that approach makes sense. I'm curious if that will make problems because kmod would be unselected by default which is not expected from such common package. That was why I made it to request kmod explicitly. I don't have much experience so I tend to accept your solution, but I'm curious if this matters at all. > Care to split the patch in two and resend that? Sure, I will make it first thing tomorrow morning. > In the meantime, I marked this patch as "cjhamges requested" in our > patchwork. > > Thanks! :-) Thank you! David
Dear David Kosir, On Tue, 20 Oct 2015 20:44:25 +0200, David Kosir wrote: > From de239fceb033d278ef70924bcea5c0a18b1f930d Mon Sep 17 00:00:00 2001 > From: David Kosir <david.kosir@bylapis.com> > Date: Tue, 20 Oct 2015 19:45:28 +0200 > Subject: [PATCH 1/1] Add more options to eudev package. > > Making possible to install only in /, avoiding /usr. > Also, make opinional to use kmod. > > Signed-off-by: David Kosir <david.kosir@bylapis.com> > --- > package/eudev/Config.in | 12 +++++++++++- > package/eudev/eudev.mk | 16 +++++++++++++--- > 2 files changed, 24 insertions(+), 4 deletions(-) Your patch received a good number of comments, especially from Arnout. Could you take them into account and post an updated version of your patch? In the mean time, I'll mark your patch as "Changes Requested" in our patch tracking system, which means that if you don't send a new version, we'll forget about it. Thanks a lot! Thomas
diff --git a/package/eudev/Config.in b/package/eudev/Config.in index 76df409..2474444 100644 --- a/package/eudev/Config.in +++ b/package/eudev/Config.in @@ -7,7 +7,6 @@ config BR2_PACKAGE_EUDEV select BR2_PACKAGE_HAS_UDEV select BR2_PACKAGE_UTIL_LINUX select BR2_PACKAGE_UTIL_LINUX_LIBBLKID - select BR2_PACKAGE_KMOD help Userspace device daemon. This is a standalone version, independent of systemd. It is a fork maintained by Gentoo. @@ -22,6 +21,12 @@ if BR2_PACKAGE_EUDEV config BR2_PACKAGE_PROVIDES_UDEV default "eudev" +config BR2_PACKAGE_EUDEV_AVOID_USR + bool "don't install to /usr" + help + Avoid installing to /usr, use (e)prefix=/ + Useful when having separate /usr partition. + config BR2_PACKAGE_EUDEV_RULES_GEN bool "enable rules generator" help @@ -33,6 +38,11 @@ config BR2_PACKAGE_EUDEV_ENABLE_HWDB help Enables hardware database installation to /etc/udev/hwdb.d +config BR2_PACKAGE_EUDEV_ENABLE_KMOD + bool "enable kernel module support" + select BR2_PACKAGE_KMOD + help + Enable loadable kernel modules support (kmod) endif comment "eudev needs eudev /dev management" diff --git a/package/eudev/eudev.mk b/package/eudev/eudev.mk index a005f45..559a61e 100644 --- a/package/eudev/eudev.mk +++ b/package/eudev/eudev.mk @@ -24,16 +24,20 @@ EUDEV_CONF_OPTS = \ --sbindir=/sbin \ --libexecdir=/lib \ --with-firmware-path=/lib/firmware \ - --disable-introspection \ - --enable-libkmod + --disable-introspection -EUDEV_DEPENDENCIES = host-gperf host-pkgconf util-linux kmod +EUDEV_DEPENDENCIES = host-gperf host-pkgconf util-linux EUDEV_PROVIDES = udev ifeq ($(BR2_ROOTFS_MERGED_USR),) EUDEV_CONF_OPTS += --with-rootlibdir=/lib --enable-split-usr endif +ifeq ($(BR2_PACKAGE_EUDEV_AVOID_USR),y) +EUDEV_CONF_OPTS += --prefix=/ +EUDEV_CONF_OPTS += --eprefix=/ +endif + ifeq ($(BR2_PACKAGE_EUDEV_RULES_GEN),y) EUDEV_CONF_OPTS += --enable-rule_generator endif @@ -44,6 +48,12 @@ else EUDEV_CONF_OPTS += --disable-hwdb endif +ifeq ($(BR2_PACKAGE_EUDEV_ENABLE_KMOD),y) +EUDEV_DEPENDENCIES += kmod +else +EUDEV_CONF_OPTS += --disable-kmod +endif + ifeq ($(BR2_PACKAGE_LIBGLIB2),y) EUDEV_CONF_OPTS += --enable-gudev EUDEV_DEPENDENCIES += libglib2