| Message ID | 20180821110646.5671-1-mark.corbin@embecosm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] Add support for building toolchains against custom kernel headers. | expand |
Hello, On Tue, 21 Aug 2018 12:06:46 +0100, Mark Corbin wrote: > Allows the selection of a manual version, custom tarball or custom > git repository for the toolchain kernel headers. This enables > toolchains to be built against custom kernel headers without having > to build a full kernel. > > Signed-off-by: Mark Corbin <mark.corbin@embecosm.com> Thanks for working on this! Looks good overall, but I have a few possible suggestions of improvement. First, the commit title should be prefixed with the affected package, so something like: linux-headers: add support for custom kernel headers or something like that. > config BR2_KERNEL_HEADERS_VERSION > bool "Manually specified Linux version" > + help > + This option allows you to use a specific official version from > + kernel.org, like 2.6.x, 2.6.x.y, 3.x.y, ... > + > + Note: you cannot use this option to select a _longterm_ 2.6 > + kernel, because these kernels are not located at the standard > + URL at kernel.org. Instead, select "Custom tarball" and > + specify the right URL directly. Adding this help text is good, but somewhat unrelated. Could you do that as a separate, preliminary commit ? > diff --git a/package/linux-headers/linux-headers.mk b/package/linux-headers/linux-headers.mk > index 954c6b7978..3c2509e249 100644 > --- a/package/linux-headers/linux-headers.mk > +++ b/package/linux-headers/linux-headers.mk > @@ -64,8 +64,18 @@ endef > > LINUX_HEADERS_POST_PATCH_HOOKS += LINUX_HEADERS_APPLY_LOCAL_PATCHES > > -else # ! BR2_KERNEL_HEADERS_AS_KERNEL > - > +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_TARBALL),y) > +LINUX_HEADERS_TARBALL = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION)) > +LINUX_HEADERS_SITE = $(patsubst %/,%,$(dir $(LINUX_HEADERS_TARBALL))) > +LINUX_HEADERS_SOURCE = $(notdir $(LINUX_HEADERS_TARBALL)) > +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) > +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_GIT),y) > +LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) > +LINUX_HEADERS_SITE = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_REPO_URL)) > +LINUX_HEADERS_SITE_METHOD = git > +LINUX_HEADERS_SOURCE = linux-$(LINUX_HEADERS_VERSION).tar.gz > +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) > +else > LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) > ifeq ($(findstring x2.6.,x$(LINUX_HEADERS_VERSION)),x2.6.) > LINUX_HEADERS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/v2.6 > @@ -80,7 +90,7 @@ ifeq ($(BR2_KERNEL_HEADERS_VERSION),y) > BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) > endif First, I'm not sure why we bother updating the BR_NO_CHECK_HASH_FOR variable. I know we already do it, but linux-headers doesn't have a .hash file, so I don't see the point in doing this. Arnout, you reworked this in commit 24f650aed2d9d92d8cabf0cb160fcf7964f9811e, why didn't you just remove the BR_NO_CHECK_HASH_FOR ? Second, with your change there's quite a bit of duplication between the code to handle the "same headers as kernel" case and the "custom version" stuff you've added. Could you try something along the lines of: ifeq ($(BR2_KERNEL_HEADERS_AS_KERNEL),y) LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR = BR2_LINUX_KERNEL_CUSTOM_TARBALL_LOCATION LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR = BR2_LINUX_KERNEL_CUSTOM_REPO_URL else LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR = BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR = BR2_KERNEL_HEADERS_CUSTOM_REPO_URL endif and then use $($(LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR)) and $($(LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR)) to avoid duplicating all the logic testing if we have a tarball, or a Git repo, or, etc. Note that I haven't tried to refactor the code myself, perhaps there are some obstacles to achieve this, perhaps it makes the code uglier, but I think it is worth trying. Also, a few comments after the key "else" and "endif" would help figure out where each condition starts/stops, at least for the large conditions, where it's hard to know when they start/stop. Thanks! Thomas
Hello Thomas On 21/08/18 13:05, Thomas Petazzoni wrote: > Hello, > > On Tue, 21 Aug 2018 12:06:46 +0100, Mark Corbin wrote: >> Allows the selection of a manual version, custom tarball or custom >> git repository for the toolchain kernel headers. This enables >> toolchains to be built against custom kernel headers without having >> to build a full kernel. >> >> Signed-off-by: Mark Corbin <mark.corbin@embecosm.com> > Thanks for working on this! Looks good overall, but I have a few > possible suggestions of improvement. > > First, the commit title should be prefixed with the affected package, > so something like: > > linux-headers: add support for custom kernel headers > > or something like that. I'll add 'package/linux-headers:' as per other patches. > >> config BR2_KERNEL_HEADERS_VERSION >> bool "Manually specified Linux version" >> + help >> + This option allows you to use a specific official version from >> + kernel.org, like 2.6.x, 2.6.x.y, 3.x.y, ... >> + >> + Note: you cannot use this option to select a _longterm_ 2.6 >> + kernel, because these kernels are not located at the standard >> + URL at kernel.org. Instead, select "Custom tarball" and >> + specify the right URL directly. > Adding this help text is good, but somewhat unrelated. Could you do > that as a separate, preliminary commit ? Do you mean just generate a separate patch? > >> diff --git a/package/linux-headers/linux-headers.mk b/package/linux-headers/linux-headers.mk >> index 954c6b7978..3c2509e249 100644 >> --- a/package/linux-headers/linux-headers.mk >> +++ b/package/linux-headers/linux-headers.mk >> @@ -64,8 +64,18 @@ endef >> >> LINUX_HEADERS_POST_PATCH_HOOKS += LINUX_HEADERS_APPLY_LOCAL_PATCHES >> >> -else # ! BR2_KERNEL_HEADERS_AS_KERNEL >> - >> +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_TARBALL),y) >> +LINUX_HEADERS_TARBALL = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION)) >> +LINUX_HEADERS_SITE = $(patsubst %/,%,$(dir $(LINUX_HEADERS_TARBALL))) >> +LINUX_HEADERS_SOURCE = $(notdir $(LINUX_HEADERS_TARBALL)) >> +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) >> +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_GIT),y) >> +LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) >> +LINUX_HEADERS_SITE = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_REPO_URL)) >> +LINUX_HEADERS_SITE_METHOD = git >> +LINUX_HEADERS_SOURCE = linux-$(LINUX_HEADERS_VERSION).tar.gz >> +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) >> +else >> LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) >> ifeq ($(findstring x2.6.,x$(LINUX_HEADERS_VERSION)),x2.6.) >> LINUX_HEADERS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/v2.6 >> @@ -80,7 +90,7 @@ ifeq ($(BR2_KERNEL_HEADERS_VERSION),y) >> BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) >> endif > First, I'm not sure why we bother updating the BR_NO_CHECK_HASH_FOR > variable. I know we already do it, but linux-headers doesn't have > a .hash file, so I don't see the point in doing this. Arnout, you > reworked this in commit 24f650aed2d9d92d8cabf0cb160fcf7964f9811e, why > didn't you just remove the BR_NO_CHECK_HASH_FOR ? I did wonder about this - I'll plan to remove it unless anybody objects. > > Second, with your change there's quite a bit of duplication between the > code to handle the "same headers as kernel" case and the "custom > version" stuff you've added. > > Could you try something along the lines of: > > ifeq ($(BR2_KERNEL_HEADERS_AS_KERNEL),y) > LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR = BR2_LINUX_KERNEL_CUSTOM_TARBALL_LOCATION > LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR = BR2_LINUX_KERNEL_CUSTOM_REPO_URL > else > LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR = BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION > LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR = BR2_KERNEL_HEADERS_CUSTOM_REPO_URL > endif > > and then use $($(LINUX_HEADERS_CUSTOM_TARBALL_LOCATION_KCONFIG_VAR)) > and $($(LINUX_HEADERS_CUSTOM_REPO_URL_KCONFIG_VAR)) to avoid > duplicating all the logic testing if we have a tarball, or a Git repo, > or, etc. > > Note that I haven't tried to refactor the code myself, perhaps there > are some obstacles to achieve this, perhaps it makes the code uglier, > but I think it is worth trying. I'll take a look at this. I was trying to keep the changes to a minimum, but a re-factor makes sense. > > Also, a few comments after the key "else" and "endif" would help figure > out where each condition starts/stops, at least for the large > conditions, where it's hard to know when they start/stop. Will do. Thanks for the great feedback. > > Thanks! > > Thomas
Mark, Thomas, All, On 2018-08-21 13:51 +0100, Mark Corbin spake thusly: > On 21/08/18 13:05, Thomas Petazzoni wrote: > > On Tue, 21 Aug 2018 12:06:46 +0100, Mark Corbin wrote: > >> Allows the selection of a manual version, custom tarball or custom > >> git repository for the toolchain kernel headers. This enables > >> toolchains to be built against custom kernel headers without having > >> to build a full kernel. [--SNIP--] > >> config BR2_KERNEL_HEADERS_VERSION > >> bool "Manually specified Linux version" > >> + help > >> + This option allows you to use a specific official version from > >> + kernel.org, like 2.6.x, 2.6.x.y, 3.x.y, ... > >> + > >> + Note: you cannot use this option to select a _longterm_ 2.6 > >> + kernel, because these kernels are not located at the standard > >> + URL at kernel.org. Instead, select "Custom tarball" and > >> + specify the right URL directly. > > Adding this help text is good, but somewhat unrelated. Could you do > > that as a separate, preliminary commit ? > Do you mean just generate a separate patch? Yes, a separate, first patch that adds this help text, and only that. [--SNIP--] > >> +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) [--SNIP--] > > First, I'm not sure why we bother updating the BR_NO_CHECK_HASH_FOR > > variable. I know we already do it, but linux-headers doesn't have > > a .hash file, so I don't see the point in doing this. Arnout, you > > reworked this in commit 24f650aed2d9d92d8cabf0cb160fcf7964f9811e, why > > didn't you just remove the BR_NO_CHECK_HASH_FOR ? > I did wonder about this - I'll plan to remove it unless anybody objects. No, I'd prefer if we were to actualyl add a .hash file for linux-headers and for linux as well, btw, with the former being a symlink to the latter. So, when you respin, prepare a series that: 1. adds the help text alone, 2. adds a .hash for linux and a symlink to it from linux-headers 3. refactors of the variables 4. adds the custom locations thanks! :-) Regards, Yann E. MORIN.
Hello Yann On 21/08/18 14:11, Yann E. MORIN wrote: > Mark, Thomas, All, > > On 2018-08-21 13:51 +0100, Mark Corbin spake thusly: >> On 21/08/18 13:05, Thomas Petazzoni wrote: >>> On Tue, 21 Aug 2018 12:06:46 +0100, Mark Corbin wrote: >>>> Allows the selection of a manual version, custom tarball or custom >>>> git repository for the toolchain kernel headers. This enables >>>> toolchains to be built against custom kernel headers without having >>>> to build a full kernel. > [--SNIP--] >>>> config BR2_KERNEL_HEADERS_VERSION >>>> bool "Manually specified Linux version" >>>> + help >>>> + This option allows you to use a specific official version from >>>> + kernel.org, like 2.6.x, 2.6.x.y, 3.x.y, ... >>>> + >>>> + Note: you cannot use this option to select a _longterm_ 2.6 >>>> + kernel, because these kernels are not located at the standard >>>> + URL at kernel.org. Instead, select "Custom tarball" and >>>> + specify the right URL directly. >>> Adding this help text is good, but somewhat unrelated. Could you do >>> that as a separate, preliminary commit ? >> Do you mean just generate a separate patch? > Yes, a separate, first patch that adds this help text, and only that. > > [--SNIP--] >>>> +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) > [--SNIP--] >>> First, I'm not sure why we bother updating the BR_NO_CHECK_HASH_FOR >>> variable. I know we already do it, but linux-headers doesn't have >>> a .hash file, so I don't see the point in doing this. Arnout, you >>> reworked this in commit 24f650aed2d9d92d8cabf0cb160fcf7964f9811e, why >>> didn't you just remove the BR_NO_CHECK_HASH_FOR ? >> I did wonder about this - I'll plan to remove it unless anybody objects. > No, I'd prefer if we were to actualyl add a .hash file for linux-headers > and for linux as well, btw, with the former being a symlink to the > latter. > > So, when you respin, prepare a series that: > > 1. adds the help text alone, > 2. adds a .hash for linux and a symlink to it from linux-headers Does this need populating with just the hashes for the current 'default' versions, i.e. 4.17.17, 4.16.18, 4.15.18, 4.14.65,...etc. or all possible versions (!!!) from kernel.org? I presume that custom versions will remain exempt via 'BR_NO_CHECK_HASH_FOR' as originally intended? Thanks Mark > 3. refactors of the variables > 4. adds the custom locations > > thanks! :-) > > Regards, > Yann E. MORIN. >
Mark, All, On 2018-08-21 14:28 +0100, Mark Corbin spake thusly: > On 21/08/18 14:11, Yann E. MORIN wrote: > > No, I'd prefer if we were to actualyl add a .hash file for linux-headers > > and for linux as well, btw, with the former being a symlink to the > > latter. > > > > So, when you respin, prepare a series that: > > > > 1. adds the help text alone, > > 2. adds a .hash for linux and a symlink to it from linux-headers > Does this need populating with just the hashes for the current 'default' > versions, i.e. 4.17.17, 4.16.18, 4.15.18, 4.14.65,...etc. or all > possible versions (!!!) from kernel.org? We indeed only want the ones we explicitly refer to from Config.in. > I presume that custom versions will remain exempt via > 'BR_NO_CHECK_HASH_FOR' as originally intended? Exactly, yes. Regards, Yann E. MORIN.
On 21/08/2018 15:11, Yann E. MORIN wrote: > Mark, Thomas, All, > > On 2018-08-21 13:51 +0100, Mark Corbin spake thusly: >> On 21/08/18 13:05, Thomas Petazzoni wrote: [snip] >>> First, I'm not sure why we bother updating the BR_NO_CHECK_HASH_FOR >>> variable. I know we already do it, but linux-headers doesn't have >>> a .hash file, so I don't see the point in doing this. Arnout, you >>> reworked this in commit 24f650aed2d9d92d8cabf0cb160fcf7964f9811e, why >>> didn't you just remove the BR_NO_CHECK_HASH_FOR ? >> I did wonder about this - I'll plan to remove it unless anybody objects. > > No, I'd prefer if we were to actualyl add a .hash file for linux-headers > and for linux as well, btw, with the former being a symlink to the > latter. I think that was actually the reason that I didn't remove it: the idea was to add a hash file for linux-headers. Regards, Arnout > So, when you respin, prepare a series that: > > 1. adds the help text alone, > 2. adds a .hash for linux and a symlink to it from linux-headers > 3. refactors of the variables > 4. adds the custom locations > > thanks! :-) > > Regards, > Yann E. MORIN. >
diff --git a/package/linux-headers/Config.in.host b/package/linux-headers/Config.in.host index 783cc7ea3a..df2e2263c4 100644 --- a/package/linux-headers/Config.in.host +++ b/package/linux-headers/Config.in.host @@ -77,6 +77,31 @@ config BR2_KERNEL_HEADERS_4_17 config BR2_KERNEL_HEADERS_VERSION bool "Manually specified Linux version" + help + This option allows you to use a specific official version from + kernel.org, like 2.6.x, 2.6.x.y, 3.x.y, ... + + Note: you cannot use this option to select a _longterm_ 2.6 + kernel, because these kernels are not located at the standard + URL at kernel.org. Instead, select "Custom tarball" and + specify the right URL directly. + +config BR2_KERNEL_HEADERS_CUSTOM_TARBALL + bool "Custom tarball" + help + This option allows you to specify a URL pointing to a kernel + source tarball. This URL can use any protocol recognized by + Buildroot, like http://, ftp://, file:// or scp://. + + When pointing to a local tarball using file://, you may want + to use a make variable like $(TOPDIR) to reference the root of + the Buildroot tree. + +config BR2_KERNEL_HEADERS_CUSTOM_GIT + bool "Custom Git repository" + help + This option allows Buildroot to get the Linux kernel source + code from a Git repository. endchoice @@ -87,9 +112,27 @@ config BR2_DEFAULT_KERNEL_VERSION Specify the version you want to use. E.G.: 3.6.10 +config BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION + string "URL of custom kernel tarball" + depends on BR2_KERNEL_HEADERS_CUSTOM_TARBALL + +if BR2_KERNEL_HEADERS_CUSTOM_GIT + +config BR2_KERNEL_HEADERS_CUSTOM_REPO_URL + string "URL of custom repository" + +config BR2_KERNEL_HEADERS_CUSTOM_REPO_VERSION + string "Custom repository version" + help + Revision to use in the typical format used by + Git/Mercurial/Subversion E.G. a sha id, a tag, branch, .. + +endif + choice bool "Custom kernel headers series" - depends on BR2_KERNEL_HEADERS_VERSION || BR2_KERNEL_HEADERS_AS_KERNEL + depends on BR2_KERNEL_HEADERS_VERSION || BR2_KERNEL_HEADERS_AS_KERNEL || \ + BR2_KERNEL_HEADERS_CUSTOM_TARBALL || BR2_KERNEL_HEADERS_CUSTOM_GIT help Specify the kernel headers series you manually selected, above. @@ -269,3 +312,6 @@ config BR2_DEFAULT_KERNEL_HEADERS default "4.16.18" if BR2_KERNEL_HEADERS_4_16 default "4.17.17" if BR2_KERNEL_HEADERS_4_17 default BR2_DEFAULT_KERNEL_VERSION if BR2_KERNEL_HEADERS_VERSION + default "custom" if BR2_KERNEL_HEADERS_CUSTOM_TARBALL + default BR2_KERNEL_HEADERS_CUSTOM_REPO_VERSION \ + if BR2_KERNEL_HEADERS_CUSTOM_GIT diff --git a/package/linux-headers/linux-headers.mk b/package/linux-headers/linux-headers.mk index 954c6b7978..3c2509e249 100644 --- a/package/linux-headers/linux-headers.mk +++ b/package/linux-headers/linux-headers.mk @@ -64,8 +64,18 @@ endef LINUX_HEADERS_POST_PATCH_HOOKS += LINUX_HEADERS_APPLY_LOCAL_PATCHES -else # ! BR2_KERNEL_HEADERS_AS_KERNEL - +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_TARBALL),y) +LINUX_HEADERS_TARBALL = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_TARBALL_LOCATION)) +LINUX_HEADERS_SITE = $(patsubst %/,%,$(dir $(LINUX_HEADERS_TARBALL))) +LINUX_HEADERS_SOURCE = $(notdir $(LINUX_HEADERS_TARBALL)) +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) +else ifeq ($(BR2_KERNEL_HEADERS_CUSTOM_GIT),y) +LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) +LINUX_HEADERS_SITE = $(call qstrip,$(BR2_KERNEL_HEADERS_CUSTOM_REPO_URL)) +LINUX_HEADERS_SITE_METHOD = git +LINUX_HEADERS_SOURCE = linux-$(LINUX_HEADERS_VERSION).tar.gz +BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) +else LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS)) ifeq ($(findstring x2.6.,x$(LINUX_HEADERS_VERSION)),x2.6.) LINUX_HEADERS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/v2.6 @@ -80,7 +90,7 @@ ifeq ($(BR2_KERNEL_HEADERS_VERSION),y) BR_NO_CHECK_HASH_FOR += $(LINUX_HEADERS_SOURCE) endif -endif # ! BR2_KERNEL_HEADERS_AS_KERNEL +endif # linux-headers really is the same as the linux package LINUX_HEADERS_DL_SUBDIR = linux @@ -125,7 +135,7 @@ define LINUX_HEADERS_INSTALL_STAGING_CMDS headers_install) endef -ifeq ($(BR2_KERNEL_HEADERS_VERSION)$(BR2_KERNEL_HEADERS_AS_KERNEL),y) +ifeq ($(BR2_KERNEL_HEADERS_VERSION)$(BR2_KERNEL_HEADERS_AS_KERNEL)$(BR2_KERNEL_HEADERS_CUSTOM_TARBALL)$(BR2_KERNEL_HEADERS_CUSTOM_GIT),y) define LINUX_HEADERS_CHECK_VERSION $(call check_kernel_headers_version,\ $(STAGING_DIR),\
Allows the selection of a manual version, custom tarball or custom git repository for the toolchain kernel headers. This enables toolchains to be built against custom kernel headers without having to build a full kernel. Signed-off-by: Mark Corbin <mark.corbin@embecosm.com> --- package/linux-headers/Config.in.host | 48 +++++++++++++++++++++++++- package/linux-headers/linux-headers.mk | 18 +++++++--- 2 files changed, 61 insertions(+), 5 deletions(-)