| Message ID | 20220126090457.32015-1-mf@go-sys.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] package/network-manager: add optional nmcli support | expand |
Hello, +Yann on legacy handling (see below) On Wed, 26 Jan 2022 10:04:57 +0100 Michael Fischer <mf@go-sys.de> wrote: > When NetworkManager is built without the READLINE package the nmcli was not build. Well, this was kind of expected, and isn't really a correct explanation for this change. A better explanation I believe is more something like this: """ The network-manager package builds the nmcli utility when the readline package is enabled. However, this is not necessarily obvious to the user. Therefore, this commit adds an explicit option to enable the nmcli tool, which automatically selects readline. """ > > Signed-off-by: Michael Fischer <mf@go-sys.de> > +config BR2_PACKAGE_NETWORK_MANAGER_CLI > + bool "nmcli support" > + select BR2_PACKAGE_READLINE > + help > + This option enables support for NetworkManager Command Line Interface I think this line is too long, make sure to run "make check-package". But a bigger problem is the legacy handling. Indeed, before your patch, a configuration with BR2_PACKAGE_NETWORK_MANAGER=y and BR2_PACKAGE_READLINE=y gets nmcli. After your patch, such a configuration no longer has nmcli compiled, because BR2_PACKAGE_NETWORK_MANAGER_CLI is not enabled. Is this important to address, I don't know. Is it possible to address is by doing: config BR2_PACKAGE_NETWORK_MANAGER_CLI bool "nmcli support" default y if BR2_PACKAGE_READLINE select BR2_PACKAGE_READLINE I don't know if Kconfig is happy about such a construct. Yann? :-) Thomas
Thomas, Michael, All, On 2022-01-26 23:14 +0100, Thomas Petazzoni spake thusly: > On Wed, 26 Jan 2022 10:04:57 +0100 > Michael Fischer <mf@go-sys.de> wrote: [--SNIP--] > > +config BR2_PACKAGE_NETWORK_MANAGER_CLI > > + bool "nmcli support" > > + select BR2_PACKAGE_READLINE > > + help > > + This option enables support for NetworkManager Command Line Interface > > I think this line is too long, make sure to run "make check-package". > > But a bigger problem is the legacy handling. Indeed, before your patch, > a configuration with BR2_PACKAGE_NETWORK_MANAGER=y and > BR2_PACKAGE_READLINE=y gets nmcli. After your patch, such a > configuration no longer has nmcli compiled, because > BR2_PACKAGE_NETWORK_MANAGER_CLI is not enabled. I was also looking at that patch, and was wondering what we could do. I do not like efaulting symbols to 'y', even conditionally, and I would also expect people to regression-test their systems when they upgrade anyway. > Is this important to address, I don't know. > > Is it possible to address is by doing: > > config BR2_PACKAGE_NETWORK_MANAGER_CLI > bool "nmcli support" > default y if BR2_PACKAGE_READLINE > select BR2_PACKAGE_READLINE > > I don't know if Kconfig is happy about such a construct. Yann? :-) No it is not valid: you have both a backward and a forward dependency that drive the availability of the symbol: package/readline/Config.in:1:error: recursive dependency detected! package/readline/Config.in:1: symbol BR2_PACKAGE_READLINE is selected by BR2_PACKAGE_NETWORK_MANAGER_CLI package/network-manager/Config.in:36: symbol BR2_PACKAGE_NETWORK_MANAGER_CLI depends on BR2_PACKAGE_READLINE For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" TBH, I would just leave it like that... Regards, Yann E. MORIN.
diff --git a/package/network-manager/Config.in b/package/network-manager/Config.in index a48cb37b15..1a123a26e2 100644 --- a/package/network-manager/Config.in +++ b/package/network-manager/Config.in @@ -37,6 +37,12 @@ config BR2_PACKAGE_NETWORK_MANAGER_TUI help This option enables terminal based UI +config BR2_PACKAGE_NETWORK_MANAGER_CLI + bool "nmcli support" + select BR2_PACKAGE_READLINE + help + This option enables support for NetworkManager Command Line Interface + config BR2_PACKAGE_NETWORK_MANAGER_MODEM_MANAGER bool "modem-manager support" select BR2_PACKAGE_MODEM_MANAGER @@ -54,7 +60,6 @@ config BR2_PACKAGE_NETWORK_MANAGER_OVS select BR2_PACKAGE_JANSSON help This option enables support for OpenVSwitch - endif comment "NetworkManager needs udev /dev management and a glibc toolchain w/ headers >= 4.6, dynamic library, wchar, threads, gcc >= 4.9" diff --git a/package/network-manager/network-manager.mk b/package/network-manager/network-manager.mk index 974320fce0..1eb9233b44 100644 --- a/package/network-manager/network-manager.mk +++ b/package/network-manager/network-manager.mk @@ -135,7 +135,7 @@ else NETWORK_MANAGER_CONF_OPTS += --disable-polkit endif -ifeq ($(BR2_PACKAGE_READLINE),y) +ifeq ($(BR2_PACKAGE_NETWORK_MANAGER_CLI),y) NETWORK_MANAGER_DEPENDENCIES += readline NETWORK_MANAGER_CONF_OPTS += --with-nmcli else
When NetworkManager is built without the READLINE package the nmcli was not build. Signed-off-by: Michael Fischer <mf@go-sys.de> --- package/network-manager/Config.in | 7 ++++++- package/network-manager/network-manager.mk | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)