Re: [PATCH v7 03/28] build: build Kconfig and config rules

[Doug Goldstein <cardoe@cardoe.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4610 bytes --]

On 12/14/15 10:35 AM, Jan Beulich wrote:
>>>> On 10.12.15 at 17:48, <cardoe@cardoe.com> wrote:
>> --- /dev/null
>> +++ b/xen/Kconfig
>> @@ -0,0 +1,24 @@
>> +#
>> +# For a description of the syntax of this configuration file,
>> +# see docs/misc/kconfig-language.txt
>> +#
>> +mainmenu "Xen/$SRCARCH $XEN_FULLVERSION Configuration"
>> +
>> +config SRCARCH
>> +	string
>> +	option env="SRCARCH"
>> +
>> +config ARCH
>> +	string
>> +	option env="ARCH"
>> +
>> +source "arch/$SRCARCH/Kconfig"
>> +
>> +config XEN_FULLVERSION
> 
> I continue to wonder what use the XEN_ prefix here is.

This can be named anything you want. The existing makefiles call the
version "XEN_FULLVERSION" so I'm just keeping it the same. Its just to
have the same version number that the makefiles generate stored in the
.config file for if/when people start posting .config files somewhere.
Honestly at this point I'll paint the bike whatever color is desired.

> 
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -20,6 +20,14 @@ MAKEFLAGS += -rR
>>  
>>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>>  
>> +# Don't break if the build process wasn't called from the top level
>> +# we need XEN_TARGET_ARCH to generate the proper config
>> +include $(XEN_ROOT)/Config.mk
>> +
>> +# Allow someone to change their config file
>> +KCONFIG_CONFIG ?= .config
>> +export KCONFIG_CONFIG
> 
> For compactness' sake this could be done on one line afaik.

Sure. I copied and pasted this from the Linux kernel where its two
lines. I'll send another squash in.

> 
>> --- /dev/null
>> +++ b/xen/arch/x86/Kconfig
>> @@ -0,0 +1,17 @@
>> +config X86_64
>> +	def_bool y
>> +
>> +config X86
>> +	def_bool y
>> +
>> +config ARCH_DEFCONFIG
>> +	string
>> +	default "arch/x86/configs/x86_64_defconfig"
>> +
>> +menu "Architecture Features"
>> +
>> +endmenu
>> +
>> +source "common/Kconfig"
>> +
>> +source "drivers/Kconfig"
> 
> I'm still missing "config 64BIT" in this file.

You had wanted me to factor that out in earlier series. So now its only
in the arm side which is the only place its used.

> 
>> --- /dev/null
>> +++ b/xen/tools/kconfig/Makefile.kconfig
>> @@ -0,0 +1,76 @@
>> +# xen/tools/kconfig
>> +
>> +# default rule to do nothing
>> +all:
>> +
>> +XEN_ROOT = $(CURDIR)/..

This was meant to go away once the xen/Makefile exported it. Which has
already been merged in.

>> +
>> +# Xen doesn't have a silent build flag
>> +quiet := silent_
>> +Q := @
>> +kecho := :
>> +
>> +# eventually you'll want to do out of tree builds
>> +srctree = $(XEN_ROOT)/xen
> 
> Doesn't this add one /xen too many? Or wait, don't you need
> two more /.. when setting XEN_ROOT above?

See above comment on XEN_ROOT.

> 
>> +objtree = $(srctree)
> 
> Do both of them really need to use = (instead of the more efficient
> ":=")?

Sure. I will send you a squash.

> 
>> +src := tools/kconfig
>> +obj := $(src)
>> +KBUILD_SRC :=
>> +
>> +# handle functions (most of these lifted from different Linux makefiles
>> +dot-target = $(dir $@).$(notdir $@)
>> +depfile = $(subst $(comma),,$(dot-target).d)
>> +basetarget = $(basename $(notdir $@))
>> +cmd = $(cmd_$(1))
>> +if_changed = $(if y, \
>> +	@set -e; \
>> +	$(cmd_$(1)); \
>> +	)
>> +
>> +if_changed_dep = $(if y, \
>> +	@set -e;	\
>> +	$(cmd_$(1)); \
>> +	)
> 
> I'm quite sure to have asked for these strange multi line constructs
> to be collapsed.

Sure. In the squash.

> 
>> +# provide the host compiler
>> +HOSTCC := gcc
>> +HOSTCXX := g++
> 
> Didn't you mean to inherit these instead of forcing them here?

No. I pointed out that the only real way to inherit them is to pull in
top-level Config.mk which is too heavy handed. If we have some agreement
to break up the pieces that truly belong in there then sure.

Honestly I would prefer to do that break up after the fact. I've being
working on this series for 3 months now and a lot of the issues brought
up can happen after the fact because, like this one, it involves fixing
up existing Xen makefiles rather than issues with this series.

> 
>> +# force target
>> +PHONY += FORCE
>> +
>> +FORCE:
>> +
>> +SRCARCH = $(shell echo $(ARCH) | \
>> +	sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
>> +export SRCARCH
>> +
>> +# include the original Makefile from Linux
>> +include $(src)/Makefile
>> +# include the Makefile.host from Linux
>> +include $(src)/Makefile.host
> 
> Just one comment for both lines would do.
> 
> Jan
> 

Sure. I'll send you a squash.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel