|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCHv6] 02/28] build: build Kconfig and config rules
On 11/30/15 8:36 AM, Jan Beulich wrote: >>>> On 24.11.15 at 18:51, <cardoe@xxxxxxxxxx> wrote: >> --- a/.gitignore >> +++ b/.gitignore >> @@ -217,6 +217,11 @@ tools/xentrace/tbctl >> tools/xentrace/xenctx >> tools/xentrace/xentrace >> xen/.banner >> +xen/.config >> +xen/.config.old >> +xen/defconfig >> +xen/**/*.cmd >> +xen/**/modules.order > > The last two seem rather dubious to me in our environment. > >> @@ -239,6 +244,9 @@ xen/include/headers++.chk >> xen/include/asm >> xen/include/asm-*/asm-offsets.h >> xen/include/compat/* >> +xen/include/config.h > > Linux doesn't seem to generate a file like this. Is this perhaps a > stale entry you're copying over? > >> --- /dev/null >> +++ b/xen/Kconfig >> @@ -0,0 +1,26 @@ >> +# >> +# For a description of the syntax of this configuration file, >> +# see Documentation/kbuild/kconfig-language.txt from the Linux > > This path needs fixing. > >> +# kernel source tree. >> +# >> +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 >> + string >> + option env="XEN_FULLVERSION" >> + >> +config DEFCONFIG_LIST >> + string >> + option defconfig_list >> + default "$ARCH_DEFCONFIG" >> + default "arch/$SRCARCH/defconfig" > > Linux'es variant has just SRCARCH and the source directive. Why > do we need so much more here? In any even I again think you > should at least briefly explain any extensions you do in the commit > message. I'm not understanding the question here. Linux has 5 items in their list. The last two are identical to the 2 that I retained. Given other evolutions of the patchset I can drop the last one and keep it as just $ARCH_DEFCONFIG since I've always got that defined for arm and x86. > >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -17,6 +17,9 @@ export XEN_ROOT := $(BASEDIR)/.. >> >> EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi >> >> +# Don't break if the build process wasn't called from the top level >> +XEN_TARGET_ARCH ?= $(shell uname -m) > > I don't see the need for this. We already require setting this on the > command line when invoking a build process bypassing the top level > directory. Later in the series, when actually using the produced > xen/.config, I could see this becoming a nice enhancement (by > reading the value from the file). One of the early on reviews told me it was not required (and I can confirm that I can run "git clean -xdf && cd xen && make" and it will build). This change makes that behavior still work. > >> @@ -216,3 +220,16 @@ FORCE: >> >> %/: FORCE >> $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o built_in_bin.o >> + >> +kconfig := silentoldconfig oldconfig config menuconfig defconfig \ >> + nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig >> +.PHONY: $(kconfig) >> +$(kconfig): >> + $(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile ARCH=$(XEN_TARGET_ARCH) >> $@ >> + >> +$(BASEDIR)/include/config/%.conf: $(BASEDIR)/include/config/auto.conf.cmd >> + $(Q)$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile >> ARCH=$(XEN_TARGET_ARCH) silentoldconfig > > Okay, I have found the Linux original to this and it's the same there, > but I'd like to ask anyway: How can this work without $* getting > passed on? silentoldconfig is the step that actually takes the .config and generates out all the files like auto.conf and generated/config.h It should be in xen/scripts/kconfig/Makefile.linux I believe. > >> +# Allow people to just run `make` as before and not force them to configure >> +$(BASEDIR)/.config $(BASEDIR)/include/config/auto.conf.cmd: ; >> + $(Q)$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile >> ARCH=$(XEN_TARGET_ARCH) defconfig > > Is this correct? If I have xen/.config but no > xen/include/config/auto.conf.cmd I surely don't want "defconfig" to > be run (unless "defconfig" honors xen/.config, in which case the > goal's name would be questionable)? > > Also do you really need all the explicit $(BASEDIR) references in the > rules above? I was asked to make the build work with out of tree builds and that's why I went this route. The rest of the Xen build system doesn't support out of tree builds so one can argue that's not needed. > >> --- /dev/null >> +++ b/xen/arch/arm/configs/arm32_defconfig >> @@ -0,0 +1 @@ >> +CONFIG_64BIT=n >> diff --git a/xen/arch/arm/configs/arm64_defconfig >> b/xen/arch/arm/configs/arm64_defconfig >> new file mode 100644 >> index 0000000..e69de29 > > As said before I'm not really up to speed with defconfigs, but the > arm64 variant being empty but the arm32 one disabling 64BIT > seems backwards: You don't even have a choice on arm32. I agree its squirrelly here. I believe I followed Linux and it only seems to come into play when you don't specify you're target arch and are running natively on the platform but want to build for the other arch. It likely can be dropped if its too confusing. > >> --- /dev/null >> +++ b/xen/arch/x86/Kconfig >> @@ -0,0 +1,18 @@ >> +config X86_64 >> + def_bool y >> + >> +config X86 >> + def_bool y >> + select HAS_GDBSX > > Didn't you mean to convert HAS_* in subsequent patches? Yes. Typo. > > Also - not 64BIT here, yet this - if added to ARM - should be added > here too so it can be used in common code. That can fold into the above drop of 64BIT as configurable. > >> +config ARCH_DEFCONFIG >> + string >> + default "arch/x86/configs/x86_64_defconfig" > > x86_defconfig perhaps? No. I was told to drop support for x86 entirely in an earlier review. Its not possible to configure for 32-bit x86 in v6. > >> --- /dev/null >> +++ b/xen/scripts/kconfig/Makefile >> @@ -0,0 +1,77 @@ >> +# xen/scripts/kconfig >> + >> +MAKEFLAGS += -rR > > Why? kconfig in Linux makes the assumption that implicit rules and variables are turned off for the whole build process. Xen (oddly) does not do this. I sent another patch a while back that I planned on resending that disables the implicit rules and implicit variables. Currently it makes no difference to the build of the hypervisor to have implicit rules/variables disabled because all the rules are explicitly defined and the implicit variables are unused but it certainly helps debugging of the build process (and build time) to turn them off if they aren't needed. > >> +# default rule to do nothing >> +all: >> + >> +XEN_ROOT = $(CURDIR)/.. >> + >> +# 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 >> +objtree = $(srctree) >> +src := scripts/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)); \ >> + ) > > Considering how much you stripped these fro their Linux original, why > did you leave the always true $(if ...) in here, and why did you not > consolidate things only single lines (helping readability). Funny story about GNU Make but it clears up build problems between 3.81 and 4.0 on machines that I tested this on. > >> +define multi_depend >> +$(foreach m, $(notdir $1), \ >> + $(eval $(obj)/$m: \ >> + $(addprefix $(obj)/, $(foreach s, $3, $($(m:%$(strip $2)=%$(s))))))) >> +endef > > Do you really need this? Yes. Makefile.host uses it. I brought this little snippet in to avoid bringing in 2 more files that reference each other. > >> +# Set our default defconfig file >> +KBUILD_DEFCONFIG := $(ARCH)_defconfig >> + >> +# provide our shell >> +CONFIG_SHELL := $(SHELL) >> + >> +# provide the host compiler >> +HOSTCC := gcc >> +HOSTCXX := g++ > > Why is the C setting from ./Config.mk not good enough (the C++ > one should then be added there if indeed needed, which I doubt)? > > Jan > From an earlier review I dropped the usage of Config.mk because I was told the hypervisor (xen/) should build without ./configure being run. The C++ is needed for the kconfig frontends. -- Doug Goldstein Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |