diff mbox

[3/4] linux-headers: rename _AS_KERNEL to _FROM_KERNEL

Message ID 1461099155-30702-4-git-send-email-vivien.didelot@savoirfairelinux.com
State Superseded
Headers show

Commit Message

Vivien Didelot April 19, 2016, 8:52 p.m. UTC
BR2_KERNEL_HEADERS_AS_KERNEL is confusing when making the choice between
downloading the headers from an independent kernel source, and using the
headers found in the kernel built by Buildroot.

Rename it to BR2_KERNEL_HEADERS_FROM_KERNEL.

In the meantime, document this distinction between the two choices.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 package/linux-headers/Config.in.host   | 19 ++++++++++++-------
 package/linux-headers/linux-headers.mk |  8 ++++----
 2 files changed, 16 insertions(+), 11 deletions(-)

Comments

Thomas Petazzoni April 19, 2016, 9:13 p.m. UTC | #1
Hello,

On Tue, 19 Apr 2016 16:52:34 -0400, Vivien Didelot wrote:
> BR2_KERNEL_HEADERS_AS_KERNEL is confusing when making the choice between
> downloading the headers from an independent kernel source, and using the
> headers found in the kernel built by Buildroot.
> 
> Rename it to BR2_KERNEL_HEADERS_FROM_KERNEL.
> 
> In the meantime, document this distinction between the two choices.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

I tend to disagree here. The headers are not "from" the kernel, since
in practice, we download the kernel source code twice and extract it
twice. So the "as kernel" really seem better. It could be "like kernel"
as well, but "from kernel" seems wrong.

However, I'm all for improving the help texts to make this clearer.

Thomas
Vivien Didelot April 19, 2016, 10:12 p.m. UTC | #2
Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

>> BR2_KERNEL_HEADERS_AS_KERNEL is confusing when making the choice between
>> downloading the headers from an independent kernel source, and using the
>> headers found in the kernel built by Buildroot.
>> 
>> Rename it to BR2_KERNEL_HEADERS_FROM_KERNEL.
>> 
>> In the meantime, document this distinction between the two choices.
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>
> I tend to disagree here. The headers are not "from" the kernel, since
> in practice, we download the kernel source code twice and extract it
> twice. So the "as kernel" really seem better. It could be "like kernel"
> as well, but "from kernel" seems wrong.

So with BR2_KERNEL_HEADERS_AS_KERNEL, the kernel source tree to extract
the headers from is the same site as the Linux kernel source. With
!BR2_KERNEL_HEADERS_AS_KERNEL, the kernel source is downloaded from
kernel.org (or a mirror). Correct?

If yes, BR2_KERNEL_HEADERS_SAME_SOURCE or BR2_KERNEL_HEADERS_SAME_SITE
seem clearer to me.

> However, I'm all for improving the help texts to make this clearer.

Thanks,
Vivien
Yann E. MORIN July 1, 2016, 2:34 p.m. UTC | #3
Vivien, All,

On 2016-04-19 16:52 -0400, Vivien Didelot spake thusly:
> BR2_KERNEL_HEADERS_AS_KERNEL is confusing when making the choice between
> downloading the headers from an independent kernel source, and using the
> headers found in the kernel built by Buildroot.
> 
> Rename it to BR2_KERNEL_HEADERS_FROM_KERNEL.
> 
> In the meantime, document this distinction between the two choices.

We'v ediscussed this during the Buildroot summer-camp, and we've come to
the conclusion that:
  - the naming of the variable might indeed not be the best;
  - however, it's been part of a release now, and handling legacy for a
    symbol in a choice is not easy;
  - we can improve the prompt and the help text.

So, I'll take this and rework the help and prompts.

In the meantime, I've marked this patch as "changes request4ed" in
patchwork.

Thanks!

