Config.mk | 6 ------ xen/Makefile | 2 ++ xen/common/Kconfig | 7 +++++-- 3 files changed, 7 insertions(+), 8 deletions(-)
This will help prevent the CI loop from having build failures when
`checkpolicy` isn't available, when doing "randconfig" jobs.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Config.mk | 6 ------
xen/Makefile | 2 ++
xen/common/Kconfig | 7 +++++--
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/Config.mk b/Config.mk
index d08fa8d60dd7..97d3633706b3 100644
--- a/Config.mk
+++ b/Config.mk
@@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y
build_id_linker := --build-id=sha1
endif
-ifndef XEN_HAS_CHECKPOLICY
- CHECKPOLICY ?= checkpolicy
- XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n)
- export XEN_HAS_CHECKPOLICY
-endif
-
define buildmakevars2shellvars
export PREFIX="$(prefix)"; \
export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)"; \
diff --git a/xen/Makefile b/xen/Makefile
index 8023680ffbf2..a60e49903d0c 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname)
PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python)
export PYTHON ?= $(PYTHON_INTERPRETER)
+export CHECKPOLICY ?= checkpolicy
+
export BASEDIR := $(CURDIR)
export XEN_ROOT := $(BASEDIR)/..
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11af3..13537e460b8f 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -25,6 +25,9 @@ config GRANT_TABLE
config HAS_ALTERNATIVE
bool
+config HAS_CHECKPOLICY
+ def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
+
config HAS_COMPAT
bool
@@ -235,8 +238,8 @@ config XSM_FLASK_AVC_STATS
config XSM_FLASK_POLICY
bool "Compile Xen with a built-in FLASK security policy"
- default y if "$(XEN_HAS_CHECKPOLICY)" = "y"
- depends on XSM_FLASK
+ default y
+ depends on XSM_FLASK && HAS_CHECKPOLICY
---help---
This includes a default XSM policy in the hypervisor so that the
bootloader does not need to load a policy to get sane behavior from an
--
Anthony PERARD
On Wed, Jul 14, 2021 at 12:23 PM Anthony PERARD <anthony.perard@citrix.com> wrote: > > This will help prevent the CI loop from having build failures when > `checkpolicy` isn't available, when doing "randconfig" jobs. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
On 14/07/2021 17:17, Anthony PERARD wrote: > This will help prevent the CI loop from having build failures when > `checkpolicy` isn't available, when doing "randconfig" jobs. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Thankyou for doing this. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 14.07.2021 18:17, Anthony PERARD wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -25,6 +25,9 @@ config GRANT_TABLE > config HAS_ALTERNATIVE > bool > > +config HAS_CHECKPOLICY > + def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) > + This is no different from other aspects of "Kconfig vs tool chain capabilities" sent out last August to start a discussion about whether we really want such. Besides Jürgen no-one cared to reply iirc, which to me means no-one really cares one way or the other. Which I didn't think was the case ... So here we are again, with all the same questions still open. I'm not going to nack the patch, because there's an immediate purpose / need, but I also can't avoid commenting (and I won't put my name on it in any positive way, i.e. also not as a committer; if anything then to record my reservations). Independent of this I'd like to raise the question of whether the chosen placement is optimal. Other capability checks live in xen/Kconfig. Jan
On Thu, Jul 15, 2021 at 08:25:31AM +0200, Jan Beulich wrote: > On 14.07.2021 18:17, Anthony PERARD wrote: > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -25,6 +25,9 @@ config GRANT_TABLE > > config HAS_ALTERNATIVE > > bool > > > > +config HAS_CHECKPOLICY > > + def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) > > + > > This is no different from other aspects of "Kconfig vs tool chain > capabilities" sent out last August to start a discussion about > whether we really want such. Besides Jürgen no-one cared to reply > iirc, which to me means no-one really cares one way or the other. > Which I didn't think was the case ... So here we are again, with > all the same questions still open. It's true, I don't really care either way. But with maybe a slight preference for testing the environment every time `make` is run. But there weren't really a precedent for testing in Makefile and using the result in Kconfig (or I don't think there is). > I'm not going to nack the patch, because there's an immediate > purpose / need, but I also can't avoid commenting (and I won't > put my name on it in any positive way, i.e. also not as a > committer; if anything then to record my reservations). I've prepared an update which test in Makefile, which I hope you'll like better. Thanks, -- Anthony PERARD
On 15/07/2021 07:25, Jan Beulich wrote: > On 14.07.2021 18:17, Anthony PERARD wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -25,6 +25,9 @@ config GRANT_TABLE >> config HAS_ALTERNATIVE >> bool >> >> +config HAS_CHECKPOLICY >> + def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) >> + > This is no different from other aspects of "Kconfig vs tool chain > capabilities" sent out last August to start a discussion about > whether we really want such. Besides Jürgen no-one cared to reply > iirc, which to me means no-one really cares one way or the other. You know full well that upgrading Kconfig was specifically to be able to use this functionality, and you know full well that I firmly support using this mechanism, because we've had both of these arguments several times before. The absence of replies doesn't mean people agree with you, or even that they don't care. It either means people didn't read the email, or didn't have time to reply, or didn't feel like wasting time rehashing the same arguments yet again with no hope for progress. If you really insist on refusing to features specifically intended for the improvement of our build processes, then call a vote so we can be done with the argument for once and for all. ~Andrew
On 16.07.2021 15:15, Andrew Cooper wrote: > On 15/07/2021 07:25, Jan Beulich wrote: >> On 14.07.2021 18:17, Anthony PERARD wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -25,6 +25,9 @@ config GRANT_TABLE >>> config HAS_ALTERNATIVE >>> bool >>> >>> +config HAS_CHECKPOLICY >>> + def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) >>> + >> This is no different from other aspects of "Kconfig vs tool chain >> capabilities" sent out last August to start a discussion about >> whether we really want such. Besides Jürgen no-one cared to reply >> iirc, which to me means no-one really cares one way or the other. > > You know full well that upgrading Kconfig was specifically to be able to > use this functionality, and you know full well that I firmly support > using this mechanism, because we've had both of these arguments several > times before. > > The absence of replies doesn't mean people agree with you, or even that > they don't care. It either means people didn't read the email, or > didn't have time to reply, or didn't feel like wasting time rehashing > the same arguments yet again with no hope for progress. > > > If you really insist on refusing to features specifically intended for > the improvement of our build processes, then call a vote so we can be > done with the argument for once and for all. I'm sorry Andrew, but this is not a way to discuss controversial items. Back at the time you were pointing me at a discussion at a summit that I didn't recall and hence presumably didn't attend for whatever reason. Whatever may have been the result of such a discussion imo _has_ to be under the precondition that no other contrary arguments arise. I do not recall there having been spelled out up front this specific purpose of upgrading kconfig, and even if it was spelled out, the ramifications may not have become clear until the actual first uses were proposed. It has to be possible to change views at such a point even for people who did signal agreement earlier on. Not to speak of unaware ones. Further iirc it was you who told me to start a mail thread about the issue. Which I did. And now you say "... didn't feel like wasting time rehashing the same arguments yet again with no hope for progress"? Can you please point me to a place where those "same arguments" are put down in writing, including reasons why they were either turned down or considered of less relevance? I can't help the feeling that when our opinions don't match you try to silence me by whatever means you find suitable - ignoring my input, claiming my earlier agreement, denying me any influence, etc. I'm afraid I have to say that I don't think this is a way to run a community project. The only two ways to get past my objections (or really just reservations here) are to either convince me (which you don't appear to be willing to) or to outvote me (which you haven't tried at all so far if I'm not mistaken). Jan
On Fri, Jul 16, 2021 at 02:15:28PM +0100, Andrew Cooper wrote: > On 15/07/2021 07:25, Jan Beulich wrote: > > On 14.07.2021 18:17, Anthony PERARD wrote: > >> --- a/xen/common/Kconfig > >> +++ b/xen/common/Kconfig > >> @@ -25,6 +25,9 @@ config GRANT_TABLE > >> config HAS_ALTERNATIVE > >> bool > >> > >> +config HAS_CHECKPOLICY > >> + def_bool $(success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) > >> + > > This is no different from other aspects of "Kconfig vs tool chain > > capabilities" sent out last August to start a discussion about > > whether we really want such. Besides Jürgen no-one cared to reply > > iirc, which to me means no-one really cares one way or the other. > > You know full well that upgrading Kconfig was specifically to be able to > use this functionality, and you know full well that I firmly support To be honest, I've upgraded Kconfig mostly because I needed to start somewhere with upgrading our build system to look more like Kbuild. I may have adding `CC --version` and some other CONFIG_* running shells, but I don't think anymore that is a necessary things to do, it just look cleaner. > using this mechanism, because we've had both of these arguments several > times before. I'd like to read about your (or someone else's) arguments in favor of doing more in Kconfig only, do you have links (or maybe subject, keyword) to look at? I think I understand Jan's arguments. Cheers, -- Anthony PERARD
> On Jul 14, 2021, at 5:17 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: > > This will help prevent the CI loop from having build failures when > `checkpolicy` isn't available, when doing "randconfig" jobs. Hang on, just to clarify what’s going on here. ‘randconfig’ is setting CONFIG_XSM_FLASK_POLICY in the .config file; and then when the build happens, we error out because one of the required components isn’t there. What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? Or does the randconfig test run kConfig one more time, at which point *then* the .config will be disabled? The former I think is broken; the latter I think is fine. -George
On 16.07.21 17:26, George Dunlap wrote: > > >> On Jul 14, 2021, at 5:17 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: >> >> This will help prevent the CI loop from having build failures when >> `checkpolicy` isn't available, when doing "randconfig" jobs. > > Hang on, just to clarify what’s going on here. > > ‘randconfig’ is setting CONFIG_XSM_FLASK_POLICY in the .config file; and then when the build happens, we error out because one of the required components isn’t there. > > What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? > > Or does the randconfig test run kConfig one more time, at which point *then* the .config will be disabled? > > The former I think is broken; the latter I think is fine. I still think that Kconfig should not disable explicit settings due to tools not being available. I'd be fine with trimodal settings for such parameters: - Off - On - On if tools available And the last one could even be the default. But anyone wanting to test a specific option or to enable an option should not be catched by surprise because some internal modification is requesting another tool which happens not to be available. BTW, the same applies to ./configure options for tools. I really don't like that some components are silently not built because e.g. a header file is not available. I'd rather have the possibility to tell ./configure that I want everything built and let the build fail if that is not possible. Juergen
On Fri, Jul 16, 2021 at 04:26:39PM +0100, George Dunlap wrote: > > > > On Jul 14, 2021, at 5:17 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: > > > > This will help prevent the CI loop from having build failures when > > `checkpolicy` isn't available, when doing "randconfig" jobs. > > Hang on, just to clarify what’s going on here. > > ‘randconfig’ is setting CONFIG_XSM_FLASK_POLICY in the .config file; and then when the build happens, we error out because one of the required components isn’t there. > > What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? > > Or does the randconfig test run kConfig one more time, at which point *then* the .config will be disabled? > > The former I think is broken; the latter I think is fine. That's right, Xen's build system is broken. Kconfig doesn't say whether a given .config is correct or not, it removes non existing CONFIG_* setting as well as those that are missing dependencies. This isn't new, this is the default, this is how Linux is using Kconfig. But there's a way out of that. There's an option to prevent Kconfig from updating .config, setting KCONFIG_NOSILENTUPDATE in the environment (see docs/misc/kconfig.rst). But that won't work as expected with the Xen build system because to update the config via "syncconfig" doesn't prevent the build system from building Xen (and probably fail later). If it were working, build would fail, and user would have to run "oldconfig" or other *config targets, and check the result (diff .config.old .config). Cheers, -- Anthony PERARD
On 16/07/2021 16:26, George Dunlap wrote: > >> On Jul 14, 2021, at 5:17 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: >> >> This will help prevent the CI loop from having build failures when >> `checkpolicy` isn't available, when doing "randconfig" jobs. > Hang on, just to clarify what’s going on here. > > ‘randconfig’ is setting CONFIG_XSM_FLASK_POLICY in the .config file; and then when the build happens, we error out because one of the required components isn’t there. > > What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? Yes, but that's how ~everything in the Xen and Linux build works currently. What this new version will do is produce a config/build combo, with the config reporting that CONFIG_XSM_FLASK_POLICY was not active. This is a damnsignt better than the "old" way of doing feature checks in the makefiles, where there is no trace that the build system disabled a feature because your compiler was too old. ~Andrew
On 16.07.2021 18:14, Andrew Cooper wrote: > On 16/07/2021 16:26, George Dunlap wrote: >> >>> On Jul 14, 2021, at 5:17 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: >>> >>> This will help prevent the CI loop from having build failures when >>> `checkpolicy` isn't available, when doing "randconfig" jobs. >> Hang on, just to clarify what’s going on here. >> >> ‘randconfig’ is setting CONFIG_XSM_FLASK_POLICY in the .config file; and then when the build happens, we error out because one of the required components isn’t there. >> >> What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? > > Yes, but that's how ~everything in the Xen and Linux build works currently. > > What this new version will do is produce a config/build combo, with the > config reporting that CONFIG_XSM_FLASK_POLICY was not active. > > This is a damnsignt better than the "old" way of doing feature checks in > the makefiles, where there is no trace that the build system disabled a > feature because your compiler was too old. Disabling features at build time is of course no better than disabling them at configure time. Instead, if a chose configuration cannot be built, the build should fail - ideally with an error message clearly telling people what the reason is and what they can do about it. Jan
On Fri, Jul 16, 2021 at 04:26:39PM +0100, George Dunlap wrote: > What this patch does is to make it so that if someone explicitly sets CONFIG_XSM_FLASK_POLICY=y, but doesn’t have checkpolicy, the build system will silently disable the policy behind their backs without telling them? FYI, silenty disabling unavailable config options is actually documented, in "INSTALL", section "Xen Hypervisor". ;-) Well kind of. -- Anthony PERARD
This will help prevent the CI loop from having build failures when
`checkpolicy` isn't available, when doing "randconfig" jobs.
Also, move the check out of Config.mk and into xen/ build system.
Nothing in tools/ is using that information as it's done by
./configure.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
We might want to have a new Makefile for this kind of check that
Kconfig is going to use, just to keep the main Makefile a bit cleaner.
But maybe another time, if more are comming.
v2:
- move check to Makefile
---
Config.mk | 6 ------
xen/Makefile | 4 ++++
xen/common/Kconfig | 4 ++--
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/Config.mk b/Config.mk
index d08fa8d60dd7..97d3633706b3 100644
--- a/Config.mk
+++ b/Config.mk
@@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y
build_id_linker := --build-id=sha1
endif
-ifndef XEN_HAS_CHECKPOLICY
- CHECKPOLICY ?= checkpolicy
- XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n)
- export XEN_HAS_CHECKPOLICY
-endif
-
define buildmakevars2shellvars
export PREFIX="$(prefix)"; \
export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)"; \
diff --git a/xen/Makefile b/xen/Makefile
index 8023680ffbf2..045ddb18ad68 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname)
PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python)
export PYTHON ?= $(PYTHON_INTERPRETER)
+export CHECKPOLICY ?= checkpolicy
+
export BASEDIR := $(CURDIR)
export XEN_ROOT := $(BASEDIR)/..
@@ -156,6 +158,8 @@ CFLAGS += $(CLANG_FLAGS)
export CLANG_FLAGS
endif
+export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
+
export root-make-done := y
endif # root-make-done
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11af3..a5ef3814f531 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -235,8 +235,8 @@ config XSM_FLASK_AVC_STATS
config XSM_FLASK_POLICY
bool "Compile Xen with a built-in FLASK security policy"
- default y if "$(XEN_HAS_CHECKPOLICY)" = "y"
- depends on XSM_FLASK
+ default y
+ depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"
---help---
This includes a default XSM policy in the hypervisor so that the
bootloader does not need to load a policy to get sane behavior from an
--
Anthony PERARD
On 16.07.2021 14:38, Anthony PERARD wrote: > This will help prevent the CI loop from having build failures when > `checkpolicy` isn't available, when doing "randconfig" jobs. > > Also, move the check out of Config.mk and into xen/ build system. > Nothing in tools/ is using that information as it's done by > ./configure. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > We might want to have a new Makefile for this kind of check that > Kconfig is going to use, just to keep the main Makefile a bit cleaner. > But maybe another time, if more are comming. > > v2: > - move check to Makefile I'm afraid I don't understand: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname) > PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python) > export PYTHON ?= $(PYTHON_INTERPRETER) > > +export CHECKPOLICY ?= checkpolicy > + > export BASEDIR := $(CURDIR) > export XEN_ROOT := $(BASEDIR)/.. > > @@ -156,6 +158,8 @@ CFLAGS += $(CLANG_FLAGS) > export CLANG_FLAGS > endif > > +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) While the setting indeed gets obtained in a Makefile now, ... > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -235,8 +235,8 @@ config XSM_FLASK_AVC_STATS > > config XSM_FLASK_POLICY > bool "Compile Xen with a built-in FLASK security policy" > - default y if "$(XEN_HAS_CHECKPOLICY)" = "y" > - depends on XSM_FLASK > + default y > + depends on XSM_FLASK && "$(HAS_CHECKPOLICY)" ... it's still used as a Kconfig dependency. This in particular does not address George's concern about a setting silently getting turned off behind the back of the person having enabled it (and this could happen at any time, not just during the initial build, where one might still remember to diff .config against .config.old). The minimal thing imo is some kind of warning then. Even better would be if the setting was left unchanged and the build would fail; a solution for randconfig would then still be needed of course. If we wanted to go this route, a tristate type for the values may be unavoidable, along the lines of what Jürgen has suggested. I wouldn't think of a global override though, but a distinction of the origin of each option's setting. This might be as simple as y vs Y for "positive" values and "# CONFIG_... is not set" present (for visible options) or absent (for options the user can't control) for "negative" ones. But yes, this would likely be an intrusive change _and_ it would not be clear how to transform existing .config-s, so is unlikely to be suitable for the immediate issue at hand. Jan
On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote: > On 16.07.2021 14:38, Anthony PERARD wrote: > > +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) > > While the setting indeed gets obtained in a Makefile now, ... > > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -235,8 +235,8 @@ config XSM_FLASK_AVC_STATS > > > > config XSM_FLASK_POLICY > > bool "Compile Xen with a built-in FLASK security policy" > > - default y if "$(XEN_HAS_CHECKPOLICY)" = "y" > > - depends on XSM_FLASK > > + default y > > + depends on XSM_FLASK && "$(HAS_CHECKPOLICY)" > > ... it's still used as a Kconfig dependency. This in particular > does not address George's concern about a setting silently getting > turned off behind the back of the person having enabled it (and This patch v2 wasn't meant to address George's concern which didn't exist at the time this v2 was sent... I was trying to address yours. But it seems that "George's concern" is part of your issues with Kconfig too, which I missed when trying to right this v2. Anyway, those two patches are the only way I'm going to try to fix the random build failure in the GitLab CI, I'm not going to try to fix issues with the use of Kconfig for now. In the mean time either v1 or v2 is committed, or will just keep getting random build failure in the GitLab CI. -- Anthony PERARD
On 19.07.2021 12:47, Anthony PERARD wrote: > On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote: >> On 16.07.2021 14:38, Anthony PERARD wrote: >>> +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) >> >> While the setting indeed gets obtained in a Makefile now, ... >> >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -235,8 +235,8 @@ config XSM_FLASK_AVC_STATS >>> >>> config XSM_FLASK_POLICY >>> bool "Compile Xen with a built-in FLASK security policy" >>> - default y if "$(XEN_HAS_CHECKPOLICY)" = "y" >>> - depends on XSM_FLASK >>> + default y >>> + depends on XSM_FLASK && "$(HAS_CHECKPOLICY)" >> >> ... it's still used as a Kconfig dependency. This in particular >> does not address George's concern about a setting silently getting >> turned off behind the back of the person having enabled it (and > > This patch v2 wasn't meant to address George's concern which didn't > exist at the time this v2 was sent... I was trying to address yours. > > But it seems that "George's concern" is part of your issues with > Kconfig too, which I missed when trying to right this v2. > > Anyway, those two patches are the only way I'm going to try to fix the > random build failure in the GitLab CI, I'm not going to try to fix > issues with the use of Kconfig for now. In the mean time either v1 or v2 > is committed, or will just keep getting random build failure in the > GitLab CI. Fair enough. I actually think that randconfig shouldn't act quite as randomly as it does. But what's sensible as behavior there really depends heavily on the future intentions with .config. If we follow Linux'es model (which Andrew advocates for), its randomness would be limited by options which could get randomly set getting further altered by environmental conditions. Hence that would limit what can actually be tested, but it would avoid failures resulting from the environment not matching the chose settings. Otoh with our current model (largely, leaving aside the few environment checks we've already got) what is being asked for is what is going to get built. But failure from environmental constraints shouldn't be treated the same as failure from bad interaction of options; it's (aiui) the latter which randconfig is supposed to point out. Jan
On 19 Jul 2021, at 12:04, Jan Beulich <jbeulich@suse.com<mailto:jbeulich@suse.com>> wrote: On 19.07.2021 12:47, Anthony PERARD wrote: On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote: On 16.07.2021 14:38, Anthony PERARD wrote: +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) While the setting indeed gets obtained in a Makefile now, ... --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -235,8 +235,8 @@ config XSM_FLASK_AVC_STATS config XSM_FLASK_POLICY bool "Compile Xen with a built-in FLASK security policy" - default y if "$(XEN_HAS_CHECKPOLICY)" = "y" - depends on XSM_FLASK + default y + depends on XSM_FLASK && "$(HAS_CHECKPOLICY)" ... it's still used as a Kconfig dependency. This in particular does not address George's concern about a setting silently getting turned off behind the back of the person having enabled it (and This patch v2 wasn't meant to address George's concern which didn't exist at the time this v2 was sent... I was trying to address yours. But it seems that "George's concern" is part of your issues with Kconfig too, which I missed when trying to right this v2. Anyway, those two patches are the only way I'm going to try to fix the random build failure in the GitLab CI, I'm not going to try to fix issues with the use of Kconfig for now. In the mean time either v1 or v2 is committed, or will just keep getting random build failure in the GitLab CI. Fair enough. I actually think that randconfig shouldn't act quite as randomly as it does. It sounds like randconfig currently works appropriately for how KConfig is meant to work. That is, for good or for ill, KConfig seems designed to silently discard options with missing dependencies (either internal or external); so generating a config with missing dependencies to hand to it is “sensible”. But what's sensible as behavior there really depends heavily on the future intentions with .config. If we follow Linux'es model (which Andrew advocates for), its randomness would be limited by options which could get randomly set getting further altered by environmental conditions. Hence that would limit what can actually be tested, but it would avoid failures resulting from the environment not matching the chose settings. Otoh with our current model (largely, leaving aside the few environment checks we've already got) what is being asked for is what is going to get built. But failure from environmental constraints shouldn't be treated the same as failure from bad interaction of options; it's (aiui) the latter which randconfig is supposed to point out. Right; so another solution to the ‘randconfig CI loop’ issue would be to have the pipeline pass (or some non-pass non-fail state if possible) when then build failed because there were missing dependencies. Maybe one way of doing that would be to have a “make check-dependencies” target that would run first. That said, since it’s basically known that “silently disabling things it can’t build" is a quirk of Kconfig, I’m less inclined to object to v1 of the patch. -George
On 16/07/2021 13:38, Anthony PERARD wrote: > This will help prevent the CI loop from having build failures when > `checkpolicy` isn't available, when doing "randconfig" jobs. > > Also, move the check out of Config.mk and into xen/ build system. > Nothing in tools/ is using that information as it's done by > ./configure. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > We might want to have a new Makefile for this kind of check that > Kconfig is going to use, just to keep the main Makefile a bit cleaner. > But maybe another time, if more are comming. I won't nack this patch, but I'm a very firm -2 against it. v1 is the correct way to go. ~Andrew
© 2016 - 2024 Red Hat, Inc.