| Message ID | 20231025082834.31680-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.19] build: Allow setting KBUILD_DEFCONFIG in the environment | expand |
On 25.10.2023 10:28, Michal Orzel wrote: > At the moment, in order to use a different defconfig target than default, > one needs to specify KBUILD_DEFCONFIG=<target> on the command line. > Switch to weak assignment, so that it can be also obtained from > environment similar to other KCONFIG/KBUILD variables. > > This change will activate the use of KBUILD_DEFCONFIG variable in CI > build jobs that so far had no effect. I'm certainly okay with the change, but the above sentence looks misleading to me: Yes, the envvar was ignored so far, but isn't it the case that the envvar as specified in CI matches what Makefile set it to (taking into account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and hence the specifications in build.yaml could be dropped (until such time where truly an override was intended)? Jan > Note, that we will deviate from Linux in this regard. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > In case of reluctance to this approach, we can always do: > make -C xen defconfig KBUILD_DEFCONFIG=${KBUILD_DEFCONFIG} > in automation/scripts/build. > > Also, Linux defers setting the variable to arch specific Makefile, so it is not > sth mandated by the top level Makefile or documentation. > --- > xen/Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/Makefile b/xen/Makefile > index fd0e63d29efb..40feefff4ab5 100644 > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -256,7 +256,8 @@ export YACC = $(if $(BISON),$(BISON),bison) > export LEX = $(if $(FLEX),$(FLEX),flex) > > # Default file for 'make defconfig'. > -export KBUILD_DEFCONFIG := $(ARCH)_defconfig > +# May be overruled on the command line or set in the environment. > +export KBUILD_DEFCONFIG ?= $(ARCH)_defconfig > > # Copy CFLAGS generated by "Config.mk" so they can be reused later without > # reparsing Config.mk by e.g. arch/x86/boot/.
Hi, On 25/10/2023 11:10, Jan Beulich wrote: > > > On 25.10.2023 10:28, Michal Orzel wrote: >> At the moment, in order to use a different defconfig target than default, >> one needs to specify KBUILD_DEFCONFIG=<target> on the command line. >> Switch to weak assignment, so that it can be also obtained from >> environment similar to other KCONFIG/KBUILD variables. >> >> This change will activate the use of KBUILD_DEFCONFIG variable in CI >> build jobs that so far had no effect. > > I'm certainly okay with the change, but the above sentence looks misleading > to me: Yes, the envvar was ignored so far, but isn't it the case that the > envvar as specified in CI matches what Makefile set it to (taking into > account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and > hence the specifications in build.yaml could be dropped (until such time > where truly an override was intended)? Well, today riscv64_defconfig matches tiny64_defconfig but it can change. Otherwise, why would we need to have 2 identical files? Looking at the latest full build series from Oleksi, only the tiny64_defconfig file gets updated which would be the clear indication that what is specified in the CI matches the author's expectation. Also, I never mentioned that this change fixes something. I just wrote that it gives a meaning to a variable that so far had no effect. ~Michal
On 25.10.2023 11:21, Michal Orzel wrote: > On 25/10/2023 11:10, Jan Beulich wrote: >> On 25.10.2023 10:28, Michal Orzel wrote: >>> At the moment, in order to use a different defconfig target than default, >>> one needs to specify KBUILD_DEFCONFIG=<target> on the command line. >>> Switch to weak assignment, so that it can be also obtained from >>> environment similar to other KCONFIG/KBUILD variables. >>> >>> This change will activate the use of KBUILD_DEFCONFIG variable in CI >>> build jobs that so far had no effect. >> >> I'm certainly okay with the change, but the above sentence looks misleading >> to me: Yes, the envvar was ignored so far, but isn't it the case that the >> envvar as specified in CI matches what Makefile set it to (taking into >> account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and >> hence the specifications in build.yaml could be dropped (until such time >> where truly an override was intended)? > Well, today riscv64_defconfig matches tiny64_defconfig but it can change. Otherwise, why > would we need to have 2 identical files? Looking at the latest full build series from Oleksi, > only the tiny64_defconfig file gets updated which would be the clear indication that what is > specified in the CI matches the author's expectation. > > Also, I never mentioned that this change fixes something. I just wrote that it gives a meaning > to a variable that so far had no effect. Well, sure, but if you e.g. said "... that so far would have had no effect if they didn't match the default anyway", things would have been unambiguous. Jan
On 25/10/2023 11:26, Jan Beulich wrote: > > > On 25.10.2023 11:21, Michal Orzel wrote: >> On 25/10/2023 11:10, Jan Beulich wrote: >>> On 25.10.2023 10:28, Michal Orzel wrote: >>>> At the moment, in order to use a different defconfig target than default, >>>> one needs to specify KBUILD_DEFCONFIG=<target> on the command line. >>>> Switch to weak assignment, so that it can be also obtained from >>>> environment similar to other KCONFIG/KBUILD variables. >>>> >>>> This change will activate the use of KBUILD_DEFCONFIG variable in CI >>>> build jobs that so far had no effect. >>> >>> I'm certainly okay with the change, but the above sentence looks misleading >>> to me: Yes, the envvar was ignored so far, but isn't it the case that the >>> envvar as specified in CI matches what Makefile set it to (taking into >>> account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and >>> hence the specifications in build.yaml could be dropped (until such time >>> where truly an override was intended)? >> Well, today riscv64_defconfig matches tiny64_defconfig but it can change. Otherwise, why >> would we need to have 2 identical files? Looking at the latest full build series from Oleksi, >> only the tiny64_defconfig file gets updated which would be the clear indication that what is >> specified in the CI matches the author's expectation. >> >> Also, I never mentioned that this change fixes something. I just wrote that it gives a meaning >> to a variable that so far had no effect. > > Well, sure, but if you e.g. said "... that so far would have had no effect > if they didn't match the default anyway", things would have been unambiguous. Ok, I can see you did not provide any tag in which case I will wait for other's feedback. Then, I can either respin the patch adding sentence you suggested or leave it to Stefano to do when committing to his for-4.19 branch. ~Michal
On 25.10.2023 11:35, Michal Orzel wrote: > > > On 25/10/2023 11:26, Jan Beulich wrote: >> >> >> On 25.10.2023 11:21, Michal Orzel wrote: >>> On 25/10/2023 11:10, Jan Beulich wrote: >>>> On 25.10.2023 10:28, Michal Orzel wrote: >>>>> At the moment, in order to use a different defconfig target than default, >>>>> one needs to specify KBUILD_DEFCONFIG=<target> on the command line. >>>>> Switch to weak assignment, so that it can be also obtained from >>>>> environment similar to other KCONFIG/KBUILD variables. >>>>> >>>>> This change will activate the use of KBUILD_DEFCONFIG variable in CI >>>>> build jobs that so far had no effect. >>>> >>>> I'm certainly okay with the change, but the above sentence looks misleading >>>> to me: Yes, the envvar was ignored so far, but isn't it the case that the >>>> envvar as specified in CI matches what Makefile set it to (taking into >>>> account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and >>>> hence the specifications in build.yaml could be dropped (until such time >>>> where truly an override was intended)? >>> Well, today riscv64_defconfig matches tiny64_defconfig but it can change. Otherwise, why >>> would we need to have 2 identical files? Looking at the latest full build series from Oleksi, >>> only the tiny64_defconfig file gets updated which would be the clear indication that what is >>> specified in the CI matches the author's expectation. >>> >>> Also, I never mentioned that this change fixes something. I just wrote that it gives a meaning >>> to a variable that so far had no effect. >> >> Well, sure, but if you e.g. said "... that so far would have had no effect >> if they didn't match the default anyway", things would have been unambiguous. > Ok, I can see you did not provide any tag in which case I will wait for other's feedback. > Then, I can either respin the patch adding sentence you suggested or leave it to Stefano > to do when committing to his for-4.19 branch. The reason I didn't offer A-b (yet) is that with the given description plus the claim on Matrix by someone that things don't work because of this override not working, it wasn't really clear to me whether that claim was wrong, or whether my view of the situation is. In the latter case I could hardly ack the patch, as that would then mean I'd ack something I don't understand. Provided there really has not been any breakage so far because of this, feel free to add Acked-by: Jan Beulich <jbeulich@suse.com> preferably with the slightly adjusted description. Jan
On Wed, 25 Oct 2023, Jan Beulich wrote: > On 25.10.2023 11:35, Michal Orzel wrote: > > > > > > On 25/10/2023 11:26, Jan Beulich wrote: > >> > >> > >> On 25.10.2023 11:21, Michal Orzel wrote: > >>> On 25/10/2023 11:10, Jan Beulich wrote: > >>>> On 25.10.2023 10:28, Michal Orzel wrote: > >>>>> At the moment, in order to use a different defconfig target than default, > >>>>> one needs to specify KBUILD_DEFCONFIG=<target> on the command line. > >>>>> Switch to weak assignment, so that it can be also obtained from > >>>>> environment similar to other KCONFIG/KBUILD variables. > >>>>> > >>>>> This change will activate the use of KBUILD_DEFCONFIG variable in CI > >>>>> build jobs that so far had no effect. > >>>> > >>>> I'm certainly okay with the change, but the above sentence looks misleading > >>>> to me: Yes, the envvar was ignored so far, but isn't it the case that the > >>>> envvar as specified in CI matches what Makefile set it to (taking into > >>>> account that for RISC-V riscv64_defconfig aliases tiny64_defconfig), and > >>>> hence the specifications in build.yaml could be dropped (until such time > >>>> where truly an override was intended)? > >>> Well, today riscv64_defconfig matches tiny64_defconfig but it can change. Otherwise, why > >>> would we need to have 2 identical files? Looking at the latest full build series from Oleksi, > >>> only the tiny64_defconfig file gets updated which would be the clear indication that what is > >>> specified in the CI matches the author's expectation. > >>> > >>> Also, I never mentioned that this change fixes something. I just wrote that it gives a meaning > >>> to a variable that so far had no effect. > >> > >> Well, sure, but if you e.g. said "... that so far would have had no effect > >> if they didn't match the default anyway", things would have been unambiguous. > > Ok, I can see you did not provide any tag in which case I will wait for other's feedback. > > Then, I can either respin the patch adding sentence you suggested or leave it to Stefano > > to do when committing to his for-4.19 branch. > > The reason I didn't offer A-b (yet) is that with the given description plus > the claim on Matrix by someone that things don't work because of this > override not working, it wasn't really clear to me whether that claim was > wrong, or whether my view of the situation is. In the latter case I could > hardly ack the patch, as that would then mean I'd ack something I don't > understand. Provided there really has not been any breakage so far because > of this, feel free to add > Acked-by: Jan Beulich <jbeulich@suse.com> > preferably with the slightly adjusted description. Added to for-4.19 with the adjusted commit message
diff --git a/xen/Makefile b/xen/Makefile index fd0e63d29efb..40feefff4ab5 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -256,7 +256,8 @@ export YACC = $(if $(BISON),$(BISON),bison) export LEX = $(if $(FLEX),$(FLEX),flex) # Default file for 'make defconfig'. -export KBUILD_DEFCONFIG := $(ARCH)_defconfig +# May be overruled on the command line or set in the environment. +export KBUILD_DEFCONFIG ?= $(ARCH)_defconfig # Copy CFLAGS generated by "Config.mk" so they can be reused later without # reparsing Config.mk by e.g. arch/x86/boot/.
At the moment, in order to use a different defconfig target than default, one needs to specify KBUILD_DEFCONFIG=<target> on the command line. Switch to weak assignment, so that it can be also obtained from environment similar to other KCONFIG/KBUILD variables. This change will activate the use of KBUILD_DEFCONFIG variable in CI build jobs that so far had no effect. Note, that we will deviate from Linux in this regard. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- In case of reluctance to this approach, we can always do: make -C xen defconfig KBUILD_DEFCONFIG=${KBUILD_DEFCONFIG} in automation/scripts/build. Also, Linux defers setting the variable to arch specific Makefile, so it is not sth mandated by the top level Makefile or documentation. --- xen/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)