Regards,
Yann E. MORIN.

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  package/linux-headers/Config.in.host   | 19 ++++++++++++-------
>  package/linux-headers/linux-headers.mk |  8 ++++----
>  2 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/package/linux-headers/Config.in.host b/package/linux-headers/Config.in.host
> index 6d86113..baadb2e 100644
> --- a/package/linux-headers/Config.in.host
> +++ b/package/linux-headers/Config.in.host
> @@ -5,15 +5,20 @@ comment "Kernel Header Options"
>  
>  choice
>  	prompt "Kernel Headers"
> -	default BR2_KERNEL_HEADERS_AS_KERNEL if BR2_LINUX_KERNEL
> +	default BR2_KERNEL_HEADERS_FROM_KERNEL if BR2_LINUX_KERNEL
>  	default BR2_KERNEL_HEADERS_4_5
>  	help
> -	  Select the version of kernel header files you wish to use.
> -	  You must select the correct set of header files to match
> -	  the kernel you intend to use on your target system.
> +	  Select the source of kernel header files which matches the kernel
> +	  version you intend to use on your target system.
>  
> -	config BR2_KERNEL_HEADERS_AS_KERNEL
> -		bool "Same as kernel"
> +	  Select BR2_KERNEL_HEADERS_FROM_KERNEL to use the header files from
> +	  the kernel source you are building for your target.
> +
> +	  Select any other choices to download and use an independent kernel
> +	  version for its headers. Useful when you are not building the kernel.
> +
> +	config BR2_KERNEL_HEADERS_FROM_KERNEL
> +		bool "From the kernel source"
>  		depends on BR2_LINUX_KERNEL
>  
>  	config BR2_KERNEL_HEADERS_3_2
> @@ -91,7 +96,7 @@ config BR2_DEFAULT_KERNEL_VERSION
>  
>  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_FROM_KERNEL
>  	default BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_REALLY_OLD
>  	help
>  	  Set to the kernel headers series you manually set above.
> diff --git a/package/linux-headers/linux-headers.mk b/package/linux-headers/linux-headers.mk
> index 0900778..9afe5dc 100644
> --- a/package/linux-headers/linux-headers.mk
> +++ b/package/linux-headers/linux-headers.mk
> @@ -7,7 +7,7 @@
>  # This package is used to provide Linux kernel headers for the
>  # internal toolchain backend.
>  
> -ifeq ($(BR2_KERNEL_HEADERS_AS_KERNEL),y)
> +ifeq ($(BR2_KERNEL_HEADERS_FROM_KERNEL),y)
>  
>  LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_LINUX_KERNEL_VERSION))
>  
> @@ -71,7 +71,7 @@ endef
>  
>  LINUX_HEADERS_POST_PATCH_HOOKS += LINUX_HEADERS_APPLY_LOCAL_PATCHES
>  
> -else # ! BR2_KERNEL_HEADERS_AS_KERNEL
> +else # ! BR2_KERNEL_HEADERS_FROM_KERNEL
>  
>  LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS))
>  ifeq ($(findstring x2.6.,x$(LINUX_HEADERS_VERSION)),x2.6.)
> @@ -83,7 +83,7 @@ LINUX_HEADERS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/v4.x
>  endif
>  LINUX_HEADERS_SOURCE = linux-$(LINUX_HEADERS_VERSION).tar.xz
>  
> -endif # ! BR2_KERNEL_HEADERS_AS_KERNEL
> +endif # ! BR2_KERNEL_HEADERS_FROM_KERNEL
>  
>  LINUX_HEADERS_LICENSE = GPLv2
>  LINUX_HEADERS_LICENSE_FILES = COPYING
> @@ -125,7 +125,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_FROM_KERNEL),y)
>  define LINUX_HEADERS_CHECK_VERSION
>  	$(call check_kernel_headers_version,\
>  		$(STAGING_DIR),\
> -- 
> 2.8.0
>
Steve Calfee July 1, 2016, 5:51 p.m. UTC | #4
On Fri, Jul 1, 2016 at 7:34 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Vivien, All,
>
<snip>

>   - however, it's been part of a release now, and handling legacy for a
>     symbol in a choice is not easy;

I think breaking user's configurations by changing config names is
very serious and should be done for a very good reason, not just to
get a slightly clearer name.

Regards, Steve
Yann E. MORIN July 1, 2016, 5:56 p.m. UTC | #5
Steve, All,

On 2016-07-01 10:51 -0700, Steve Calfee spake thusly:
> On Fri, Jul 1, 2016 at 7:34 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > Vivien, All,
> >
> <snip>
> 
> >   - however, it's been part of a release now, and handling legacy for a
> >     symbol in a choice is not easy;
> 
> I think breaking user's configurations by changing config names is
> very serious and should be done for a very good reason, not just to
> get a slightly clearer name.

That's why we did not rename the option.

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/package/linux-headers/Config.in.host b/package/linux-headers/Config.in.host
index 6d86113..baadb2e 100644
--- a/package/linux-headers/Config.in.host
+++ b/package/linux-headers/Config.in.host
@@ -5,15 +5,20 @@  comment "Kernel Header Options"
 
 choice
 	prompt "Kernel Headers"
-	default BR2_KERNEL_HEADERS_AS_KERNEL if BR2_LINUX_KERNEL
+	default BR2_KERNEL_HEADERS_FROM_KERNEL if BR2_LINUX_KERNEL
 	default BR2_KERNEL_HEADERS_4_5
 	help
-	  Select the version of kernel header files you wish to use.
-	  You must select the correct set of header files to match
-	  the kernel you intend to use on your target system.
+	  Select the source of kernel header files which matches the kernel
+	  version you intend to use on your target system.
 
-	config BR2_KERNEL_HEADERS_AS_KERNEL
-		bool "Same as kernel"
+	  Select BR2_KERNEL_HEADERS_FROM_KERNEL to use the header files from
+	  the kernel source you are building for your target.
+
+	  Select any other choices to download and use an independent kernel
+	  version for its headers. Useful when you are not building the kernel.
+
+	config BR2_KERNEL_HEADERS_FROM_KERNEL
+		bool "From the kernel source"
 		depends on BR2_LINUX_KERNEL
 
 	config BR2_KERNEL_HEADERS_3_2
@@ -91,7 +96,7 @@  config BR2_DEFAULT_KERNEL_VERSION
 
 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_FROM_KERNEL
 	default BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_REALLY_OLD
 	help
 	  Set to the kernel headers series you manually set above.
diff --git a/package/linux-headers/linux-headers.mk b/package/linux-headers/linux-headers.mk
index 0900778..9afe5dc 100644
--- a/package/linux-headers/linux-headers.mk
+++ b/package/linux-headers/linux-headers.mk
@@ -7,7 +7,7 @@ 
 # This package is used to provide Linux kernel headers for the
 # internal toolchain backend.
 
-ifeq ($(BR2_KERNEL_HEADERS_AS_KERNEL),y)
+ifeq ($(BR2_KERNEL_HEADERS_FROM_KERNEL),y)
 
 LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_LINUX_KERNEL_VERSION))
 
