| Message ID | 3cd1a050fe692425352745672295033c@talpidae.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Makefile: install modules.builtin even if CONFIG_MODULES=n | expand |
On Wed, Jun 3, 2020 at 10:34 PM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > Many applications check for available kernel features via: > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > - $(MODLIB)/modules.builtin (builtin modules) > > They fail to detect features if the kernel was built with > CONFIG_MODULES=n > and modules.builtin isn't installed. > > Therefore, add the target "_builtin_inst_" and make "install" and > "modules_install" depend on it. > > Tests results: > > - make install: kernel image is copied as before, modules.builtin > copied > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > --- Applied to linux-kbuild, but this patch format is broken. I manually fixed it up, but please use 'git send-email' next time. > Makefile | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index a7bc91cbac8f..a160efd62897 100644 > --- a/Makefile > +++ b/Makefile > @@ -1315,6 +1315,16 @@ dt_binding_check: scripts_dtc > # > --------------------------------------------------------------------------- > # Modules > > +# install modules.builtin regardless of CONFIG_MODULES > +PHONY += _builtin_inst_ > +_builtin_inst_: > + @mkdir -p $(MODLIB)/ > + @cp -f modules.builtin $(MODLIB)/ > + @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/ > + > +PHONY += install > +install: _builtin_inst_ > + > ifdef CONFIG_MODULES > > # By default, build modules as well > @@ -1344,7 +1354,7 @@ PHONY += modules_install > modules_install: _modinst_ _modinst_post > > PHONY += _modinst_ > -_modinst_: > +_modinst_: _builtin_inst_ > @rm -rf $(MODLIB)/kernel > @rm -f $(MODLIB)/source > @mkdir -p $(MODLIB)/kernel > @@ -1354,8 +1364,6 @@ _modinst_: > ln -s $(CURDIR) $(MODLIB)/build ; \ > fi > @sed 's:^:kernel/:' modules.order > $(MODLIB)/modules.order > - @cp -f modules.builtin $(MODLIB)/ > - @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/ > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst > > # This depmod is only for convenience to give the initial > -- > 2.26.2 >
Hi, On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > Many applications check for available kernel features via: > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > - $(MODLIB)/modules.builtin (builtin modules) > > They fail to detect features if the kernel was built with > CONFIG_MODULES=n > and modules.builtin isn't installed. > > Therefore, add the target "_builtin_inst_" and make "install" and > "modules_install" depend on it. > > Tests results: > > - make install: kernel image is copied as before, modules.builtin > copied > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > --- > Makefile | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) Note that this change broke builds in the Chrome OS build system because we require modules to be installed to a certain path and we weren't passing "INSTALL_MOD_PATH" when we called "make install". We can certainly fix our build system (I have a patch at https://crrev.com/c/2237511 for it), but I do wonder if others will hit the same issue. Others might not have such a nice sandboxing system so they might unknowingly try to install files to the build computer's modules directory instead of their target. -Doug
On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > > > Many applications check for available kernel features via: > > > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > > - $(MODLIB)/modules.builtin (builtin modules) > > > > They fail to detect features if the kernel was built with > > CONFIG_MODULES=n > > and modules.builtin isn't installed. > > > > Therefore, add the target "_builtin_inst_" and make "install" and > > "modules_install" depend on it. > > > > Tests results: > > > > - make install: kernel image is copied as before, modules.builtin > > copied > > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > > > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > > --- > > Makefile | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > Note that this change broke builds in the Chrome OS build system > because we require modules to be installed to a certain path and we > weren't passing "INSTALL_MOD_PATH" when we called "make install". > > We can certainly fix our build system (I have a patch at > https://crrev.com/c/2237511 for it), but I do wonder if others will > hit the same issue. Others might not have such a nice sandboxing > system so they might unknowingly try to install files to the build > computer's modules directory instead of their target. > I am more concerned with people getting errors such as mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied when running "make install", with no documentation or explanation that or why INSTALL_MOD_PATH is now mandatory for non-root installations. Even for root installations, it seems odd that "make install" now installs module files; after all, this is what "make modules_install" is for. I can understand the use case for CONFIG_MODULES=n, but the impact and changed behavior on systems with CONFIG_MODULES=y is quite unexpected. Guenter
On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> wrote: > > On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > > > > > Many applications check for available kernel features via: > > > > > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > > > - $(MODLIB)/modules.builtin (builtin modules) > > > > > > They fail to detect features if the kernel was built with > > > CONFIG_MODULES=n > > > and modules.builtin isn't installed. > > > > > > Therefore, add the target "_builtin_inst_" and make "install" and > > > "modules_install" depend on it. > > > > > > Tests results: > > > > > > - make install: kernel image is copied as before, modules.builtin > > > copied > > > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > > > > > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > > > --- > > > Makefile | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > Note that this change broke builds in the Chrome OS build system > > because we require modules to be installed to a certain path and we > > weren't passing "INSTALL_MOD_PATH" when we called "make install". > > > > We can certainly fix our build system (I have a patch at > > https://crrev.com/c/2237511 for it), but I do wonder if others will > > hit the same issue. Others might not have such a nice sandboxing > > system so they might unknowingly try to install files to the build > > computer's modules directory instead of their target. > > > > I am more concerned with people getting errors such as > > mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied > > when running "make install", with no documentation or explanation that > or why INSTALL_MOD_PATH is now mandatory for non-root installations. > Even for root installations, it seems odd that "make install" now > installs module files; after all, this is what "make modules_install" > is for. > > I can understand the use case for CONFIG_MODULES=n, but the impact and > changed behavior on systems with CONFIG_MODULES=y is quite unexpected. > > Guenter Sorry, I led this patch in a wrong way. Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n as Jonas did in v1. Another way might be to install it in /boot/modules.builtin.(ver) when CONFIG_MODULES=n but checking multiple locations would be inconvenient.
On Thu, Jun 11, 2020 at 11:57 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> wrote: > > > > On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > > > > > > > Many applications check for available kernel features via: > > > > > > > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > > > > - $(MODLIB)/modules.builtin (builtin modules) > > > > > > > > They fail to detect features if the kernel was built with > > > > CONFIG_MODULES=n > > > > and modules.builtin isn't installed. > > > > > > > > Therefore, add the target "_builtin_inst_" and make "install" and > > > > "modules_install" depend on it. > > > > > > > > Tests results: > > > > > > > > - make install: kernel image is copied as before, modules.builtin > > > > copied > > > > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > > > > > > > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > > > > --- > > > > Makefile | 14 +++++++++++--- > > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > Note that this change broke builds in the Chrome OS build system > > > because we require modules to be installed to a certain path and we > > > weren't passing "INSTALL_MOD_PATH" when we called "make install". > > > > > > We can certainly fix our build system (I have a patch at > > > https://crrev.com/c/2237511 for it), but I do wonder if others will > > > hit the same issue. Others might not have such a nice sandboxing > > > system so they might unknowingly try to install files to the build > > > computer's modules directory instead of their target. > > > > > > > I am more concerned with people getting errors such as > > > > mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission denied > > > > when running "make install", with no documentation or explanation that > > or why INSTALL_MOD_PATH is now mandatory for non-root installations. > > Even for root installations, it seems odd that "make install" now > > installs module files; after all, this is what "make modules_install" > > is for. > > > > I can understand the use case for CONFIG_MODULES=n, but the impact and > > changed behavior on systems with CONFIG_MODULES=y is quite unexpected. > > > > Guenter > > > Sorry, I led this patch in a wrong way. > > Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n > as Jonas did in v1. > Personally I think that would have been much better than the current solution. It would specifically give people the choice to install those files if they want to, without forcing others to do it (and to do it in an unrelated context/command). Thanks, Guenter > > Another way might be to install it > in /boot/modules.builtin.(ver) when CONFIG_MODULES=n > but checking multiple locations would be inconvenient. > > > > -- > Best Regards > Masahiro Yamada
On 2020-06-12 08:56, Masahiro Yamada wrote: > On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> > wrote: >> >> On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> >> wrote: >> > >> > Hi, >> > >> > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: >> > > >> > > Many applications check for available kernel features via: >> > > >> > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) >> > > - $(MODLIB)/modules.builtin (builtin modules) >> > > >> > > They fail to detect features if the kernel was built with >> > > CONFIG_MODULES=n >> > > and modules.builtin isn't installed. >> > > >> > > Therefore, add the target "_builtin_inst_" and make "install" and >> > > "modules_install" depend on it. >> > > >> > > Tests results: >> > > >> > > - make install: kernel image is copied as before, modules.builtin >> > > copied >> > > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 >> > > >> > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> >> > > --- >> > > Makefile | 14 +++++++++++--- >> > > 1 file changed, 11 insertions(+), 3 deletions(-) >> > >> > Note that this change broke builds in the Chrome OS build system >> > because we require modules to be installed to a certain path and we >> > weren't passing "INSTALL_MOD_PATH" when we called "make install". >> > >> > We can certainly fix our build system (I have a patch at >> > https://crrev.com/c/2237511 for it), but I do wonder if others will >> > hit the same issue. Others might not have such a nice sandboxing >> > system so they might unknowingly try to install files to the build >> > computer's modules directory instead of their target. >> > >> >> I am more concerned with people getting errors such as >> >> mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission >> denied >> >> when running "make install", with no documentation or explanation that >> or why INSTALL_MOD_PATH is now mandatory for non-root installations. >> Even for root installations, it seems odd that "make install" now >> installs module files; after all, this is what "make modules_install" >> is for. >> >> I can understand the use case for CONFIG_MODULES=n, but the impact and >> changed behavior on systems with CONFIG_MODULES=y is quite unexpected. >> >> Guenter > > > Sorry, I led this patch in a wrong way. > > Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n > as Jonas did in v1. > > > Another way might be to install it > in /boot/modules.builtin.(ver) when CONFIG_MODULES=n > but checking multiple locations would be inconvenient. I have noticed that my build system specified INSTALL_MOD_PATH for "make install", so the patch doesn't cause issues in my environment. However, I should have noticed that the change is breaking some existing setups. Masahiro, I still believe that the approach you favored (v2) makes more sense architecturally, but at this point it seems that v1 is more pragmatic. Would you agree to revert v2 and apply v1 instead? I will fix issues that may come up with v1, however unlikely.
On Sat, Jun 13, 2020 at 12:35 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > > On 2020-06-12 08:56, Masahiro Yamada wrote: > > On Wed, Jun 10, 2020 at 2:31 AM Guenter Roeck <groeck@google.com> > > wrote: > >> > >> On Tue, Jun 9, 2020 at 9:38 AM Doug Anderson <dianders@chromium.org> > >> wrote: > >> > > >> > Hi, > >> > > >> > On Tue, Jun 3, 2020 at 9:33 AM Jonas Zeiger <jonas.zeiger@talpidae.net> wrote: > >> > > > >> > > Many applications check for available kernel features via: > >> > > > >> > > - /proc/modules (loaded modules, present if CONFIG_MODULES=y) > >> > > - $(MODLIB)/modules.builtin (builtin modules) > >> > > > >> > > They fail to detect features if the kernel was built with > >> > > CONFIG_MODULES=n > >> > > and modules.builtin isn't installed. > >> > > > >> > > Therefore, add the target "_builtin_inst_" and make "install" and > >> > > "modules_install" depend on it. > >> > > > >> > > Tests results: > >> > > > >> > > - make install: kernel image is copied as before, modules.builtin > >> > > copied > >> > > - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 > >> > > > >> > > Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> > >> > > --- > >> > > Makefile | 14 +++++++++++--- > >> > > 1 file changed, 11 insertions(+), 3 deletions(-) > >> > > >> > Note that this change broke builds in the Chrome OS build system > >> > because we require modules to be installed to a certain path and we > >> > weren't passing "INSTALL_MOD_PATH" when we called "make install". > >> > > >> > We can certainly fix our build system (I have a patch at > >> > https://crrev.com/c/2237511 for it), but I do wonder if others will > >> > hit the same issue. Others might not have such a nice sandboxing > >> > system so they might unknowingly try to install files to the build > >> > computer's modules directory instead of their target. > >> > > >> > >> I am more concerned with people getting errors such as > >> > >> mkdir: cannot create directory '/lib/modules/5.7.0+/': Permission > >> denied > >> > >> when running "make install", with no documentation or explanation that > >> or why INSTALL_MOD_PATH is now mandatory for non-root installations. > >> Even for root installations, it seems odd that "make install" now > >> installs module files; after all, this is what "make modules_install" > >> is for. > >> > >> I can understand the use case for CONFIG_MODULES=n, but the impact and > >> changed behavior on systems with CONFIG_MODULES=y is quite unexpected. > >> > >> Guenter > > > > > > Sorry, I led this patch in a wrong way. > > > > Maybe, we should allow 'make modules_install' for CONFIG_MODULES=n > > as Jonas did in v1. > > > > > > Another way might be to install it > > in /boot/modules.builtin.(ver) when CONFIG_MODULES=n > > but checking multiple locations would be inconvenient. > > I have noticed that my build system specified INSTALL_MOD_PATH for "make > install", so the patch doesn't cause issues in my environment. > > However, I should have noticed that the change is breaking some existing > setups. > > Masahiro, I still believe that the approach you favored (v2) makes more > sense architecturally, but at this point it seems that v1 is more > pragmatic. > > Would you agree to revert v2 and apply v1 instead? Yes, I agree. Thanks. > I will fix issues that may come up with v1, however unlikely. >
diff --git a/Makefile b/Makefile index a7bc91cbac8f..a160efd62897 100644 --- a/Makefile +++ b/Makefile @@ -1315,6 +1315,16 @@ dt_binding_check: scripts_dtc # --------------------------------------------------------------------------- # Modules +# install modules.builtin regardless of CONFIG_MODULES +PHONY += _builtin_inst_ +_builtin_inst_: + @mkdir -p $(MODLIB)/ + @cp -f modules.builtin $(MODLIB)/ + @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/ + +PHONY += install +install: _builtin_inst_ + ifdef CONFIG_MODULES # By default, build modules as well @@ -1344,7 +1354,7 @@ PHONY += modules_install modules_install: _modinst_ _modinst_post PHONY += _modinst_ -_modinst_: +_modinst_: _builtin_inst_ @rm -rf $(MODLIB)/kernel @rm -f $(MODLIB)/source @mkdir -p $(MODLIB)/kernel @@ -1354,8 +1364,6 @@ _modinst_: ln -s $(CURDIR) $(MODLIB)/build ; \ fi @sed 's:^:kernel/:' modules.order > $(MODLIB)/modules.order - @cp -f modules.builtin $(MODLIB)/ - @cp -f $(objtree)/modules.builtin.modinfo $(MODLIB)/ $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst # This depmod is only for convenience to give the initial
Many applications check for available kernel features via: - /proc/modules (loaded modules, present if CONFIG_MODULES=y) - $(MODLIB)/modules.builtin (builtin modules) They fail to detect features if the kernel was built with CONFIG_MODULES=n and modules.builtin isn't installed. Therefore, add the target "_builtin_inst_" and make "install" and "modules_install" depend on it. Tests results: - make install: kernel image is copied as before, modules.builtin copied - make modules_install: (CONFIG_MODULES=n) nothing is copied, exit 1 Signed-off-by: Jonas Zeiger <jonas.zeiger@talpidae.net> --- Makefile | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)