Message ID | 06c9840e-2577-dc81-c0ab-f9301b5bcce7@mojix.com |
---|---|
State | Rejected |
Headers | show |
Series | [1/1] core: load a global.mk file if it exists to override global vars | expand |
Hi Florent, Thank you for this contribution. I'm going to reject it, but it's very useful to have a discussion about it. On 17/11/2019 17:12, Florent VIARD (Mojix) wrote: > > For example, this can be useful to permanently set a > BR2_EXTERNAL without having to use the "env var" at least once. > > The global.mk file has to be located inside TOPDIR. > > Example: > -- global.mk -- > BR2_EXTERNAL = ext-myproject It would be more logical to use $(TOPDIR)/ext-myproject here. > It can also probably be used for BR2_VERSION customization or to add Why would you want to customize BR2_VERSION? > global variables to be used in custom config files. Both of those can be set in any of the external .mk files, or in local.mk. > > To give a little bit of context of the genesis of this change: > - I have a project using buildroot for a custom product. > - I have an "ext-myproject" folder at the root of TOPDIR that is an > external folder. > - All the configs, scripts or vendor specific packages are located > inside this external folder. > > Like that, the project is kind of "out-of-tree", with everything located > in a specific dir, and the buildroot original file/folders stay clean. > So, it is more easy to merge updated versions. That's not how external was expected to be used. If you include your external inside the Buildroot tree, there's little point in making it external. You could just as well create a directory package/mojix and put your custom packages there, and configs/mojix_* and board/mojix for the rest. And in this situation, you can in fact easily put the BR2_EXTERNAL = ext-myproject in the Buildroot Makefile in your repo. Merging that with an updated Buildroot is still going to be trivial. The way an external is expected to be used is as follows: - buildroot as a subdirectory (probably submodule) of the external; - external includes a Makefile or other script to call into buildroot. I'll append an example Makefile that I've used for some project. It would actually be nice to include something like that in Buildroot, and more importantly to add the above statements as proper documentation, but I've never gotten around to that... > The issue solved by this change is that, currently, path to external > folder(s) has to be manually set through the env var at least once by > everyone in the team after a checkout and eventually again after a make > distclean. Because, the BR2_EXTERNAL path settings are saved within the > "output" folder. > > Using the global.mk file, i can setup the BR2_EXTERNAL and save/commit > it with the project, so that anyone can start building with a single > "make" without non obvious manipulation. Two times make, actually: "make foo_defconfig" and "make" :-) > As a side effect of this change, using this can help overcome a > regression in > buildroot that I have noticed: > External paths are supposed to be absolute or relative, but a change > pushed a few years ago, automatically change external relative path to > absolute before generating the temporary files. Err, that sounds like the right thing to do, so what exactly is the regression? If there really is a regression, we'll want to fix it. As noted, I've marked your patch as Rejected in patchwork. If others disagree with me, we can still go back to [1] and set it to New again. > > Signed-off-by: Florent Viard <fviard@cxignited.com> The e-mail address in your Signed-off-by should be the same as the author address (i.e. the From of the mail). > --- > Makefile | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Makefile b/Makefile > index c78c2cbcf6..f2c43ef40f 100644 > --- a/Makefile > +++ b/Makefile > @@ -170,6 +170,9 @@ export CDPATH := > BASE_DIR := $(CANONICAL_O) > $(if $(BASE_DIR),, $(error output directory "$(O)" does not exist)) > > +# Include the global override file if one is provided > +# This is useful, for example, to allow the user to permanently set > BR2_EXTERNAL This was line-wrapped. Did you use git send-email? Regards, Arnout > +-include $(TOPDIR)/global.mk > > # Handling of BR2_EXTERNAL. > # [1] http://patchwork.ozlabs.org/patch/1196387/ --------- # Top-level Makefile for an external Buildroot project # # Copyright (c) 2015 Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by # the Free Software Foundation; either version 2 of the License, or # (at your option) any later version. # # This program is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU # General Public License for more details. # # You should have received a copy of the GNU General Public License # along with this program; if not, write to the Free Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA CURDIR := $(abspath $(dir $(lastword $(MAKEFILE_LIST)))) # The directory in which this Makefile resides is used as BR2_EXTERNAL export BR2_EXTERNAL := $(CURDIR) # Put downloads in this directory instead of in the Buildroot directory ifeq ($(BR2_DL_DIR),) BR2_DL_DIR = $(CURDIR)/dl endif export BR2_DL_DIR # Output directory: if called from $(CURDIR), under output/, otherwise # under the current directory ifeq ($(realpath $(CURDIR)),$(realpath .)) OUTPUT_BASEDIR = $(CURDIR)/output else OUTPUT_BASEDIR = . endif -include $(OUTPUT_BASEDIR)/currentconfig MAKE_BUILDROOT = $(MAKE) -C $(CURDIR)/buildroot O=$(OUTPUT_BASEDIR)/$(CONFIG) .PHONY: _all _all: $(MAKE_BUILDROOT) $(all) # On first run, force selecting a config ifeq ($(CONFIG),) _all: requireconfig requireconfig: $(error First select a configuration) endif # Build an existing configuration CONFIGS = $(patsubst %_defconfig,%,$(notdir $(wildcard $(CURDIR)/configs/*_defconfig))) .PHONY: $(CONFIGS) $(CONFIGS): CONFIG=$* $(CONFIGS): %: $(CURDIR)/configs/%_defconfig @mkdir -p $(OUTPUT_BASEDIR) $(MAKE_BUILDROOT) $(CONFIG)_defconfig @echo "CONFIG ?= $(CONFIG)" > $(OUTPUT_BASEDIR)/currentconfig $(MAKE_BUILDROOT) REAL_TARGETS = Makefile $(OUTPUT_BASEDIR)/currentconfig # Anything else is passed on to buildroot all := $(filter-out $(CONFIGS) $(new_targets) $(REAL_TARGETS),$(MAKECMDGOALS)) .PHONY: $(all) $(all): _all @: Makefile:; $(OUTPUT_BASEDIR)/currentconfig:;
Hi Arnout, Thank you for your message. I didn't expect a change in the core to pass easily without a discussion :-) Here are my replies to your comments: >> Example: >> -- global.mk -- >> BR2_EXTERNAL = ext-myproject > It would be more logical to use $(TOPDIR)/ext-myproject here. True, but that was just for the example to be short and non confusing, as anyway with or without $(TOPDIR) should be equivalent here. >> It can also probably be used for BR2_VERSION customization or to add > Why would you want to customize BR2_VERSION? As far as I know, there is no option for the user to define a custom version, and an os-release file with that info is always generated (hardcoded in Makefile). So, I guess, some users might be interested to tweak that for their cases without hacking the Makefile. >> global variables to be used in custom config files. > Both of those can be set in any of the external .mk files, or in local.mk. In my case, I wanted this for the EXTERNAL case, but after a long time thinking of alternative that would be specific to externals, i came to the conclusion that it was nicer and cleaner to do something generic like that. (Ideas that I had were like, creating a new config file for external, or moving the temporary .br2_external... files out of the "output" dir) To be noted, external .mk files and local.mk are loaded later in the process, and local.mk location can also be in an "out of tree" config folder. >> To give a little bit of context of the genesis of this change: >> - I have a project using buildroot for a custom product. >> - I have an "ext-myproject" folder at the root of TOPDIR that is an >> external folder. >> - All the configs, scripts or vendor specific packages are located >> inside this external folder. >> >> Like that, the project is kind of "out-of-tree", with everything located >> in a specific dir, and the buildroot original file/folders stay clean. >> So, it is more easy to merge updated versions. > That's not how external was expected to be used. If you include your external > inside the Buildroot tree, there's little point in making it external. You could > just as well create a directory package/mojix and put your custom packages > there, and configs/mojix_* and board/mojix for the rest. > > And in this situation, you can in fact easily put the BR2_EXTERNAL = > ext-myproject in the Buildroot Makefile in your repo. Merging that with an > updated Buildroot is still going to be trivial. > > The way an external is expected to be used is as follows: > > - buildroot as a subdirectory (probably submodule) of the external; > - external includes a Makefile or other script to call into buildroot. > > I'll append an example Makefile that I've used for some project. It would > actually be nice to include something like that in Buildroot, and more > importantly to add the above statements as proper documentation, but I've never > gotten around to that... I see that your are suggesting, and it is not what I was thinking external were designed for. That being said, it is just my opinion, but I think that it is very nice and convenient how I was using it. In what you suggest, there are 2 things that I don't like very much: 1) Having to hack a core element of buildroot (Makefile) to hardcode something. I think that a lambda user would be scared and not do that, and also, in 5 years, when someone else will take over my project, he might have a hard time figuring it out that there is a hack in the Makefile for the black magic to work. 2) Having my custom things spilled all other the place in configs, package, board (or subfolder of it). It has also to be taken into account that there might be other "things" on the vendor folder side: Assets, scripts, templates, ... So, I like very much the concept of overlay, where you have the buildroot itself that is kind of pristine, and the vendor things that come cleanly with one or multiple nice "external" overlays. Sure, anyone can do higher level script or Makefile, but it is nice when things kind of work directly out of the box. >> As a side effect of this change, using this can help overcome a >> regression in >> buildroot that I have noticed: >> External paths are supposed to be absolute or relative, but a change >> pushed a few years ago, automatically change external relative path to >> absolute before generating the temporary files. > Err, that sounds like the right thing to do, so what exactly is the regression? > If there really is a regression, we'll want to fix it. The documentation coming from the external feature original implementation: https://buildroot.org/downloads/manual/manual.html#outside-br-custom >> *Note. *The path to a br2-external tree can be either absolute or relative. If it is passed as a relative path, it is important to note that it is interpreted relative to the main Buildroot source directory, *not* to the Buildroot output directory. The breaking change: https://patchwork.ozlabs.org/patch/715359/ (That looks like to be a quick fix for an issue not looking at the root cause itself) Again, just my opinion, but the "absolute" path thing is quite bad. Today, you can set the external as relative, not noticing that it will be stored as absolute. You did as said in the document, and so set the external once with a call to BR2_EXTERNAL=ext-myproject make menuconfig One day in the future, you move files, like "copying a project", and the thing does not build anymore without apparent reason. And even for the case that you described, with the external out of TOPDIR, I think that in most cases it is probably cleaner to use a relative reference like: BR2_EXTERNAL=../../real-project-root than BR2_EXTERNAL=/home/marcel/tests/copy123/real-project-root > As noted, I've marked your patch as Rejected in patchwork. If others disagree > with me, we can still go back to [1] and set it to New again. > :-) >> Signed-off-by: Florent Viard <fviard@cxignited.com> >> >> The e-mail address in your Signed-off-by should be the same as the author >> address (i.e. the From of the mail). >> >> This was line-wrapped. Did you use git send-email? >> Sorry, I usually don't contribute to plain text mailing-list based project, too much a nightmare if you don't have all right conditions. I sent it with thunderbird that I tried to configure correctly, but I have the feeling that, on the way, there is the gmail server that mess with my messages. Florent
diff --git a/Makefile b/Makefile index c78c2cbcf6..f2c43ef40f 100644 --- a/Makefile +++ b/Makefile @@ -170,6 +170,9 @@ export CDPATH := BASE_DIR := $(CANONICAL_O) $(if $(BASE_DIR),, $(error output directory "$(O)" does not exist)) +# Include the global override file if one is provided +# This is useful, for example, to allow the user to permanently set BR2_EXTERNAL +-include $(TOPDIR)/global.mk # Handling of BR2_EXTERNAL.
For example, this can be useful to permanently set a BR2_EXTERNAL without having to use the "env var" at least once. The global.mk file has to be located inside TOPDIR. Example: -- global.mk -- BR2_EXTERNAL = ext-myproject It can also probably be used for BR2_VERSION customization or to add global variables to be used in custom config files. To give a little bit of context of the genesis of this change: - I have a project using buildroot for a custom product. - I have an "ext-myproject" folder at the root of TOPDIR that is an external folder. - All the configs, scripts or vendor specific packages are located inside this external folder. Like that, the project is kind of "out-of-tree", with everything located in a specific dir, and the buildroot original file/folders stay clean. So, it is more easy to merge updated versions. The issue solved by this change is that, currently, path to external folder(s) has to be manually set through the env var at least once by everyone in the team after a checkout and eventually again after a make distclean. Because, the BR2_EXTERNAL path settings are saved within the "output" folder. Using the global.mk file, i can setup the BR2_EXTERNAL and save/commit it with the project, so that anyone can start building with a single "make" without non obvious manipulation. As a side effect of this change, using this can help overcome a regression in buildroot that I have noticed: External paths are supposed to be absolute or relative, but a change pushed a few years ago, automatically change external relative path to absolute before generating the temporary files. Signed-off-by: Florent Viard <fviard@cxignited.com> --- Makefile | 3 +++ 1 file changed, 3 insertions(+) #