@@ -71,7 +71,7 @@  endef
 
 LINUX_HEADERS_POST_PATCH_HOOKS += LINUX_HEADERS_APPLY_LOCAL_PATCHES
 
-else # ! BR2_KERNEL_HEADERS_AS_KERNEL
+else # ! BR2_KERNEL_HEADERS_FROM_KERNEL
 
 LINUX_HEADERS_VERSION = $(call qstrip,$(BR2_DEFAULT_KERNEL_HEADERS))
 ifeq ($(findstring x2.6.,x$(LINUX_HEADERS_VERSION)),x2.6.)
@@ -83,7 +83,7 @@  LINUX_HEADERS_SITE = $(BR2_KERNEL_MIRROR)/linux/kernel/v4.x
 endif
 LINUX_HEADERS_SOURCE = linux-$(LINUX_HEADERS_VERSION).tar.xz
 
-endif # ! BR2_KERNEL_HEADERS_AS_KERNEL
+endif # ! BR2_KERNEL_HEADERS_FROM_KERNEL
 
 LINUX_HEADERS_LICENSE = GPLv2
 LINUX_HEADERS_LICENSE_FILES = COPYING
@@ -125,7 +125,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_FROM_KERNEL),y)
 define LINUX_HEADERS_CHECK_VERSION
 	$(call check_kernel_headers_version,\
 		$(STAGING_DIR),\