| Message ID | 1453329821-3167-2-git-send-email-pieter@boesman.nl |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jan 20, 2016 at 11:43 PM, Pieter Smith <pieter@boesman.nl> wrote: > No functional changes, but prepares barebox for building with two > configurations (similar to package/gcc). > > Signed-off-by: Pieter Smith <pieter@boesman.nl> Tested-by: Yegor Yefremov <yegorslists@googlemail.com> > --- > boot/barebox/Config.in | 71 +++++++++++++++++++----------- > boot/barebox/barebox-1/barebox-1.hash | 1 + > boot/barebox/barebox-1/barebox-1.mk | 81 +++++++++++++++++++++++++++++++++++ > boot/barebox/barebox.mk | 61 +------------------------- > 4 files changed, 129 insertions(+), 85 deletions(-) > create mode 120000 boot/barebox/barebox-1/barebox-1.hash > create mode 100644 boot/barebox/barebox-1/barebox-1.mk > > diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in > index 39cb5d2..ed120af 100644 > --- a/boot/barebox/Config.in > +++ b/boot/barebox/Config.in > @@ -64,9 +64,52 @@ config BR2_TARGET_BAREBOX_CUSTOM_GIT_VERSION > > endif > > +config BR2_TARGET_BAREBOX_BAREBOXENV > + bool "bareboxenv tool in target" > + help > + Install bareboxenv tool in target. > + > +config BR2_TARGET_BAREBOX_CUSTOM_ENV > + bool "Generate an environment image" > + help > + Generate a custom environment image. This environment will > + contain the variables and scripts to be used at boot by > + barebox. > + > +config BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH > + string "Environment path" > + depends on BR2_TARGET_BAREBOX_CUSTOM_ENV > + help > + Path to the directory containing the custom barebox > + environment. Depending on your setup, it will probably be > + based on either the content of the defaultenv or > + defaultenv-2 directories in the barebox source code, plus > + the additions needed. The output will be an image in the > + barebox devfs format, stored in the images directory, with > + the same name as the directory name given here. > > choice > - prompt "Barebox configuration" > + prompt "Number of Barebox configurations" > + default BR2_TARGET_BAREBOX_SINGLE_CONFIG > + > +config BR2_TARGET_BAREBOX_ONE_CONFIG > + select BR2_TARGET_BAREBOX_1 > + bool "Build 1 config" > + help > + Build only one barebox config. > + Useful for building the traditional TPL (Tertiary Program > + Loader). > + > +endchoice > + > +config BR2_TARGET_BAREBOX_1 > + bool "Barebox configuration 1" > + default y > + > +if BR2_TARGET_BAREBOX_1 > + > +choice > + prompt "Type of configuration" > default BR2_TARGET_BAREBOX_USE_DEFCONFIG > > config BR2_TARGET_BAREBOX_USE_DEFCONFIG > @@ -78,7 +121,7 @@ config BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG > endchoice > > config BR2_TARGET_BAREBOX_BOARD_DEFCONFIG > - string "board defconfig" > + string "Board defconfig" > depends on BR2_TARGET_BAREBOX_USE_DEFCONFIG > help > Name of the board for which Barebox should be built, without > @@ -97,28 +140,6 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES > A space-separated list of configuration fragment files, > that will be merged to the main Barebox configuration file. > > -config BR2_TARGET_BAREBOX_BAREBOXENV > - bool "bareboxenv tool in target" > - help > - Install bareboxenv tool in target. > - > -config BR2_TARGET_BAREBOX_CUSTOM_ENV > - bool "Generate an environment image" > - help > - Generate a custom environment image. This environment will > - contain the variables and scripts to be used at boot by > - barebox. > - > -config BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH > - string "Environment path" > - depends on BR2_TARGET_BAREBOX_CUSTOM_ENV > - help > - Path to the directory containing the custom barebox > - environment. Depending on your setup, it will probably be > - based on either the content of the defaultenv or > - defaultenv-2 directories in the barebox source code, plus > - the additions needed. The output will be an image in the > - barebox devfs format, stored in the images directory, with > - the same name as the directory name given here. > +endif > > endif > diff --git a/boot/barebox/barebox-1/barebox-1.hash b/boot/barebox/barebox-1/barebox-1.hash > new file mode 120000 > index 0000000..b6462b8 > --- /dev/null > +++ b/boot/barebox/barebox-1/barebox-1.hash > @@ -0,0 +1 @@ > +../barebox.hash > \ No newline at end of file > diff --git a/boot/barebox/barebox-1/barebox-1.mk b/boot/barebox/barebox-1/barebox-1.mk > new file mode 100644 > index 0000000..3374ece > --- /dev/null > +++ b/boot/barebox/barebox-1/barebox-1.mk > @@ -0,0 +1,81 @@ > +################################################################################ > +# > +# barebox-1 > +# > +################################################################################ > + > +BAREBOX_1_VERSION = $(BAREBOX_VERSION) > +BAREBOX_1_SITE = $(BAREBOX_SITE) > +BAREBOX_1_SITE_METHOD = $(BAREBOX_SITE_METHOD) > +BAREBOX_1_SOURCE = $(BAREBOX_SOURCE) > +BAREBOX_1_DEPENDENCIES = $(BAREBOX_DEPENDENCIES) > +BAREBOX_1_LICENSE = $(BAREBOX_LICENSE) > +BAREBOX_1_LICENSE_FILES = $(BAREBOX_LICENSE_FILES) > +BAREBOX_1_POST_PATCH_HOOKS += $(BAREBOX_POST_PATCH_HOOKS) > +BAREBOX_1_MAKE_FLAGS = $(BAREBOX_MAKE_FLAGS) > +BAREBOX_1_MAKE_ENV = $(BAREBOX_MAKE_ENV) > +BAREBOX_1_INSTALL_IMAGES = $(BAREBOX_INSTALL_IMAGES) > + > +ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y) > +BAREBOX_1_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig > +else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y) > +BAREBOX_1_KCONFIG_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)) > +endif > + > +BAREBOX_1_KCONFIG_FRAGMENT_FILES = $(BAREBOX_KCONFIG_FRAGMENT_FILES) > +BAREBOX_1_KCONFIG_EDITORS = $(BAREBOX_KCONFIG_EDITORS) > +BAREBOX_1_KCONFIG_OPTS = $(BAREBOX_MAKE_FLAGS) > + > +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > +define BAREBOX_1_BUILD_BAREBOXENV_CMDS > + $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -o $(@D)/bareboxenv \ > + $(@D)/scripts/bareboxenv.c > +endef > +endif > + > +ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y) > +BAREBOX_1_ENV_NAME = $(notdir $(call qstrip,\ > + $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH))) > +define BAREBOX_1_BUILD_CUSTOM_ENV > + $(@D)/scripts/bareboxenv -s \ > + $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)) \ > + $(@D)/$(BAREBOX_1_ENV_NAME) > +endef > +define BAREBOX_1_INSTALL_CUSTOM_ENV > + cp $(@D)/$(BAREBOX_1_ENV_NAME) $(BINARIES_DIR) > +endef > +endif > + > +define BAREBOX_1_BUILD_CMDS > + $(BAREBOX_1_BUILD_BAREBOXENV_CMDS) > + $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_1_MAKE_FLAGS) -C $(@D) > + $(BAREBOX_1_BUILD_CUSTOM_ENV) > +endef > + > +define BAREBOX_1_INSTALL_IMAGES_CMDS > + if test -h $(@D)/barebox-flash-image ; then \ > + cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > + else \ > + cp $(@D)/barebox.bin $(BINARIES_DIR);\ > + fi > + $(BAREBOX_1_INSTALL_CUSTOM_ENV) > +endef > + > +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > +define BAREBOX_1_INSTALL_TARGET_CMDS > + cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin > +endef > +endif > + > +# Checks to give errors that the user can understand > +# Must be before we call to kconfig-package > +ifeq ($(BR2_TARGET_BAREBOX_2)$(BR_BUILDING),yy) > +# We must use the user-supplied kconfig value, because > +# BAREBOX_1_KCONFIG_DEFCONFIG will at least contain the > +# trailing _defconfig > +ifeq ($(or $(BAREBOX_1_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) > +$(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) > +endif > +endif > + > +$(eval $(kconfig-package)) > diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk > index 7715daf..55bd187 100644 > --- a/boot/barebox/barebox.mk > +++ b/boot/barebox/barebox.mk > @@ -55,66 +55,7 @@ endif > BAREBOX_MAKE_FLAGS = ARCH=$(BAREBOX_ARCH) CROSS_COMPILE="$(TARGET_CROSS)" > BAREBOX_MAKE_ENV = $(TARGET_MAKE_ENV) > > -ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y) > -BAREBOX_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig > -else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y) > -BAREBOX_KCONFIG_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)) > -endif > - > BAREBOX_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES)) > BAREBOX_KCONFIG_EDITORS = menuconfig xconfig gconfig nconfig > -BAREBOX_KCONFIG_OPTS = $(BAREBOX_MAKE_FLAGS) > - > -ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > -define BAREBOX_BUILD_BAREBOXENV_CMDS > - $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -o $(@D)/bareboxenv \ > - $(@D)/scripts/bareboxenv.c > -endef > -endif > - > -ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y) > -BAREBOX_ENV_NAME = $(notdir $(call qstrip,\ > - $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH))) > -define BAREBOX_BUILD_CUSTOM_ENV > - $(@D)/scripts/bareboxenv -s \ > - $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)) \ > - $(@D)/$(BAREBOX_ENV_NAME) > -endef > -define BAREBOX_INSTALL_CUSTOM_ENV > - cp $(@D)/$(BAREBOX_ENV_NAME) $(BINARIES_DIR) > -endef > -endif > - > -define BAREBOX_BUILD_CMDS > - $(BAREBOX_BUILD_BAREBOXENV_CMDS) > - $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) > - $(BAREBOX_BUILD_CUSTOM_ENV) > -endef > - > -define BAREBOX_INSTALL_IMAGES_CMDS > - if test -h $(@D)/barebox-flash-image ; then \ > - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > - else \ > - cp $(@D)/barebox.bin $(BINARIES_DIR);\ > - fi > - $(BAREBOX_INSTALL_CUSTOM_ENV) > -endef > - > -ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > -define BAREBOX_INSTALL_TARGET_CMDS > - cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin > -endef > -endif > - > -# Checks to give errors that the user can understand > -# Must be before we call to kconfig-package > -ifeq ($(BR2_TARGET_BAREBOX)$(BR_BUILDING),yy) > -# We must use the user-supplied kconfig value, because > -# BAREBOX_KCONFIG_DEFCONFIG will at least contain the > -# trailing _defconfig > -ifeq ($(or $(BAREBOX_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) > -$(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) > -endif > -endif > > -$(eval $(kconfig-package)) > +include $(sort $(wildcard boot/barebox/*/*.mk)) > -- > 2.5.0 >
On 01/20/16 23:43, Pieter Smith wrote: > No functional changes, but prepares barebox for building with two > configurations (similar to package/gcc). > > Signed-off-by: Pieter Smith <pieter@boesman.nl> > --- > boot/barebox/Config.in | 71 +++++++++++++++++++----------- > boot/barebox/barebox-1/barebox-1.hash | 1 + > boot/barebox/barebox-1/barebox-1.mk | 81 +++++++++++++++++++++++++++++++++++ > boot/barebox/barebox.mk | 61 +------------------------- > 4 files changed, 129 insertions(+), 85 deletions(-) > create mode 120000 boot/barebox/barebox-1/barebox-1.hash > create mode 100644 boot/barebox/barebox-1/barebox-1.mk > > diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in > index 39cb5d2..ed120af 100644 > --- a/boot/barebox/Config.in > +++ b/boot/barebox/Config.in > @@ -64,9 +64,52 @@ config BR2_TARGET_BAREBOX_CUSTOM_GIT_VERSION > > endif > [snip] > > choice > - prompt "Barebox configuration" > + prompt "Number of Barebox configurations" > + default BR2_TARGET_BAREBOX_SINGLE_CONFIG > + > +config BR2_TARGET_BAREBOX_ONE_CONFIG > + select BR2_TARGET_BAREBOX_1 > + bool "Build 1 config" > + help > + Build only one barebox config. > + Useful for building the traditional TPL (Tertiary Program > + Loader). > + > +endchoice There is no need for this choice. You always need barebox-1 anyway. So you can actually keep the Config.in as it is, and add the second barebox configuration at the end. > + > +config BR2_TARGET_BAREBOX_1 > + bool "Barebox configuration 1" > + default y > + > +if BR2_TARGET_BAREBOX_1 > + > +choice > + prompt "Type of configuration" > default BR2_TARGET_BAREBOX_USE_DEFCONFIG > > config BR2_TARGET_BAREBOX_USE_DEFCONFIG > @@ -78,7 +121,7 @@ config BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG > endchoice > > config BR2_TARGET_BAREBOX_BOARD_DEFCONFIG > - string "board defconfig" > + string "Board defconfig" Why change the capitalisation? Should be a separate patch. > depends on BR2_TARGET_BAREBOX_USE_DEFCONFIG > help > Name of the board for which Barebox should be built, without > @@ -97,28 +140,6 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES > A space-separated list of configuration fragment files, > that will be merged to the main Barebox configuration file. > [snip] > diff --git a/boot/barebox/barebox-1/barebox-1.mk b/boot/barebox/barebox-1/barebox-1.mk > new file mode 100644 > index 0000000..3374ece > --- /dev/null > +++ b/boot/barebox/barebox-1/barebox-1.mk > @@ -0,0 +1,81 @@ > +################################################################################ > +# > +# barebox-1 > +# > +################################################################################ > + > +BAREBOX_1_VERSION = $(BAREBOX_VERSION) > +BAREBOX_1_SITE = $(BAREBOX_SITE) > +BAREBOX_1_SITE_METHOD = $(BAREBOX_SITE_METHOD) > +BAREBOX_1_SOURCE = $(BAREBOX_SOURCE) > +BAREBOX_1_DEPENDENCIES = $(BAREBOX_DEPENDENCIES) > +BAREBOX_1_LICENSE = $(BAREBOX_LICENSE) > +BAREBOX_1_LICENSE_FILES = $(BAREBOX_LICENSE_FILES) > +BAREBOX_1_POST_PATCH_HOOKS += $(BAREBOX_POST_PATCH_HOOKS) > +BAREBOX_1_MAKE_FLAGS = $(BAREBOX_MAKE_FLAGS) > +BAREBOX_1_MAKE_ENV = $(BAREBOX_MAKE_ENV) MAKE_FLAGS and MAKE_ENV are only used in the commands below, so no need to define a barebox-1 version. > +BAREBOX_1_INSTALL_IMAGES = $(BAREBOX_INSTALL_IMAGES) This is just YES, it's a bit silly to add an extra variable for that. > + > +ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y) > +BAREBOX_1_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig > +else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y) > +BAREBOX_1_KCONFIG_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)) > +endif > + > +BAREBOX_1_KCONFIG_FRAGMENT_FILES = $(BAREBOX_KCONFIG_FRAGMENT_FILES) > +BAREBOX_1_KCONFIG_EDITORS = $(BAREBOX_KCONFIG_EDITORS) > +BAREBOX_1_KCONFIG_OPTS = $(BAREBOX_MAKE_FLAGS) > + > +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > +define BAREBOX_1_BUILD_BAREBOXENV_CMDS > + $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -o $(@D)/bareboxenv \ > + $(@D)/scripts/bareboxenv.c > +endef > +endif > + > +ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y) > +BAREBOX_1_ENV_NAME = $(notdir $(call qstrip,\ > + $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH))) > +define BAREBOX_1_BUILD_CUSTOM_ENV > + $(@D)/scripts/bareboxenv -s \ > + $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)) \ > + $(@D)/$(BAREBOX_1_ENV_NAME) > +endef > +define BAREBOX_1_INSTALL_CUSTOM_ENV > + cp $(@D)/$(BAREBOX_1_ENV_NAME) $(BINARIES_DIR) > +endef > +endif > + > +define BAREBOX_1_BUILD_CMDS > + $(BAREBOX_1_BUILD_BAREBOXENV_CMDS) > + $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_1_MAKE_FLAGS) -C $(@D) > + $(BAREBOX_1_BUILD_CUSTOM_ENV) > +endef BAREBOX_1_BUILD_CMDS and BAREBOX_2_BUILD_CMDS are very similar. You can refactor them as follows (I think): define BAREBOX_BUILD_CMDS $($(PKG)_BUILD_BAREBOXENV_CMDS) $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) $($(PKG)_BUILD_CUSTOM_ENV) endef BAREBOX_1_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) Since BAREBOX_2_BUILD_BAREBOXENV_CMDS isn't defined, that line will just disappear, leaving just the make command for barebox-2. Same for the others below. Note that introducing the $(PKG) bits should be a separate patch, not together with the move to barebox-1. > + > +define BAREBOX_1_INSTALL_IMAGES_CMDS > + if test -h $(@D)/barebox-flash-image ; then \ > + cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > + else \ > + cp $(@D)/barebox.bin $(BINARIES_DIR);\ > + fi > + $(BAREBOX_1_INSTALL_CUSTOM_ENV) > +endef > + > +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > +define BAREBOX_1_INSTALL_TARGET_CMDS > + cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin > +endef > +endif > + > +# Checks to give errors that the user can understand > +# Must be before we call to kconfig-package > +ifeq ($(BR2_TARGET_BAREBOX_2)$(BR_BUILDING),yy) BAREBOX_1 here. > +# We must use the user-supplied kconfig value, because > +# BAREBOX_1_KCONFIG_DEFCONFIG will at least contain the > +# trailing _defconfig > +ifeq ($(or $(BAREBOX_1_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) > +$(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) > +endif > +endif > + > +$(eval $(kconfig-package)) I wonder if it wouldn't be possible to keep barebox.mk unchanged, and just add at the end (after the kconfig-package): include boot/barebox/barebox-2/barebox-2.mk That's not entirely similar to gcc, but it's more consistent with what it means. You always have the barebox package, and you have an optional extra barebox-2 package which is a kind of submodule of barebox. Note however that we currently don't have this pattern at all, so it could be controversial. But I think it will simplify the patch a lot, and also simplify the logic. So in that case, you'd have a first patch that adds the required refactorings in barebox.mk so the same variables are useable for barebox-2, and a second patch that adds barebox-2 (and patches 3 and 4 stay the same of course). This is complicated stuff, thanks for working on this, and sorry that it's taking so long! Regards, Arnout [snip]
On Sat, Feb 27, 2016 at 12:17:15AM +0100, Arnout Vandecappelle wrote: > On 01/20/16 23:43, Pieter Smith wrote: [snip] > I wonder if it wouldn't be possible to keep barebox.mk unchanged, and just add > at the end (after the kconfig-package): > > include boot/barebox/barebox-2/barebox-2.mk > > That's not entirely similar to gcc, but it's more consistent with what it > means. You always have the barebox package, and you have an optional extra > barebox-2 package which is a kind of submodule of barebox. Note however that we > currently don't have this pattern at all, so it could be controversial. But I > think it will simplify the patch a lot, and also simplify the logic. > > So in that case, you'd have a first patch that adds the required refactorings > in barebox.mk so the same variables are useable for barebox-2, and a second > patch that adds barebox-2 (and patches 3 and 4 stay the same of course). Yes. Thanks. A lot less messy. I started looking into this. It seems doable. It might be neater to still split a barebox-common.mk to avoid a long list of variable copies. Give me a day on this. > This is complicated stuff, thanks for working on this, and sorry that it's > taking so long! > > Regards, > Arnout > > [snip] > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286500 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF - Pieter
On Sun, Feb 28, 2016 at 09:12:04AM +0100, Pieter Smith wrote: > On Sat, Feb 27, 2016 at 12:17:15AM +0100, Arnout Vandecappelle wrote: > > On 01/20/16 23:43, Pieter Smith wrote: > [snip] > > I wonder if it wouldn't be possible to keep barebox.mk unchanged, and just add > > at the end (after the kconfig-package): > > > > include boot/barebox/barebox-2/barebox-2.mk > > > > That's not entirely similar to gcc, but it's more consistent with what it > > means. You always have the barebox package, and you have an optional extra > > barebox-2 package which is a kind of submodule of barebox. Note however that we > > currently don't have this pattern at all, so it could be controversial. But I > > think it will simplify the patch a lot, and also simplify the logic. > > > > So in that case, you'd have a first patch that adds the required refactorings > > in barebox.mk so the same variables are useable for barebox-2, and a second > > patch that adds barebox-2 (and patches 3 and 4 stay the same of course). > > Yes. Thanks. A lot less messy. I started looking into this. It seems doable. It > might be neater to still split a barebox-common.mk to avoid a long list of > variable copies. Give me a day on this. Thanks for the suggestion. It is shaping up nicely. There is one aspect that I would appreciate input on: To all but completely eliminate duplication in the makefiles for the two packages, I am wrapping all the current functionality in boot/barebox/barebox.mk in a parameterized barebox-package function. E.g.: define barebox-package $(1)_VERSION = $$(call qstrip,$$(BR2_TARGET_BAREBOX_VERSION)) ... $$(eval $$(kconfig-package)) endef $(eval $(call barebox-package,BAREBOX)) And the whole of boot/barebox/barebox-2/barebox-2.mk becomes: $(eval $(call barebox-package,BAREBOX_2)) This however cannot avoid Config.in duplication, but the barebox-2 makefile inherits all future barebox makefile improvements. The catch is that existing barebox patches will have merge conflicts. I already tied up with Yegor on the embedded custom environment patch-set, which he is willing to rebase + solve the merge conflicts. We can then submit the series with Yegor's patch and a barebox defconfig for the Beaglebone Black as per your suggestion. Do you think this is acceptable? > > This is complicated stuff, thanks for working on this, and sorry that it's > > taking so long! > > > > Regards, > > Arnout > > > > [snip] > > > > -- > > Arnout Vandecappelle arnout at mind be > > Senior Embedded Software Architect +32-16-286500 > > Essensium/Mind http://www.mind.be > > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > > GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF > > - Pieter - Pieter
On Sat, Feb 27, 2016 at 12:17:15AM +0100, Arnout Vandecappelle wrote: > On 01/20/16 23:43, Pieter Smith wrote: > > No functional changes, but prepares barebox for building with two > > configurations (similar to package/gcc). > > > > Signed-off-by: Pieter Smith <pieter@boesman.nl> > > --- > > boot/barebox/Config.in | 71 +++++++++++++++++++----------- > > boot/barebox/barebox-1/barebox-1.hash | 1 + > > boot/barebox/barebox-1/barebox-1.mk | 81 +++++++++++++++++++++++++++++++++++ > > boot/barebox/barebox.mk | 61 +------------------------- > > 4 files changed, 129 insertions(+), 85 deletions(-) > > create mode 120000 boot/barebox/barebox-1/barebox-1.hash > > create mode 100644 boot/barebox/barebox-1/barebox-1.mk > > > > diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in > > index 39cb5d2..ed120af 100644 > > --- a/boot/barebox/Config.in > > +++ b/boot/barebox/Config.in > > @@ -64,9 +64,52 @@ config BR2_TARGET_BAREBOX_CUSTOM_GIT_VERSION > > > > endif > > > [snip] > > > > choice > > - prompt "Barebox configuration" > > + prompt "Number of Barebox configurations" > > + default BR2_TARGET_BAREBOX_SINGLE_CONFIG > > + > > +config BR2_TARGET_BAREBOX_ONE_CONFIG > > + select BR2_TARGET_BAREBOX_1 > > + bool "Build 1 config" > > + help > > + Build only one barebox config. > > + Useful for building the traditional TPL (Tertiary Program > > + Loader). > > + > > +endchoice > > There is no need for this choice. You always need barebox-1 anyway. So you can > actually keep the Config.in as it is, and add the second barebox configuration > at the end. ACK. Will not be in the v4 patch series. > > + > > +config BR2_TARGET_BAREBOX_1 > > + bool "Barebox configuration 1" > > + default y > > + > > +if BR2_TARGET_BAREBOX_1 > > + > > +choice > > + prompt "Type of configuration" > > default BR2_TARGET_BAREBOX_USE_DEFCONFIG > > > > config BR2_TARGET_BAREBOX_USE_DEFCONFIG > > @@ -78,7 +121,7 @@ config BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG > > endchoice > > > > config BR2_TARGET_BAREBOX_BOARD_DEFCONFIG > > - string "board defconfig" > > + string "Board defconfig" > > Why change the capitalisation? Should be a separate patch. Oops. This snuck through. I don't consider this necessary. > > depends on BR2_TARGET_BAREBOX_USE_DEFCONFIG > > help > > Name of the board for which Barebox should be built, without > > @@ -97,28 +140,6 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES > > A space-separated list of configuration fragment files, > > that will be merged to the main Barebox configuration file. > > > [snip] > > diff --git a/boot/barebox/barebox-1/barebox-1.mk b/boot/barebox/barebox-1/barebox-1.mk > > new file mode 100644 > > index 0000000..3374ece > > --- /dev/null > > +++ b/boot/barebox/barebox-1/barebox-1.mk > > @@ -0,0 +1,81 @@ > > +################################################################################ > > +# > > +# barebox-1 > > +# > > +################################################################################ > > + > > +BAREBOX_1_VERSION = $(BAREBOX_VERSION) > > +BAREBOX_1_SITE = $(BAREBOX_SITE) > > +BAREBOX_1_SITE_METHOD = $(BAREBOX_SITE_METHOD) > > +BAREBOX_1_SOURCE = $(BAREBOX_SOURCE) > > +BAREBOX_1_DEPENDENCIES = $(BAREBOX_DEPENDENCIES) > > +BAREBOX_1_LICENSE = $(BAREBOX_LICENSE) > > +BAREBOX_1_LICENSE_FILES = $(BAREBOX_LICENSE_FILES) > > +BAREBOX_1_POST_PATCH_HOOKS += $(BAREBOX_POST_PATCH_HOOKS) > > +BAREBOX_1_MAKE_FLAGS = $(BAREBOX_MAKE_FLAGS) > > +BAREBOX_1_MAKE_ENV = $(BAREBOX_MAKE_ENV) > > MAKE_FLAGS and MAKE_ENV are only used in the commands below, so no need to > define a barebox-1 version. > > > +BAREBOX_1_INSTALL_IMAGES = $(BAREBOX_INSTALL_IMAGES) > > This is just YES, it's a bit silly to add an extra variable for that. If you can ACK the barebox-package function suggestion, this all disappears. [snip] > BAREBOX_1_BUILD_CMDS and BAREBOX_2_BUILD_CMDS are very similar. You can > refactor them as follows (I think): > > define BAREBOX_BUILD_CMDS > $($(PKG)_BUILD_BAREBOXENV_CMDS) > $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) > $($(PKG)_BUILD_CUSTOM_ENV) > endef > BAREBOX_1_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > > Since BAREBOX_2_BUILD_BAREBOXENV_CMDS isn't defined, that line will just > disappear, leaving just the make command for barebox-2. > > Same for the others below. > > Note that introducing the $(PKG) bits should be a separate patch, not together > with the move to barebox-1. Thanks. Will spend more time eliminating code duplication in future. If you can ACK the barebox-package function suggestion, this all disappears. > > > + > > +define BAREBOX_1_INSTALL_IMAGES_CMDS > > + if test -h $(@D)/barebox-flash-image ; then \ > > + cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ > > + else \ > > + cp $(@D)/barebox.bin $(BINARIES_DIR);\ > > + fi > > + $(BAREBOX_1_INSTALL_CUSTOM_ENV) > > +endef > > + > > +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) > > +define BAREBOX_1_INSTALL_TARGET_CMDS > > + cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin > > +endef > > +endif > > + > > +# Checks to give errors that the user can understand > > +# Must be before we call to kconfig-package > > +ifeq ($(BR2_TARGET_BAREBOX_2)$(BR_BUILDING),yy) > > BAREBOX_1 here. Oops. Thanks! [snip] > I wonder if it wouldn't be possible to keep barebox.mk unchanged, and just add > at the end (after the kconfig-package): > > include boot/barebox/barebox-2/barebox-2.mk > > That's not entirely similar to gcc, but it's more consistent with what it > means. You always have the barebox package, and you have an optional extra > barebox-2 package which is a kind of submodule of barebox. Note however that we > currently don't have this pattern at all, so it could be controversial. But I > think it will simplify the patch a lot, and also simplify the logic. > > So in that case, you'd have a first patch that adds the required refactorings > in barebox.mk so the same variables are useable for barebox-2, and a second > patch that adds barebox-2 (and patches 3 and 4 stay the same of course). Already accepted the challenge in a previous mail. ;-) > This is complicated stuff, thanks for working on this, and sorry that it's > taking so long! No problem. I like the complicated stuff. Makefiles are a wonderful opportunity to prove that in any language you can write cleaner code, no matter how horrible the syntax. ;-) > Regards, > Arnout > > [snip] > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286500 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF - Pieter
On 02/29/16 08:47, Pieter Smith wrote: > On Sun, Feb 28, 2016 at 09:12:04AM +0100, Pieter Smith wrote: >> On Sat, Feb 27, 2016 at 12:17:15AM +0100, Arnout Vandecappelle wrote: >>> On 01/20/16 23:43, Pieter Smith wrote: >> [snip] >>> I wonder if it wouldn't be possible to keep barebox.mk unchanged, and just add >>> at the end (after the kconfig-package): >>> >>> include boot/barebox/barebox-2/barebox-2.mk >>> >>> That's not entirely similar to gcc, but it's more consistent with what it >>> means. You always have the barebox package, and you have an optional extra >>> barebox-2 package which is a kind of submodule of barebox. Note however that we >>> currently don't have this pattern at all, so it could be controversial. But I >>> think it will simplify the patch a lot, and also simplify the logic. >>> >>> So in that case, you'd have a first patch that adds the required refactorings >>> in barebox.mk so the same variables are useable for barebox-2, and a second >>> patch that adds barebox-2 (and patches 3 and 4 stay the same of course). >> >> Yes. Thanks. A lot less messy. I started looking into this. It seems doable. It >> might be neater to still split a barebox-common.mk to avoid a long list of >> variable copies. Give me a day on this. > > Thanks for the suggestion. It is shaping up nicely. There is one aspect that I > would appreciate input on: To all but completely eliminate duplication in the > makefiles for the two packages, I am wrapping all the current functionality in > boot/barebox/barebox.mk in a parameterized barebox-package function. E.g.: > > define barebox-package > $(1)_VERSION = $$(call qstrip,$$(BR2_TARGET_BAREBOX_VERSION)) > ... > $$(eval $$(kconfig-package)) > endef > $(eval $(call barebox-package,BAREBOX)) > > And the whole of boot/barebox/barebox-2/barebox-2.mk becomes: > > $(eval $(call barebox-package,BAREBOX_2)) No, I don't think we want this, because it hides a lot of what barebox-2 is doing internally. It's OK and actually better (in my opinion) if barebox-2.mk is just a long list like: BAREBOX_2_VERSION = $(BAREBOX_VERSION) BAREBOX_2_SITE = $(BAREBOX_SITE) ... BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) ... $(eval $(kconfig-package)) Regards, Arnout > > This however cannot avoid Config.in duplication, but the barebox-2 makefile > inherits all future barebox makefile improvements. The catch is that existing > barebox patches will have merge conflicts. > > I already tied up with Yegor on the embedded custom environment patch-set, > which he is willing to rebase + solve the merge conflicts. We can then submit > the series with Yegor's patch and a barebox defconfig for the Beaglebone Black > as per your suggestion. > > Do you think this is acceptable? > >>> This is complicated stuff, thanks for working on this, and sorry that it's >>> taking so long! >>> >>> Regards, >>> Arnout >>> >>> [snip] >>> >>> -- >>> Arnout Vandecappelle arnout at mind be >>> Senior Embedded Software Architect +32-16-286500 >>> Essensium/Mind http://www.mind.be >>> G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven >>> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle >>> GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF >> >> - Pieter > - Pieter >
On Wed, Mar 02, 2016 at 12:08:34AM +0100, Arnout Vandecappelle wrote: > On 02/29/16 08:47, Pieter Smith wrote: [snip] > > > > Thanks for the suggestion. It is shaping up nicely. There is one aspect that I > > would appreciate input on: To all but completely eliminate duplication in the > > makefiles for the two packages, I am wrapping all the current functionality in > > boot/barebox/barebox.mk in a parameterized barebox-package function. E.g.: > > > > define barebox-package > > $(1)_VERSION = $$(call qstrip,$$(BR2_TARGET_BAREBOX_VERSION)) > > ... > > $$(eval $$(kconfig-package)) > > endef > > $(eval $(call barebox-package,BAREBOX)) > > > > And the whole of boot/barebox/barebox-2/barebox-2.mk becomes: > > > > $(eval $(call barebox-package,BAREBOX_2)) > > No, I don't think we want this, because it hides a lot of what barebox-2 is > doing internally. It's OK and actually better (in my opinion) if barebox-2.mk is > just a long list like: Pity. This quite elegantly solved my concerns. barebox and barebox-2 differ only in menuconfig and barebox environment configuration space. The rest should always be identical. > BAREBOX_2_VERSION = $(BAREBOX_VERSION) > BAREBOX_2_SITE = $(BAREBOX_SITE) This shouldn't change much in future, so I can still do the variable copying if you prefer. > BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) This will not work. Both the BAREBOX_BUILD_CMDS and BAREBOX_INSTALL_IMAGES_CMDS need parameterization. I can solve this with with: 1. A scaled down version of the above barebox-package function to generate the boilerplate, or 2. Parameterizable functions and duplicated boilerplate. IMHO reducing duplication always improves clarity and maintenance, so I would go for option 1. What has your preference? > ... > $(eval $(kconfig-package)) > > > Regards, > Arnout [snip] I would like to: 1. Ensure that all improvements to barebox propagate to barebox-2 in future. 2. Avoid having something error-prone to resolve the above. IMHO using a review process to keep two pieces of code in sync is quite error-prone and an unnecessary cognitive burden on developers. - Pieter
[Adding other core devs in CC, see the beginning and the end] On 03/02/16 08:50, Pieter Smith wrote: > On Wed, Mar 02, 2016 at 12:08:34AM +0100, Arnout Vandecappelle wrote: >> On 02/29/16 08:47, Pieter Smith wrote: > [snip] >>> >>> Thanks for the suggestion. It is shaping up nicely. There is one aspect that I >>> would appreciate input on: To all but completely eliminate duplication in the >>> makefiles for the two packages, I am wrapping all the current functionality in >>> boot/barebox/barebox.mk in a parameterized barebox-package function. E.g.: >>> >>> define barebox-package >>> $(1)_VERSION = $$(call qstrip,$$(BR2_TARGET_BAREBOX_VERSION)) >>> ... >>> $$(eval $$(kconfig-package)) >>> endef >>> $(eval $(call barebox-package,BAREBOX)) >>> >>> And the whole of boot/barebox/barebox-2/barebox-2.mk becomes: >>> >>> $(eval $(call barebox-package,BAREBOX_2)) >> >> No, I don't think we want this, because it hides a lot of what barebox-2 is >> doing internally. It's OK and actually better (in my opinion) if barebox-2.mk is >> just a long list like: > > Pity. This quite elegantly solved my concerns. barebox and barebox-2 differ > only in menuconfig and barebox environment configuration space. The rest should > always be identical. > >> BAREBOX_2_VERSION = $(BAREBOX_VERSION) >> BAREBOX_2_SITE = $(BAREBOX_SITE) > > This shouldn't change much in future, so I can still do the variable copying if > you prefer. > >> BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > > This will not work. Both the BAREBOX_BUILD_CMDS and BAREBOX_INSTALL_IMAGES_CMDS > need parameterization. I can solve this with with: > 1. A scaled down version of the above barebox-package function to generate the > boilerplate, or > 2. Parameterizable functions and duplicated boilerplate. > > IMHO reducing duplication always improves clarity and maintenance, so I would > go for option 1. > > What has your preference? The BUILD_CMDS can be parameterized without using functions, by using $($(PKG)_...) variables instead of $(BAREBOX_2_...). That's what I tried to show in my first reply: define BAREBOX_BUILD_CMDS $($(PKG)_BUILD_BAREBOXENV_CMDS) $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) $($(PKG)_BUILD_CUSTOM_ENV) endef BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_BUILD_CUSTOM_ENV will not be set, so those parts are removed. The rest should be identical for barebox-2, but if you do need something else you can add something like $($(PKG)_EXTRA_FLAGS). > >> ... >> $(eval $(kconfig-package)) >> >> >> Regards, >> Arnout > > [snip] > > I would like to: > 1. Ensure that all improvements to barebox propagate to barebox-2 in future. > 2. Avoid having something error-prone to resolve the above. IMHO using a review > process to keep two pieces of code in sync is quite error-prone and an > unnecessary cognitive burden on developers. There isn't really much to keep in sync between the two, only when you suddenly add _INSTALL_STAGING_CMDS for instance you'd have to update barebox-2.mk to copy that as well. There are two reasons why I prefer the copying of variables: 1. I think that functions are a bit more difficult to understand. 2. I'm considering to add infrastructure that facilitates the pattern of copying variables, something like $(eval $(call inherit-package,barebox)) which would expand to BAREBOX_2_VERSION ?= BAREBOX_VERSION BAREBOX_2_SITE ?= BAREBOX_SITE ... BAREBOX_2_BUILD_CMDS ?= BAREBOX_BUILD_CMDS so that in the end the barebox-2.mk does indeed become nothing more than a single eval line, and that the same approach can easily be used for other packages. However, this is just my opinion, other developers may see it differently, hence the CC. Regards, Arnout > > - Pieter >
On Wed, Mar 02, 2016 at 07:12:57PM +0100, Arnout Vandecappelle wrote: > [Adding other core devs in CC, see the beginning and the end] > > On 03/02/16 08:50, Pieter Smith wrote: [snip] > >> BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > > > > This will not work. Both the BAREBOX_BUILD_CMDS and BAREBOX_INSTALL_IMAGES_CMDS > > need parameterization. I can solve this with with: > > 1. A scaled down version of the above barebox-package function to generate the > > boilerplate, or > > 2. Parameterizable functions and duplicated boilerplate. > > > > IMHO reducing duplication always improves clarity and maintenance, so I would > > go for option 1. > > > > What has your preference? > > The BUILD_CMDS can be parameterized without using functions, by using > $($(PKG)_...) variables instead of $(BAREBOX_2_...). That's what I tried to show > in my first reply: Off course, silly me... I've been making use of this since forever. It works because of the eval in $(eval $(kconfig-package)). I'll do it this way. > define BAREBOX_BUILD_CMDS > $($(PKG)_BUILD_BAREBOXENV_CMDS) > $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) > $($(PKG)_BUILD_CUSTOM_ENV) > endef > BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > > BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_BUILD_CUSTOM_ENV will not be set, > so those parts are removed. The rest should be identical for barebox-2, but if > you do need something else you can add something like $($(PKG)_EXTRA_FLAGS). I agree with not needing the additional BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_INSTALL_BAREBOXENV_CMDS (why would you need to install bareboxenv to the rootfs twice), but I would like to keep the BAREBOX_2_BUILD_CUSTOM_ENV and the built-in variant in Yegor's pending patch-set. They allow customization of the barebox environment, and therefore the boot behavior. [snip] > > I would like to: > > 1. Ensure that all improvements to barebox propagate to barebox-2 in future. > > 2. Avoid having something error-prone to resolve the above. IMHO using a review > > process to keep two pieces of code in sync is quite error-prone and an > > unnecessary cognitive burden on developers. > > There isn't really much to keep in sync between the two, only when you suddenly > add _INSTALL_STAGING_CMDS for instance you'd have to update barebox-2.mk to copy > that as well. > > There are two reasons why I prefer the copying of variables: > > 1. I think that functions are a bit more difficult to understand. > 2. I'm considering to add infrastructure that facilitates the pattern of copying > variables, something like > > $(eval $(call inherit-package,barebox)) > > > which would expand to > > BAREBOX_2_VERSION ?= BAREBOX_VERSION > BAREBOX_2_SITE ?= BAREBOX_SITE > ... > BAREBOX_2_BUILD_CMDS ?= BAREBOX_BUILD_CMDS > > so that in the end the barebox-2.mk does indeed become nothing more than a > single eval line, and that the same approach can easily be used for other packages. This would certainly be an elegant improvement! Do any of the other packages need this? I can help with this if you don't have the time. Off-course I would rather do this in a later patch-series. > However, this is just my opinion, other developers may see it differently, > hence the CC. Off course. Your opinion is held in rather high regard, so I would like to have at least one of the regular barebox developers on board. ;-) [snip] - Pieter
On Wed, Mar 02, 2016 at 10:32:56PM +0100, Pieter Smith wrote: > On Wed, Mar 02, 2016 at 07:12:57PM +0100, Arnout Vandecappelle wrote: [snip] > > The BUILD_CMDS can be parameterized without using functions, by using > > $($(PKG)_...) variables instead of $(BAREBOX_2_...). That's what I tried to show > > in my first reply: > > Off course, silly me... I've been making use of this since forever. It works > because of the eval in $(eval $(kconfig-package)). I'll do it this way. > > > define BAREBOX_BUILD_CMDS > > $($(PKG)_BUILD_BAREBOXENV_CMDS) > > $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) > > $($(PKG)_BUILD_CUSTOM_ENV) > > endef > > BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) > > > > BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_BUILD_CUSTOM_ENV will not be set, > > so those parts are removed. The rest should be identical for barebox-2, but if > > you do need something else you can add something like $($(PKG)_EXTRA_FLAGS). > > I agree with not needing the additional BAREBOX_2_BUILD_BAREBOXENV_CMDS and > BAREBOX_2_INSTALL_BAREBOXENV_CMDS (why would you need to install bareboxenv to > the rootfs twice), but I would like to keep the BAREBOX_2_BUILD_CUSTOM_ENV and > the built-in variant in Yegor's pending patch-set. They allow customization of > the barebox environment, and therefore the boot behavior. I am having some trouble with this. I am not able to handle ifdef-space diversity in this way. Ifdef-space diversity is used to determine how barebox should be configured, whether / how the barebox environment should be built, and to print user-friendly config errors. The ifdef logic is not all trivial (almost half of the makefile logic), so I would like to avoid duplicating it between barebox and barebox-2. The only way I know of to avoid this duplication is by extracting the logic info a definition and using $(eval), which is exactly what you want to avoid. Also, for two of the ifdef-space diversity sections, $(PKG) seems to be undefined at the time of evaluation. Getting to the bottom of this is proving quite taxing. I can get around this by doing an `$(eval $(call ...,BAREBOX))`, which looks a lot like my initial suggestion. I am getting to the point where I am just going to duplicate the ifdef sections and leave the consistency burden with future reviewers. I understand your concern that some developers do not understanding $(eval) all that well, but this is like trying to solve the problem with my shoelaces tied. [snip] > > However, this is just my opinion, other developers may see it differently, > > hence the CC. > > Off course. Your opinion is held in rather high regard, so I would like to have > at least one of the regular barebox developers on board. ;-) Is there anyone else we can prod to weigh in on this? Please have a look at the templated proposal and confirm your opinion on $(eval): https://github.com/smipi1/bbb_buildroot/tree/barebox_2nd_config_build-v4-templated. [snip] - Pieter
On 03/05/16 14:16, Pieter Smith wrote: > On Wed, Mar 02, 2016 at 10:32:56PM +0100, Pieter Smith wrote: >> On Wed, Mar 02, 2016 at 07:12:57PM +0100, Arnout Vandecappelle wrote: > > [snip] > >>> The BUILD_CMDS can be parameterized without using functions, by using >>> $($(PKG)_...) variables instead of $(BAREBOX_2_...). That's what I tried to show >>> in my first reply: >> >> Off course, silly me... I've been making use of this since forever. It works >> because of the eval in $(eval $(kconfig-package)). I'll do it this way. >> >>> define BAREBOX_BUILD_CMDS >>> $($(PKG)_BUILD_BAREBOXENV_CMDS) >>> $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) >>> $($(PKG)_BUILD_CUSTOM_ENV) >>> endef >>> BAREBOX_2_BUILD_CMDS = $(BAREBOX_BUILD_CMDS) >>> >>> BAREBOX_2_BUILD_BAREBOXENV_CMDS and BAREBOX_2_BUILD_CUSTOM_ENV will not be set, >>> so those parts are removed. The rest should be identical for barebox-2, but if >>> you do need something else you can add something like $($(PKG)_EXTRA_FLAGS). >> >> I agree with not needing the additional BAREBOX_2_BUILD_BAREBOXENV_CMDS and >> BAREBOX_2_INSTALL_BAREBOXENV_CMDS (why would you need to install bareboxenv to >> the rootfs twice), but I would like to keep the BAREBOX_2_BUILD_CUSTOM_ENV and >> the built-in variant in Yegor's pending patch-set. They allow customization of >> the barebox environment, and therefore the boot behavior. > > I am having some trouble with this. I am not able to handle ifdef-space > diversity in this way. Ifdef-space diversity is used to determine how barebox > should be configured, whether / how the barebox environment should be built, > and to print user-friendly config errors. The ifdef logic is not all trivial > (almost half of the makefile logic), so I would like to avoid duplicating it > between barebox and barebox-2. The only way I know of to avoid this duplication > is by extracting the logic info a definition and using $(eval), which is > exactly what you want to avoid. Ah yes, I didn't realize that. Indeed, for those things there is no way to handle them with $(PKG). Actually I would like those things to move to kconfig-package, but that would be dragging things a bit too far for this series. So yes, your idea of $(call barebox-package) looks good. One small remark that you can already fix before submitting: in the patch that adds the barebox-package function, say explicitly in the commit message if anything has changed except for $ -> $$ and BAREBOX -> $(1). That'll make review easier. Regards, Arnout > > Also, for two of the ifdef-space diversity sections, $(PKG) seems to be > undefined at the time of evaluation. Getting to the bottom of this is proving > quite taxing. I can get around this by doing an `$(eval $(call ...,BAREBOX))`, > which looks a lot like my initial suggestion. > > I am getting to the point where I am just going to duplicate the ifdef sections > and leave the consistency burden with future reviewers. I understand your > concern that some developers do not understanding $(eval) all that well, but > this is like trying to solve the problem with my shoelaces tied. > > [snip] > >>> However, this is just my opinion, other developers may see it differently, >>> hence the CC. >> >> Off course. Your opinion is held in rather high regard, so I would like to have >> at least one of the regular barebox developers on board. ;-) > > Is there anyone else we can prod to weigh in on this? Please have a look at the > templated proposal and confirm your opinion on $(eval): > https://github.com/smipi1/bbb_buildroot/tree/barebox_2nd_config_build-v4-templated. > > [snip] > > - Pieter >
On Sun, Mar 06, 2016 at 10:16:55PM +0100, Arnout Vandecappelle wrote: > On 03/05/16 14:16, Pieter Smith wrote: [snip] > > I am having some trouble with this. I am not able to handle ifdef-space > > diversity in this way. Ifdef-space diversity is used to determine how barebox > > should be configured, whether / how the barebox environment should be built, > > and to print user-friendly config errors. The ifdef logic is not all trivial > > (almost half of the makefile logic), so I would like to avoid duplicating it > > between barebox and barebox-2. The only way I know of to avoid this duplication > > is by extracting the logic info a definition and using $(eval), which is > > exactly what you want to avoid. > > Ah yes, I didn't realize that. Indeed, for those things there is no way to > handle them with $(PKG). > > Actually I would like those things to move to kconfig-package, but that would > be dragging things a bit too far for this series. > > So yes, your idea of $(call barebox-package) looks good. One small remark that > you can already fix before submitting: in the patch that adds the > barebox-package function, say explicitly in the commit message if anything has > changed except for $ -> $$ and BAREBOX -> $(1). That'll make review easier. Thanks for having a look. I will break the series up into smaller pieces to ease review: 1 - 2 as before. 3. Break out barebox-package (Explain the $ -> $$ subst). 4. Make the variable name-space configurable (Explain BAREBOX -> $(1) subst). 5. Make the package source name-space configurable (Explain BAREBOX -> $(2) subst). 6. Introduce boot/barebox-2/barebox-2.mk and Config.in with $(call barebox-package,BAREBOX_2,BAREBOX) 7. Add a defconfig for the BBB. [snip] Thank you for helping to bang this into shape. - Pieter
diff --git a/boot/barebox/Config.in b/boot/barebox/Config.in index 39cb5d2..ed120af 100644 --- a/boot/barebox/Config.in +++ b/boot/barebox/Config.in @@ -64,9 +64,52 @@ config BR2_TARGET_BAREBOX_CUSTOM_GIT_VERSION endif +config BR2_TARGET_BAREBOX_BAREBOXENV + bool "bareboxenv tool in target" + help + Install bareboxenv tool in target. + +config BR2_TARGET_BAREBOX_CUSTOM_ENV + bool "Generate an environment image" + help + Generate a custom environment image. This environment will + contain the variables and scripts to be used at boot by + barebox. + +config BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH + string "Environment path" + depends on BR2_TARGET_BAREBOX_CUSTOM_ENV + help + Path to the directory containing the custom barebox + environment. Depending on your setup, it will probably be + based on either the content of the defaultenv or + defaultenv-2 directories in the barebox source code, plus + the additions needed. The output will be an image in the + barebox devfs format, stored in the images directory, with + the same name as the directory name given here. choice - prompt "Barebox configuration" + prompt "Number of Barebox configurations" + default BR2_TARGET_BAREBOX_SINGLE_CONFIG + +config BR2_TARGET_BAREBOX_ONE_CONFIG + select BR2_TARGET_BAREBOX_1 + bool "Build 1 config" + help + Build only one barebox config. + Useful for building the traditional TPL (Tertiary Program + Loader). + +endchoice + +config BR2_TARGET_BAREBOX_1 + bool "Barebox configuration 1" + default y + +if BR2_TARGET_BAREBOX_1 + +choice + prompt "Type of configuration" default BR2_TARGET_BAREBOX_USE_DEFCONFIG config BR2_TARGET_BAREBOX_USE_DEFCONFIG @@ -78,7 +121,7 @@ config BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG endchoice config BR2_TARGET_BAREBOX_BOARD_DEFCONFIG - string "board defconfig" + string "Board defconfig" depends on BR2_TARGET_BAREBOX_USE_DEFCONFIG help Name of the board for which Barebox should be built, without @@ -97,28 +140,6 @@ config BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES A space-separated list of configuration fragment files, that will be merged to the main Barebox configuration file. -config BR2_TARGET_BAREBOX_BAREBOXENV - bool "bareboxenv tool in target" - help - Install bareboxenv tool in target. - -config BR2_TARGET_BAREBOX_CUSTOM_ENV - bool "Generate an environment image" - help - Generate a custom environment image. This environment will - contain the variables and scripts to be used at boot by - barebox. - -config BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH - string "Environment path" - depends on BR2_TARGET_BAREBOX_CUSTOM_ENV - help - Path to the directory containing the custom barebox - environment. Depending on your setup, it will probably be - based on either the content of the defaultenv or - defaultenv-2 directories in the barebox source code, plus - the additions needed. The output will be an image in the - barebox devfs format, stored in the images directory, with - the same name as the directory name given here. +endif endif diff --git a/boot/barebox/barebox-1/barebox-1.hash b/boot/barebox/barebox-1/barebox-1.hash new file mode 120000 index 0000000..b6462b8 --- /dev/null +++ b/boot/barebox/barebox-1/barebox-1.hash @@ -0,0 +1 @@ +../barebox.hash \ No newline at end of file diff --git a/boot/barebox/barebox-1/barebox-1.mk b/boot/barebox/barebox-1/barebox-1.mk new file mode 100644 index 0000000..3374ece --- /dev/null +++ b/boot/barebox/barebox-1/barebox-1.mk @@ -0,0 +1,81 @@ +################################################################################ +# +# barebox-1 +# +################################################################################ + +BAREBOX_1_VERSION = $(BAREBOX_VERSION) +BAREBOX_1_SITE = $(BAREBOX_SITE) +BAREBOX_1_SITE_METHOD = $(BAREBOX_SITE_METHOD) +BAREBOX_1_SOURCE = $(BAREBOX_SOURCE) +BAREBOX_1_DEPENDENCIES = $(BAREBOX_DEPENDENCIES) +BAREBOX_1_LICENSE = $(BAREBOX_LICENSE) +BAREBOX_1_LICENSE_FILES = $(BAREBOX_LICENSE_FILES) +BAREBOX_1_POST_PATCH_HOOKS += $(BAREBOX_POST_PATCH_HOOKS) +BAREBOX_1_MAKE_FLAGS = $(BAREBOX_MAKE_FLAGS) +BAREBOX_1_MAKE_ENV = $(BAREBOX_MAKE_ENV) +BAREBOX_1_INSTALL_IMAGES = $(BAREBOX_INSTALL_IMAGES) + +ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y) +BAREBOX_1_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig +else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y) +BAREBOX_1_KCONFIG_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)) +endif + +BAREBOX_1_KCONFIG_FRAGMENT_FILES = $(BAREBOX_KCONFIG_FRAGMENT_FILES) +BAREBOX_1_KCONFIG_EDITORS = $(BAREBOX_KCONFIG_EDITORS) +BAREBOX_1_KCONFIG_OPTS = $(BAREBOX_MAKE_FLAGS) + +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) +define BAREBOX_1_BUILD_BAREBOXENV_CMDS + $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -o $(@D)/bareboxenv \ + $(@D)/scripts/bareboxenv.c +endef +endif + +ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y) +BAREBOX_1_ENV_NAME = $(notdir $(call qstrip,\ + $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH))) +define BAREBOX_1_BUILD_CUSTOM_ENV + $(@D)/scripts/bareboxenv -s \ + $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)) \ + $(@D)/$(BAREBOX_1_ENV_NAME) +endef +define BAREBOX_1_INSTALL_CUSTOM_ENV + cp $(@D)/$(BAREBOX_1_ENV_NAME) $(BINARIES_DIR) +endef +endif + +define BAREBOX_1_BUILD_CMDS + $(BAREBOX_1_BUILD_BAREBOXENV_CMDS) + $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_1_MAKE_FLAGS) -C $(@D) + $(BAREBOX_1_BUILD_CUSTOM_ENV) +endef + +define BAREBOX_1_INSTALL_IMAGES_CMDS + if test -h $(@D)/barebox-flash-image ; then \ + cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ + else \ + cp $(@D)/barebox.bin $(BINARIES_DIR);\ + fi + $(BAREBOX_1_INSTALL_CUSTOM_ENV) +endef + +ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) +define BAREBOX_1_INSTALL_TARGET_CMDS + cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin +endef +endif + +# Checks to give errors that the user can understand +# Must be before we call to kconfig-package +ifeq ($(BR2_TARGET_BAREBOX_2)$(BR_BUILDING),yy) +# We must use the user-supplied kconfig value, because +# BAREBOX_1_KCONFIG_DEFCONFIG will at least contain the +# trailing _defconfig +ifeq ($(or $(BAREBOX_1_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) +$(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) +endif +endif + +$(eval $(kconfig-package)) diff --git a/boot/barebox/barebox.mk b/boot/barebox/barebox.mk index 7715daf..55bd187 100644 --- a/boot/barebox/barebox.mk +++ b/boot/barebox/barebox.mk @@ -55,66 +55,7 @@ endif BAREBOX_MAKE_FLAGS = ARCH=$(BAREBOX_ARCH) CROSS_COMPILE="$(TARGET_CROSS)" BAREBOX_MAKE_ENV = $(TARGET_MAKE_ENV) -ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y) -BAREBOX_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig -else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y) -BAREBOX_KCONFIG_FILE = $(call qstrip,$(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)) -endif - BAREBOX_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_TARGET_BAREBOX_CONFIG_FRAGMENT_FILES)) BAREBOX_KCONFIG_EDITORS = menuconfig xconfig gconfig nconfig -BAREBOX_KCONFIG_OPTS = $(BAREBOX_MAKE_FLAGS) - -ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) -define BAREBOX_BUILD_BAREBOXENV_CMDS - $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -o $(@D)/bareboxenv \ - $(@D)/scripts/bareboxenv.c -endef -endif - -ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y) -BAREBOX_ENV_NAME = $(notdir $(call qstrip,\ - $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH))) -define BAREBOX_BUILD_CUSTOM_ENV - $(@D)/scripts/bareboxenv -s \ - $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)) \ - $(@D)/$(BAREBOX_ENV_NAME) -endef -define BAREBOX_INSTALL_CUSTOM_ENV - cp $(@D)/$(BAREBOX_ENV_NAME) $(BINARIES_DIR) -endef -endif - -define BAREBOX_BUILD_CMDS - $(BAREBOX_BUILD_BAREBOXENV_CMDS) - $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) - $(BAREBOX_BUILD_CUSTOM_ENV) -endef - -define BAREBOX_INSTALL_IMAGES_CMDS - if test -h $(@D)/barebox-flash-image ; then \ - cp -L $(@D)/barebox-flash-image $(BINARIES_DIR)/barebox.bin ; \ - else \ - cp $(@D)/barebox.bin $(BINARIES_DIR);\ - fi - $(BAREBOX_INSTALL_CUSTOM_ENV) -endef - -ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y) -define BAREBOX_INSTALL_TARGET_CMDS - cp $(@D)/bareboxenv $(TARGET_DIR)/usr/bin -endef -endif - -# Checks to give errors that the user can understand -# Must be before we call to kconfig-package -ifeq ($(BR2_TARGET_BAREBOX)$(BR_BUILDING),yy) -# We must use the user-supplied kconfig value, because -# BAREBOX_KCONFIG_DEFCONFIG will at least contain the -# trailing _defconfig -ifeq ($(or $(BAREBOX_KCONFIG_FILE),$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))),) -$(error No Barebox config. Check your BR2_TARGET_BAREBOX_BOARD_DEFCONFIG or BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE settings) -endif -endif -$(eval $(kconfig-package)) +include $(sort $(wildcard boot/barebox/*/*.mk))
No functional changes, but prepares barebox for building with two configurations (similar to package/gcc). Signed-off-by: Pieter Smith <pieter@boesman.nl> --- boot/barebox/Config.in | 71 +++++++++++++++++++----------- boot/barebox/barebox-1/barebox-1.hash | 1 + boot/barebox/barebox-1/barebox-1.mk | 81 +++++++++++++++++++++++++++++++++++ boot/barebox/barebox.mk | 61 +------------------------- 4 files changed, 129 insertions(+), 85 deletions(-) create mode 120000 boot/barebox/barebox-1/barebox-1.hash create mode 100644 boot/barebox/barebox-1/barebox-1.mk