[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[David Kosir]

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Yann E. MORIN]

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 E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[David Kosir]

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

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[David Kosir]

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[David Kosir]

Yann,

I've sent first patch, for kmod dependency.

I've tried to make second patch better, but realized that it is more
complicated than I thought.
Best solution I came with to add --libdir=/lib to EUDEV_CONF_OPTS
I see that we already specify --libexecdir=/lib
That option would allow to use separate /usr, at least to start udev
normaly at boot, before /usr is mounted.
I think that is good option as it would not break anything (at least
what I see).
Do you have better idea?

--
David

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Thomas Petazzoni]

Hello David,

On Wed, 21 Oct 2015 13:50:17 +0200, David Kosir wrote:

> I've tried to make second patch better, but realized that it is more
> complicated than I thought.
> Best solution I came with to add --libdir=/lib to EUDEV_CONF_OPTS
> I see that we already specify --libexecdir=/lib
> That option would allow to use separate /usr, at least to start udev
> normaly at boot, before /usr is mounted.
> I think that is good option as it would not break anything (at least
> what I see).
> Do you have better idea?

While I understand what you are trying to do, I am not sure we can
support this in Buildroot upstream. You want eudev to be in / and
not /usr, but maybe the next person will want this other package to be
in / and not /usr. How do we handle this ? Add one more option to all
packages ? Doesn't seem really maintainable.

I don't really have a good suggestion on how to solve this problem, but
I'm pretty sure that adding an option to the eudev package is the right
solution.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Arnout Vandecappelle]

On 21-10-15 13:42, David Kosir wrote:
>>From aac359a0f280476ba9654e995dd4fd5565ffc22f Mon Sep 17 00:00:00 2001
> From: David Kosir <david.kosir@bylapis.com>
> Date: Wed, 21 Oct 2015 12:06:58 +0200
> Subject: [PATCH 1/1] Don't force kmod with eudev package

 Hi David,

 This patch looks good, but was not sent properly and did not appear in our
patch tracking system. Can you resubmit properly, using git send-email?

 Groeten,
 Arnout

> 
> Signed-off-by: David Kosir <david.kosir@bylapis.com>
> ---
>  package/eudev/Config.in |  1 -
>  package/eudev/eudev.mk  | 12 +++++++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/package/eudev/Config.in b/package/eudev/Config.in
> index 76df409..10f24c0 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.
> diff --git a/package/eudev/eudev.mk b/package/eudev/eudev.mk
> index a005f45..455ce25 100644
> --- a/package/eudev/eudev.mk
> +++ b/package/eudev/eudev.mk
> @@ -24,12 +24,18 @@ 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_PACKAGE_KMOD),y)
> +EUDEV_DEPENDENCIES += kmod
> +EUDEV_CONF_OPTS += --enable-libkmod
> +else
> +EUDEV_CONF_OPTS += --disable-libkmod
> +endif
> +
>  ifeq ($(BR2_ROOTFS_MERGED_USR),)
>  EUDEV_CONF_OPTS += --with-rootlibdir=/lib --enable-split-usr
>  endif
> 


-- 
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 1/1] Add more options to eudev package.

[Arnout Vandecappelle]

On 21-10-15 21:24, Thomas Petazzoni wrote:
> Hello David,
> 
> On Wed, 21 Oct 2015 13:50:17 +0200, David Kosir wrote:
> 
>> I've tried to make second patch better, but realized that it is more
>> complicated than I thought.
>> Best solution I came with to add --libdir=/lib to EUDEV_CONF_OPTS
>> I see that we already specify --libexecdir=/lib
>> That option would allow to use separate /usr, at least to start udev
>> normaly at boot, before /usr is mounted.
>> I think that is good option as it would not break anything (at least
>> what I see).
>> Do you have better idea?
> 
> While I understand what you are trying to do, I am not sure we can
> support this in Buildroot upstream. You want eudev to be in / and
> not /usr, but maybe the next person will want this other package to be
> in / and not /usr. How do we handle this ? Add one more option to all
> packages ? Doesn't seem really maintainable.

 Actually, it makes sense to me to install it completely in / instead of /usr -
that would also remove the need to have all the --libdir etc. options that we
currently pass. The way it's currently done, eudev's install anyway adds
symlinks back from /lib/libudev.so to /usr/lib/libudev.so.

 Of course, includes and pkgconfig still have to be put in /usr.


 Regards,
 Arnout

> 
> I don't really have a good suggestion on how to solve this problem, but
> I'm pretty sure that adding an option to the eudev package is the right
> solution.
> 
> Best regards,
> 
> Thomas
> 


-- 
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 1/1] Add more options to eudev package.

[Thomas Petazzoni]

Hello,

On Wed, 21 Oct 2015 22:00:35 +0200, Arnout Vandecappelle wrote:

> > While I understand what you are trying to do, I am not sure we can
> > support this in Buildroot upstream. You want eudev to be in / and
> > not /usr, but maybe the next person will want this other package to be
> > in / and not /usr. How do we handle this ? Add one more option to all
> > packages ? Doesn't seem really maintainable.
> 
>  Actually, it makes sense to me to install it completely in / instead of /usr -
> that would also remove the need to have all the --libdir etc. options that we
> currently pass. The way it's currently done, eudev's install anyway adds
> symlinks back from /lib/libudev.so to /usr/lib/libudev.so.

If we install it unconditionally in /, then it's fine with me. What I
would dislike is to start having options for such things.

However, there is still the question: why eudev in /, and not any other
package that other users may find important to have in / ? Where do we
put the boundary ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Arnout Vandecappelle]

On 21-10-15 22:18, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 21 Oct 2015 22:00:35 +0200, Arnout Vandecappelle wrote:
> 
>>> While I understand what you are trying to do, I am not sure we can
>>> support this in Buildroot upstream. You want eudev to be in / and
>>> not /usr, but maybe the next person will want this other package to be
>>> in / and not /usr. How do we handle this ? Add one more option to all
>>> packages ? Doesn't seem really maintainable.
>>
>>  Actually, it makes sense to me to install it completely in / instead of /usr -
>> that would also remove the need to have all the --libdir etc. options that we
>> currently pass. The way it's currently done, eudev's install anyway adds
>> symlinks back from /lib/libudev.so to /usr/lib/libudev.so.
> 
> If we install it unconditionally in /, then it's fine with me. What I
> would dislike is to start having options for such things.
> 
> However, there is still the question: why eudev in /, and not any other
> package that other users may find important to have in / ? Where do we
> put the boundary ?

 Because eudev is already installed mostly in / - as I said, even if libudev.so
is installed in /usr/lib, there's still a symlink created to point to it from
/lib. Same for udevadm: installed in /usr/bin but a symlink is created from
/bin. Basically, it's eudev itself that decides it should be in / in addition to
/usr.

 That said, I don't think David's use case is one that we should support,
because IMHO it's the wrong way to go at it. I think it's better to make a
separate full filesystem and either pivot_root or union-mount it. I think
depending on the /bin or /usr/bin location is a recipe for disaster.

 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 1/1] Add more options to eudev package.

[David Kosir]

>> However, there is still the question: why eudev in /, and not any other
>> package that other users may find important to have in / ? Where do we
>> put the boundary ?
>
>  Because eudev is already installed mostly in / - as I said, even if libudev.so
> is installed in /usr/lib, there's still a symlink created to point to it from
> /lib. Same for udevadm: installed in /usr/bin but a symlink is created from
> /bin. Basically, it's eudev itself that decides it should be in / in addition to
> /usr.

Exactly, you can check in src/libudevMakefile, install-exec-hook.

>  This patch looks good, but was not sent properly and did not appear in our
> patch tracking system. Can you resubmit properly, using git send-email?

I will try it. I just copy-pasted in into gmail as send-mail was
complaining about something regarding ssl. I guess that is why it
didn't work.

--
David

[Buildroot] [PATCH 1/1] Add more options to eudev package.

[Arnout Vandecappelle]

On 22-10-15 08:53, David Kosir wrote:
>>> However, there is still the question: why eudev in /, and not any other
>>> package that other users may find important to have in / ? Where do we
>>> put the boundary ?
>>
>>  Because eudev is already installed mostly in / - as I said, even if libudev.so
>> is installed in /usr/lib, there's still a symlink created to point to it from
>> /lib. Same for udevadm: installed in /usr/bin but a symlink is created from
>> /bin. Basically, it's eudev itself that decides it should be in / in addition to
>> /usr.
> 
> Exactly, you can check in src/libudevMakefile, install-exec-hook.
> 
>>  This patch looks good, but was not sent properly and did not appear in our
>> patch tracking system. Can you resubmit properly, using git send-email?
> 
> I will try it. I just copy-pasted in into gmail as send-mail was
> complaining about something regarding ssl. I guess that is why it
> didn't work.

'git help send-email' gives a very good explanation about how to configure gmail.

 Regards,
 Arnout

-- 
Arnout Vandecappelle      arnout dot vandecappelle at essensium dot com
Senior Embedded Software Architect . . . . . . +32-478-010353 (mobile)
Essensium, Mind division . . . . . . . . . . . . . . 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 1/1] Add more options to eudev package.

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com