[PATCH RFC 0/6] Generate Kconfig environment in Ansible

[PATCH RFC 0/6] Generate Kconfig environment in Ansible

[Daniel Gomez]

This is an effort to replace the scripts/* we use from Kconfig to
retrieve any possible Kconfig value (y/n/string) with Ansible. I believe
this has been discussed a few times on the mailing list, perhaps as a
potential idea. This aims to explore whether this approach makes sense
and what are the options.

The new workflow is:
* Run make menuconfig
* menuconfig runs kconfig-env target
* kconfig-env target calls Ansible kconfig.yml playbook
* kconfig.yml playbook generates a Kconfig fragment with the environment
(.env.config)
* .env.config is merged with .config
* make olddefconfig is run to generate a valid .config for menuconfig
* menuconfig is shown with the environment generated variables

While this adds new targets, it also removes the $shell calls in
Kconfig. However, when merging fragments, it appears that hidden symbols
cannot be merged and updated in .config, unless these are visible (and
therefore editable). So for this reason, a new Environment menu has
been created to list all these new vars. Any ideas/suggestion to keep
them invisible?

Note that only CONFIG_TOPDIR_PATH_SHA256SUM= is generated with Ansible
and added/merged to the Kconfig environment menu vars.

Thoughts?

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
Daniel Gomez (6):
      merge_config: add fragment support from kernel
      scripts/kconfig/kconfig.Makefile: extend simple-targets
      kconfig-fragments: add docs and fragments folder
      kconfig-env: generate kconfig environment
      kconfig: env: add support
      playbooks: add kconfig support

 .gitignore                             |   2 +
 Kconfig                                |   9 +-
 Makefile                               |   1 +
 configs/callback-debug.config          |   2 +
 configs/callback-dense.config          |   2 +
 docs/kconfig-fragments.md              |  19 +++
 kconfigs/Kconfig.env                   |   5 +
 playbooks/kconfig.yml                  |   6 +
 playbooks/roles/kconfig/tasks/main.yml |  15 +++
 scripts/kconfig-env.Makefile           |  14 +++
 scripts/kconfig/Makefile               |   3 +
 scripts/kconfig/kconfig.Makefile       |   6 +-
 scripts/kconfig/merge_config.sh        | 213 +++++++++++++++++++++++++++++++++
 13 files changed, 289 insertions(+), 8 deletions(-)
---
base-commit: f83ea7595cc531e1c5413ffdf7f82714fc8ab7f0
change-id: 20250328-kconfig-env-with-ansible-33e3c3b3ae97

Best regards,
-- 
Daniel Gomez <da.gomez@samsung.com>

[PATCH RFC 1/6] merge_config: add fragment support from kernel

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

Add merge_config.sh script support from upstream Linux kernel.

This allows to use fragments in kdevops.

Example: A fragment to enable fstests is called fstests.config. To
enable it while keeping the same config, we can simply do:

./scripts/kconfig/merge_config.sh -m .config fstests.config
make olddefconfig

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 scripts/kconfig/merge_config.sh | 213 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 213 insertions(+)

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
new file mode 100755
index 0000000000000000000000000000000000000000..ad35a60de350ae1c5b60d39bf752115d27276f52
--- /dev/null
+++ b/scripts/kconfig/merge_config.sh
@@ -0,0 +1,213 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+#  merge_config.sh - Takes a list of config fragment values, and merges
+#  them one by one. Provides warnings on overridden values, and specified
+#  values that did not make it to the resulting .config file (due to missed
+#  dependencies or config symbol removal).
+#
+#  Portions reused from kconf_check and generate_cfg:
+#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/kconf_check
+#  http://git.yoctoproject.org/cgit/cgit.cgi/yocto-kernel-tools/tree/tools/generate_cfg
+#
+#  Copyright (c) 2009-2010 Wind River Systems, Inc.
+#  Copyright 2011 Linaro
+
+set -e
+
+clean_up() {
+	rm -f $TMP_FILE
+	rm -f $MERGE_FILE
+}
+
+usage() {
+	echo "Usage: $0 [OPTIONS] [CONFIG [...]]"
+	echo "  -h    display this help text"
+	echo "  -m    only merge the fragments, do not execute the make command"
+	echo "  -n    use allnoconfig instead of alldefconfig"
+	echo "  -r    list redundant entries when merging fragments"
+	echo "  -y    make builtin have precedence over modules"
+	echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
+	echo "  -s    strict mode. Fail if the fragment redefines any value."
+	echo "  -Q    disable warning messages for overridden options."
+	echo
+	echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
+}
+
+RUNMAKE=true
+ALLTARGET=alldefconfig
+WARNREDUN=false
+BUILTIN=false
+OUTPUT=.
+STRICT=false
+CONFIG_PREFIX=${CONFIG_-CONFIG_}
+WARNOVERRIDE=echo
+
+while true; do
+	case $1 in
+	"-n")
+		ALLTARGET=allnoconfig
+		shift
+		continue
+		;;
+	"-m")
+		RUNMAKE=false
+		shift
+		continue
+		;;
+	"-h")
+		usage
+		exit
+		;;
+	"-r")
+		WARNREDUN=true
+		shift
+		continue
+		;;
+	"-y")
+		BUILTIN=true
+		shift
+		continue
+		;;
+	"-O")
+		if [ -d $2 ];then
+			OUTPUT=$(echo $2 | sed 's/\/*$//')
+		else
+			echo "output directory $2 does not exist" 1>&2
+			exit 1
+		fi
+		shift 2
+		continue
+		;;
+	"-s")
+		STRICT=true
+		shift
+		continue
+		;;
+	"-Q")
+		WARNOVERRIDE=true
+		shift
+		continue
+		;;
+	*)
+		break
+		;;
+	esac
+done
+
+if [ "$#" -lt 1 ] ; then
+	usage
+	exit
+fi
+
+if [ -z "$KCONFIG_CONFIG" ]; then
+	if [ "$OUTPUT" != . ]; then
+		KCONFIG_CONFIG=$(readlink -m -- "$OUTPUT/.config")
+	else
+		KCONFIG_CONFIG=.config
+	fi
+fi
+
+INITFILE=$1
+shift;
+
+if [ ! -r "$INITFILE" ]; then
+	echo "The base file '$INITFILE' does not exist. Creating one..." >&2
+	touch $INITFILE
+fi
+
+MERGE_LIST=$*
+SED_CONFIG_EXP1="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*/\1/p"
+SED_CONFIG_EXP2="s/^# \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1/p"
+
+TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX)
+MERGE_FILE=$(mktemp ./.merge_tmp.config.XXXXXXXXXX)
+
+echo "Using $INITFILE as base"
+
+trap clean_up EXIT
+
+cat $INITFILE > $TMP_FILE
+
+# Merge files, printing warnings on overridden values
+for ORIG_MERGE_FILE in $MERGE_LIST ; do
+	echo "Merging $ORIG_MERGE_FILE"
+	if [ ! -r "$ORIG_MERGE_FILE" ]; then
+		echo "The merge file '$ORIG_MERGE_FILE' does not exist.  Exit." >&2
+		exit 1
+	fi
+	cat $ORIG_MERGE_FILE > $MERGE_FILE
+	CFG_LIST=$(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $MERGE_FILE)
+
+	for CFG in $CFG_LIST ; do
+		grep -q -w $CFG $TMP_FILE || continue
+		PREV_VAL=$(grep -w $CFG $TMP_FILE)
+		NEW_VAL=$(grep -w $CFG $MERGE_FILE)
+		BUILTIN_FLAG=false
+		if [ "$BUILTIN" = "true" ] && [ "${NEW_VAL#CONFIG_*=}" = "m" ] && [ "${PREV_VAL#CONFIG_*=}" = "y" ]; then
+			${WARNOVERRIDE} Previous  value: $PREV_VAL
+			${WARNOVERRIDE} New value:       $NEW_VAL
+			${WARNOVERRIDE} -y passed, will not demote y to m
+			${WARNOVERRIDE}
+			BUILTIN_FLAG=true
+		elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
+			${WARNOVERRIDE} Value of $CFG is redefined by fragment $ORIG_MERGE_FILE:
+			${WARNOVERRIDE} Previous  value: $PREV_VAL
+			${WARNOVERRIDE} New value:       $NEW_VAL
+			${WARNOVERRIDE}
+			if [ "$STRICT" = "true" ]; then
+				STRICT_MODE_VIOLATED=true
+			fi
+		elif [ "$WARNREDUN" = "true" ]; then
+			${WARNOVERRIDE} Value of $CFG is redundant by fragment $ORIG_MERGE_FILE:
+		fi
+		if [ "$BUILTIN_FLAG" = "false" ]; then
+			sed -i "/$CFG[ =]/d" $TMP_FILE
+		else
+			sed -i "/$CFG[ =]/d" $MERGE_FILE
+		fi
+	done
+	# In case the previous file lacks a new line at the end
+	echo >> $TMP_FILE
+	cat $MERGE_FILE >> $TMP_FILE
+done
+
+if [ "$STRICT_MODE_VIOLATED" = "true" ]; then
+	echo "The fragment redefined a value and strict mode had been passed."
+	exit 1
+fi
+
+if [ "$RUNMAKE" = "false" ]; then
+	cp -T -- "$TMP_FILE" "$KCONFIG_CONFIG"
+	echo "#"
+	echo "# merged configuration written to $KCONFIG_CONFIG (needs make)"
+	echo "#"
+	exit
+fi
+
+# If we have an output dir, setup the O= argument, otherwise leave
+# it blank, since O=. will create an unnecessary ./source softlink
+OUTPUT_ARG=""
+if [ "$OUTPUT" != "." ] ; then
+	OUTPUT_ARG="O=$OUTPUT"
+fi
+
+
+# Use the merged file as the starting point for:
+# alldefconfig: Fills in any missing symbols with Kconfig default
+# allnoconfig: Fills in any missing symbols with # CONFIG_* is not set
+make KCONFIG_ALLCONFIG=$TMP_FILE $OUTPUT_ARG $ALLTARGET
+
+
+# Check all specified config values took (might have missed-dependency issues)
+for CFG in $(sed -n -e "$SED_CONFIG_EXP1" -e "$SED_CONFIG_EXP2" $TMP_FILE); do
+
+	REQUESTED_VAL=$(grep -w -e "$CFG" $TMP_FILE)
+	ACTUAL_VAL=$(grep -w -e "$CFG" "$KCONFIG_CONFIG" || true)
+	if [ "x$REQUESTED_VAL" != "x$ACTUAL_VAL" ] ; then
+		echo "Value requested for $CFG not in final .config"
+		echo "Requested value:  $REQUESTED_VAL"
+		echo "Actual value:     $ACTUAL_VAL"
+		echo ""
+	fi
+done

-- 
2.47.2

[PATCH RFC 2/6] scripts/kconfig/kconfig.Makefile: extend simple-targets

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

Add oldconfig and olddefconfig targets.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 scripts/kconfig/Makefile         | 3 +++
 scripts/kconfig/kconfig.Makefile | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
index e3694365bf98281a00fb56d76d3400d236776476..7cef3498e4c9e751c05190fb3da7507587d9ad84 100644
--- a/scripts/kconfig/Makefile
+++ b/scripts/kconfig/Makefile
@@ -69,6 +69,9 @@ help:
 	@echo "allyesconfig       - enables all bells and whistles"
 	@echo "allnoconfig        - disables all bells and whistles"
 	@echo "randconfig         - random configuration"
+	@echo "oldconfig          - update a configuration using provided .config as base"
+	@echo "olddefconfig       - Same as oldconfig but sets new symbosl to their default"
+
 	@echo "defconfig-*        - If you have files in the defconfig directory use default config from there"
 
 .PHONY: clean
diff --git a/scripts/kconfig/kconfig.Makefile b/scripts/kconfig/kconfig.Makefile
index e227ea4233a7e790e90f38c1817c745a0c805132..cc24c7ff3e1c23487aec767075c69fdf3876b39d 100644
--- a/scripts/kconfig/kconfig.Makefile
+++ b/scripts/kconfig/kconfig.Makefile
@@ -51,7 +51,7 @@ $(KCONFIG_DIR)/conf:
 
 # More are supported, however we only list the ones tested on this top
 # level Makefile.
-simple-targets := allnoconfig allyesconfig alldefconfig randconfig
+simple-targets := allnoconfig allyesconfig alldefconfig randconfig oldconfig olddefconfig
 PHONY += $(simple-targets)
 
 $(simple-targets): $(KCONFIG_DIR)/conf Kconfig

-- 
2.47.2

[PATCH RFC 3/6] kconfig-fragments: add docs and fragments folder

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

Add configs/ directory to hold kdevops fragments.
Add docs/kconfig-fragments explaining the fragment workflow.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 configs/callback-debug.config |  2 ++
 configs/callback-dense.config |  2 ++
 docs/kconfig-fragments.md     | 19 +++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/configs/callback-debug.config b/configs/callback-debug.config
new file mode 100644
index 0000000000000000000000000000000000000000..880da5fcb4ec60a7ee04490f21ea2a7efa240926
--- /dev/null
+++ b/configs/callback-debug.config
@@ -0,0 +1,2 @@
+CONFIG_ANSIBLE_CFG_CALLBACK_PLUGIN_DENSE=n
+CONFIG_ANSIBLE_CFG_CALLBACK_PLUGIN_DEBUG=y
diff --git a/configs/callback-dense.config b/configs/callback-dense.config
new file mode 100644
index 0000000000000000000000000000000000000000..c59925c4bd30634633be322c9e3b24bb91d2d25b
--- /dev/null
+++ b/configs/callback-dense.config
@@ -0,0 +1,2 @@
+CONFIG_ANSIBLE_CFG_CALLBACK_PLUGIN_DENSE=y
+CONFIG_ANSIBLE_CFG_CALLBACK_PLUGIN_DEBUG=n
diff --git a/docs/kconfig-fragments.md b/docs/kconfig-fragments.md
new file mode 100644
index 0000000000000000000000000000000000000000..969a7289b8be10fbb9eb2fb96d3a28be7b5db39d
--- /dev/null
+++ b/docs/kconfig-fragments.md
@@ -0,0 +1,19 @@
+# Kconfig Fragments
+
+kdevops fragments are supported and can be used with the `scripts/kconfig/merge_config.sh` script.
+
+Workflow:
+
+```sh
+./scripts/kconfig/merge_config.sh \
+-m <INITCONF> \
+<FRAGMENT_LIST>
+```
+
+Example:
+
+```sh
+./scripts/kconfig/merge_config.sh \
+-m .config \
+callback-dense.config
+```

-- 
2.47.2

[PATCH RFC 4/6] kconfig-env: generate kconfig environment

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

When menuconfig target is run, generate first the kconfig environment
and merge with current .config.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 Makefile                         |  1 +
 scripts/kconfig-env.Makefile     | 14 ++++++++++++++
 scripts/kconfig/kconfig.Makefile |  4 ++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 5ee7db7d988b9271fed7cb470f807bb9e9ce3cb6..1f939c52284f218a23430a7c5350d7827cb1b905 100644
--- a/Makefile
+++ b/Makefile
@@ -21,6 +21,7 @@ export KDEVOPS_VAGRANT :=
 export PYTHONUNBUFFERED=1
 export TOPDIR=./
 export TOPDIR_PATH = $(shell readlink -f $(TOPDIR))
+include scripts/kconfig-env.Makefile
 include scripts/refs.Makefile
 
 KDEVOPS_NODES_ROLE_TEMPLATE_DIR :=		$(KDEVOPS_PLAYBOOKS_DIR)/roles/gen_nodes/templates
diff --git a/scripts/kconfig-env.Makefile b/scripts/kconfig-env.Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..dc2b9f2a65001ba8698a61bfb20b43b366e3d92a
--- /dev/null
+++ b/scripts/kconfig-env.Makefile
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: copyleft-next-0.3.1
+
+PHONY += kconfig-env
+kconfig-env:
+	$(Q)ANSIBLE_STDOUT_CALLBACK=null ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
+		--inventory localhost, \
+		playbooks/kconfig.yml \
+		-e 'ansible_python_interpreter=/usr/bin/python3' \
+		--extra-vars "topdir_path=$(TOPDIR_PATH)"
+	$(Q)$(TOPDIR_PATH)/scripts/kconfig/merge_config.sh -m .config \
+	$(TOPDIR_PATH)/.env.config
+	$(Q)$(MAKE) -C $(TOPDIR_PATH) olddefconfig
+
+.PHONY = $(PHONY)
diff --git a/scripts/kconfig/kconfig.Makefile b/scripts/kconfig/kconfig.Makefile
index cc24c7ff3e1c23487aec767075c69fdf3876b39d..19ed56b01db5817dc1e8b7a4db6fa726d87d8065 100644
--- a/scripts/kconfig/kconfig.Makefile
+++ b/scripts/kconfig/kconfig.Makefile
@@ -36,8 +36,8 @@ $(KCONFIG_DIR)/mconf:
 	$(MAKE) -C $(KCONFIG_DIR)/ mconf
 
 PHONY += menuconfig
-menuconfig: $(KCONFIG_DIR)/mconf include/config/project.release Kconfig
-	@$< Kconfig
+menuconfig: $(KCONFIG_DIR)/mconf include/config/project.release Kconfig kconfig-env
+	$< Kconfig
 
 $(KCONFIG_DIR)/nconf:
 	$(MAKE) -C $(KCONFIG_DIR)/ nconf

-- 
2.47.2

[PATCH RFC 5/6] kconfig: env: add support

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

Updating Kconfig with a fragment requires symbols to be visible. Add an
environment submenu to display all generated variables.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 Kconfig              | 9 ++++-----
 kconfigs/Kconfig.env | 5 +++++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Kconfig b/Kconfig
index 357341be108de3dab79883f9a6babd29547d62de..6877b7aebd5057a925b371d8c6afd9ad568faa6e 100644
--- a/Kconfig
+++ b/Kconfig
@@ -15,11 +15,6 @@ config TOPDIR_PATH_HAS_SHA256SUM
 	output yaml
 	default y
 
-config TOPDIR_PATH_SHA256SUM
-	string
-	output yaml
-	default $(shell, ./scripts/compute_sha256sum.sh $(TOPDIR_PATH))
-
 config HAVE_KDEVOPS_CUSTOM_DEFAULTS
 	bool
 	default n
@@ -81,3 +76,7 @@ endmenu
 menu "Kdevops configuration"
 source "kconfigs/Kconfig.kdevops"
 endmenu
+
+menu "Environment"
+source "kconfigs/Kconfig.env"
+endmenu
diff --git a/kconfigs/Kconfig.env b/kconfigs/Kconfig.env
new file mode 100644
index 0000000000000000000000000000000000000000..400643f9a304556f27a63792239a383be51866c3
--- /dev/null
+++ b/kconfigs/Kconfig.env
@@ -0,0 +1,5 @@
+config TOPDIR_PATH_SHA256SUM
+	string "TOPDIR_PATH checksum"
+	output yaml
+	help
+	  TOPDIR_PATH checksum

-- 
2.47.2

[PATCH RFC 6/6] playbooks: add kconfig support

[Daniel Gomez]

From: Daniel Gomez <da.gomez@samsung.com>

Add kconfig playbook to generate Kconfig environment with Ansible.

The output will be a Kconfig fragment (.env.config) that will be merged
with existing configuration file.

Add .env.config to the list of git-ignored files.

Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
---
 .gitignore                             |  2 ++
 playbooks/kconfig.yml                  |  6 ++++++
 playbooks/roles/kconfig/tasks/main.yml | 15 +++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/.gitignore b/.gitignore
index 5ddfcc45e544ddb09ac81807e84fe88b329aec88..2fd102276301a2ce159e1c475f88ad464e2dc735 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,8 @@
 *.o
 *.retry
 
+.env.config
+
 .kdevops\.depcheck
 .provisioned_once*
 
diff --git a/playbooks/kconfig.yml b/playbooks/kconfig.yml
new file mode 100644
index 0000000000000000000000000000000000000000..4fa725c9046282a32d9a57c72c637194f3ad2335
--- /dev/null
+++ b/playbooks/kconfig.yml
@@ -0,0 +1,6 @@
+---
+- name: Kconfig
+  hosts: localhost
+  gather_facts: no
+  roles:
+    - role: kconfig
diff --git a/playbooks/roles/kconfig/tasks/main.yml b/playbooks/roles/kconfig/tasks/main.yml
new file mode 100644
index 0000000000000000000000000000000000000000..36e92076c3a63d617f8df6b66dae9d2925800208
--- /dev/null
+++ b/playbooks/roles/kconfig/tasks/main.yml
@@ -0,0 +1,15 @@
+---
+- name: Checksum
+  ansible.builtin.shell: |
+    set -o pipefail
+    echo "{{ topdir_path }}" | sha256sum | awk '{print $1}'
+  register: kconfig_checksum
+  changed_when: false
+
+- name: Create env.config
+  ansible.builtin.copy:
+    dest: "{{ topdir_path }}/.env.config"
+    mode: "0755"
+    content: |
+      CONFIG_TOPDIR_PATH_SHA256SUM="{{ kconfig_checksum.stdout }}"
+  changed_when: true

-- 
2.47.2

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Luis Chamberlain]

On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> From: Daniel Gomez <da.gomez@samsung.com>
> 
> Add kconfig playbook to generate Kconfig environment with Ansible.
> 
> The output will be a Kconfig fragment (.env.config) that will be merged
> with existing configuration file.
> 
> Add .env.config to the list of git-ignored files.
> 
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>

I think I get what this is doing and this seems pretty awesome!
Essentially we replace the kdevops Kconfigs which rely on scripts
with ansible calls which can rely on jinja2 template parsing on the
fly. This works especially well for use given we alreayd process
config symbols to yaml entries and always read them for ansible
playbooks.

While we could explore adapting this for complex scripts we use
like the ones which try to check your existing libvirt pools,
a simple jinja2 processing trick might be nicer to help us scale
with more symbols, otherwise kconfig will be rather slow. So,
any chance we might be able to leverage some regular python jinja2
thing which perhaps upstream Linux can also embrace?

Also I think it makes sense to combine this and patch #5, and also
you forgot to remove the script this is replacing?

  Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Daniel Gomez]

On Fri, Mar 28, 2025 at 03:58:50PM +0100, Luis Chamberlain wrote:
> On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> > From: Daniel Gomez <da.gomez@samsung.com>
> > 
> > Add kconfig playbook to generate Kconfig environment with Ansible.
> > 
> > The output will be a Kconfig fragment (.env.config) that will be merged
> > with existing configuration file.
> > 
> > Add .env.config to the list of git-ignored files.
> > 
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> 
> I think I get what this is doing and this seems pretty awesome!
> Essentially we replace the kdevops Kconfigs which rely on scripts
> with ansible calls

Exactly.

> which can rely on jinja2 template parsing on the
> fly.

The patch still does require templates as it's just mapping
'CONFIG_VARIABLE={{ ansible_generated_variable }}' in the last task ("Create
env.config").

> This works especially well for use given we alreayd process
> config symbols to yaml entries and always read them for ansible
> playbooks.

You're right!

Also, generating CONFIG_TOPDIR_PATH_SHA256SUM in a Kconfig fragment doesn't make
sense since we don't use it in Kconfig. So, we likely need two output files:

1. An Ansible YAML file for extra-vars
2. A Kconfig fragment for Kconfig dependencies (or user modifications)

Some environment variables (like those in Kconfig.distro) will go into the
Kconfig fragment, while others can be used directly in Ansible. I'm just not
sure yet about the ratio.

I think merging all generated variables into Kconfig makes this a bit easier as
we don't have to deal with another Ansible YAML file. Ideally, we would merge
this new YAML with the Kconfig YAML output. Can this be done? Makes sense?

> 
> While we could explore adapting this for complex scripts we use
> like the ones which try to check your existing libvirt pools,
> a simple jinja2 processing trick might be nicer to help us scale
> with more symbols, otherwise kconfig will be rather slow. So,

I agree. However, we may need to expose some symbols to Kconfig anyway, if they
are used as dependency symbols or the user wants to change them like the libvirt
pools.

> any chance we might be able to leverage some regular python jinja2
> thing which perhaps upstream Linux can also embrace?

Do you mean upstream (Linux) Kconfig?

I'll look into the libvirt pool scripts and see how can they be integrated in
Ansible.

> 
> Also I think it makes sense to combine this and patch #5, and also
> you forgot to remove the script this is replacing?

That's right. I'll do this in the next iteration.

> 
>   Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Luis Chamberlain]

On Mon, Mar 31, 2025 at 08:54:59AM +0200, Daniel Gomez wrote:
> On Fri, Mar 28, 2025 at 03:58:50PM +0100, Luis Chamberlain wrote:
> > On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> > > From: Daniel Gomez <da.gomez@samsung.com>
> > > 
> > > Add kconfig playbook to generate Kconfig environment with Ansible.
> > > 
> > > The output will be a Kconfig fragment (.env.config) that will be merged
> > > with existing configuration file.
> > > 
> > > Add .env.config to the list of git-ignored files.
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > 
> > I think I get what this is doing and this seems pretty awesome!
> > Essentially we replace the kdevops Kconfigs which rely on scripts
> > with ansible calls
> 
> Exactly.
> 
> > which can rely on jinja2 template parsing on the
> > fly.
> 
> The patch still does require templates as it's just mapping
> 'CONFIG_VARIABLE={{ ansible_generated_variable }}' in the last task ("Create
> env.config").
> 
> > This works especially well for use given we alreayd process
> > config symbols to yaml entries and always read them for ansible
> > playbooks.
> 
> You're right!
> 
> Also, generating CONFIG_TOPDIR_PATH_SHA256SUM in a Kconfig fragment doesn't make
> sense since we don't use it in Kconfig. So, we likely need two output files:
> 
> 1. An Ansible YAML file for extra-vars

Oh I see what you're saying, so Kconfig simply invokes the *need* for this,
and so we don't need to actually get Kconfig to really call anything fo
us. So we want to instruct essentially Kconfig we don't need this for
.config but want this as part of a secondary effort to just ammend our
yaml file, somehow.

> 2. A Kconfig fragment for Kconfig dependencies (or user modifications)

Makes sense, these can however easily go out of sync so we'll need a
sanity checker pre-commit or something later.

> Some environment variables (like those in Kconfig.distro) will go into the
> Kconfig fragment, while others can be used directly in Ansible. I'm just not
> sure yet about the ratio.

No rush, was with optional output yaml, we can start with just 1 and scale
later.

> I think merging all generated variables into Kconfig makes this a bit easier as
> we don't have to deal with another Ansible YAML file. Ideally, we would merge
> this new YAML with the Kconfig YAML output. Can this be done? Makes sense?

Yes. We have:

Kconfig -->
.config --> optional output yaml --> 1st.yaml
        |
	|
	v------- what we need -----> 2nd.yaml
	|
	|
	v--------------------------> extra_vars.yaml
	

Is that right?

> > While we could explore adapting this for complex scripts we use
> > like the ones which try to check your existing libvirt pools,
> > a simple jinja2 processing trick might be nicer to help us scale
> > with more symbols, otherwise kconfig will be rather slow. So,
> 
> I agree. However, we may need to expose some symbols to Kconfig anyway, if they
> are used as dependency symbols or the user wants to change them like the libvirt
> pools.

Well, we already support dynconfig for dynamic Kconfig generation for
really slow automatic Kconfig file writing which is just optional, but
indeed, we have many things in the middle, things we *need* to run on Kconfig,
inferences. So yeah I do agree, there is a middle ground.

> > any chance we might be able to leverage some regular python jinja2
> > thing which perhaps upstream Linux can also embrace?
> 
> Do you mean upstream (Linux) Kconfig?

Yup.

For instance, the challenge there is how can we get to say support
different compiler options so that if you support different set you
get to pick the one you want, and we use generic language to express
this. This allows us to use a .config to *use* a specific target
compiler with a .config to reproduce a build with without having to use
variables on the command line.

I think we've been pushing Kconfig to its limit on kdevops for fancy
stuff so it may make sense for us to leverage what we can to see if
we can help with this endeavor upstream.

  Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Daniel Gomez]

On Mon, Mar 31, 2025 at 12:32:29AM +0100, Luis Chamberlain wrote:
> On Mon, Mar 31, 2025 at 08:54:59AM +0200, Daniel Gomez wrote:
> > On Fri, Mar 28, 2025 at 03:58:50PM +0100, Luis Chamberlain wrote:
> > > On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> > > > From: Daniel Gomez <da.gomez@samsung.com>

> > I think merging all generated variables into Kconfig makes this a bit easier as
> > we don't have to deal with another Ansible YAML file. Ideally, we would merge
> > this new YAML with the Kconfig YAML output. Can this be done? Makes sense?
> 
> Yes. We have:
> 
> Kconfig -->
> .config --> optional output yaml --> 1st.yaml
>         |
> 	|
> 	v------- what we need -----> 2nd.yaml
> 	|
> 	|
> 	v--------------------------> extra_vars.yaml
> 	
> 
> Is that right?

I was thinking this instead:

What we have (please, correct if it's wrong):

Kconfig --> .config
        --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible

What I'm suggesting above:

Makefile --> Ansible --> env.yaml                                                                                             |
                     --> env.config (Kconfig fragment) --> Kconfig --> .config                                                v
                                                                   --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible

But looking at this, do you see a way to simplify this?

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Luis Chamberlain]

On Mon, Mar 31, 2025 at 08:31:55PM +0200, Daniel Gomez wrote:
> On Mon, Mar 31, 2025 at 12:32:29AM +0100, Luis Chamberlain wrote:
> > On Mon, Mar 31, 2025 at 08:54:59AM +0200, Daniel Gomez wrote:
> > > On Fri, Mar 28, 2025 at 03:58:50PM +0100, Luis Chamberlain wrote:
> > > > On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> > > > > From: Daniel Gomez <da.gomez@samsung.com>
> 
> > > I think merging all generated variables into Kconfig makes this a bit easier as
> > > we don't have to deal with another Ansible YAML file. Ideally, we would merge
> > > this new YAML with the Kconfig YAML output. Can this be done? Makes sense?
> > 
> > Yes. We have:
> > 
> > Kconfig -->
> > .config --> optional output yaml --> 1st.yaml
> >         |
> > 	|
> > 	v------- what we need -----> 2nd.yaml
> > 	|
> > 	|
> > 	v--------------------------> extra_vars.yaml
> > 	
> > 
> > Is that right?
> 
> I was thinking this instead:
> 
> What we have (please, correct if it's wrong):
> 
> Kconfig --> .config
>         --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible
> 
> What I'm suggesting above:
> 
> Makefile --> Ansible --> env.yaml                                                                                             |
>                      --> env.config (Kconfig fragment) --> Kconfig --> .config                                                v
>                                                                    --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible
> 
> But looking at this, do you see a way to simplify this?

Ah yes.

The 'make dynconfig' support already does this but it we currently only
use it it to scrape your PCI tree to support PCI passthrough through
Kconfig support.

It was built to grow, so for example in the future I've hinted before we
could use cloud tools to scrape your cloud options and so to generate
Kconfig files automatically for us, instead of us having to write each
entry.

I think this usecase begs for a more basic form of 'make dynconfig', one
which we always call.

If we're going to do that then we can kill two birds with one stone by
having info-dynconfig and system-info-dynconfig, where the info-dynconfig
consists of targets which won't require root or sudo. This is the part
that might be useful to Masahiro, things like your compiler. Then the
system-info-dynconfig target can be targets which require sudo or root.
In that way, if a user does not want to have local playbooks run sudo
they can simply describe their environment with a yaml file as input.

Nix users can also then just describe their system info and and be happy
with calling qemu in a simple way, if they don't want libvirt to manage
crap for you.

Thoughts?

  Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Daniel Gomez]

On Tue, Apr 01, 2025 at 02:02:23PM +0100, Luis Chamberlain wrote:
> On Mon, Mar 31, 2025 at 08:31:55PM +0200, Daniel Gomez wrote:
> > On Mon, Mar 31, 2025 at 12:32:29AM +0100, Luis Chamberlain wrote:
> > > On Mon, Mar 31, 2025 at 08:54:59AM +0200, Daniel Gomez wrote:
> > > > On Fri, Mar 28, 2025 at 03:58:50PM +0100, Luis Chamberlain wrote:
> > > > > On Fri, Mar 28, 2025 at 08:56:19PM +0000, Daniel Gomez wrote:
> > > > > > From: Daniel Gomez <da.gomez@samsung.com>
> > 
> > > > I think merging all generated variables into Kconfig makes this a bit easier as
> > > > we don't have to deal with another Ansible YAML file. Ideally, we would merge
> > > > this new YAML with the Kconfig YAML output. Can this be done? Makes sense?
> > > 
> > > Yes. We have:
> > > 
> > > Kconfig -->
> > > .config --> optional output yaml --> 1st.yaml
> > >         |
> > > 	|
> > > 	v------- what we need -----> 2nd.yaml
> > > 	|
> > > 	|
> > > 	v--------------------------> extra_vars.yaml
> > > 	
> > > 
> > > Is that right?
> > 
> > I was thinking this instead:
> > 
> > What we have (please, correct if it's wrong):
> > 
> > Kconfig --> .config
> >         --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible
> > 
> > What I'm suggesting above:
> > 
> > Makefile --> Ansible --> env.yaml                                                                                             |
> >                      --> env.config (Kconfig fragment) --> Kconfig --> .config                                                v
> >                                                                    --> .extra_vars_auto.yaml --> Makefile --> extra_vars.yaml --> Ansible
> > 
> > But looking at this, do you see a way to simplify this?
> 
> Ah yes.
> 
> The 'make dynconfig' support already does this but it we currently only
> use it it to scrape your PCI tree to support PCI passthrough through
> Kconfig support.
> 
> It was built to grow, so for example in the future I've hinted before we
> could use cloud tools to scrape your cloud options and so to generate
> Kconfig files automatically for us, instead of us having to write each
> entry.
> 
> I think this usecase begs for a more basic form of 'make dynconfig', one
> which we always call.

The only difference I see is that dynconfig generates a full Kconfig file, while
this one creates a fragment meant to be merged with an existing .config using
predefined Kconfig options.

If I understood dynconfig correctly, then I agree with you. There's no reason
to declare these in the current Kconfig, unless we want to avoid generating them
every time. Then we'd need the fragment generation -> merge. Also, the fragment
generation is much simpler to generate than an entire Kconfig file.

> 
> If we're going to do that then we can kill two birds with one stone by
> having info-dynconfig and system-info-dynconfig, where the info-dynconfig
> consists of targets which won't require root or sudo. This is the part
> that might be useful to Masahiro, things like your compiler. Then the
> system-info-dynconfig target can be targets which require sudo or root.
> In that way, if a user does not want to have local playbooks run sudo
> they can simply describe their environment with a yaml file as input.

This works with either option, since both are Ansible playbooks anyway.

> 
> Nix users can also then just describe their system info and and be happy
> with calling qemu in a simple way, if they don't want libvirt to manage
> crap for you.

I'm not sure about this. Can you ellaborate?

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Luis Chamberlain]

On Fri, Apr 04, 2025 at 10:33:07PM +0200, Daniel Gomez wrote:
> On Tue, Apr 01, 2025 at 02:02:23PM +0100, Luis Chamberlain wrote:
> > I think this usecase begs for a more basic form of 'make dynconfig', one
> > which we always call.
> 
> The only difference I see is that dynconfig generates a full Kconfig file, while
> this one creates a fragment meant to be merged with an existing .config using
> predefined Kconfig options.

The relationship is a requirement to be generated before existing
Kconfig files. The current kconfig fork maintained by linux-kdevops/kconfig [0]
is maintained as a git subtree inside kdevops so to allow us to not require
those odd git submodule strategy, ie, we maintain all history inside git
and for users they just see the git tree one, not two.

And so the common goal is to be able to try to avoid having to modify
things which can make kconfig independent from Linux's version of kconfig.

[0] https://github.com/linux-kdevops/kconfig

> If I understood dynconfig correctly, then I agree with you. There's no reason
> to declare these in the current Kconfig, unless we want to avoid generating them
> every time. Then we'd need the fragment generation -> merge. Also, the fragment
> generation is much simpler to generate than an entire Kconfig file.

Indeed,

> > If we're going to do that then we can kill two birds with one stone by
> > having info-dynconfig and system-info-dynconfig, where the info-dynconfig
> > consists of targets which won't require root or sudo. This is the part
> > that might be useful to Masahiro, things like your compiler. Then the
> > system-info-dynconfig target can be targets which require sudo or root.
> > In that way, if a user does not want to have local playbooks run sudo
> > they can simply describe their environment with a yaml file as input.
> 
> This works with either option, since both are Ansible playbooks anyway.

Sure.

> > Nix users can also then just describe their system info and and be happy
> > with calling qemu in a simple way, if they don't want libvirt to manage
> > crap for you.
> 
> I'm not sure about this. Can you ellaborate?

OK so for example kdevops today aims to try infer tons of information
from a user's system for libvirt uses. For debian based systems we go so
far as having to use sudo to query the libvirt pools that exist. To many
this is a security push they wish they could avoid. So an alternative to
this is to instruct users by defaul to write down their systems's
information about storage pool information. For nix users the same might
apply so we do not repeat the same history. Instead of "requiring" to
run sudo to check a system's virsh pool, we just go the other way with
nix, and just have the user write down on a file manually where their
virtpool stuff is. Only if they user a special command do we do the
inference hoop which may require sudo. This sort of informaiton is
needed before kconfig starts processign Kconfig files, and so we have
two parts to this, a) new featuers b) existing features. The b) featuers
are like the libvirt pool stuff. While an example of a) is nix OS
suport.

 Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Daniel Gomez]

On Sun, Apr 06, 2025 at 09:03:38PM +0100, Luis Chamberlain wrote:
> On Fri, Apr 04, 2025 at 10:33:07PM +0200, Daniel Gomez wrote:
> > On Tue, Apr 01, 2025 at 02:02:23PM +0100, Luis Chamberlain wrote:
> > > I think this usecase begs for a more basic form of 'make dynconfig', one
> > > which we always call.
> > 
> > The only difference I see is that dynconfig generates a full Kconfig file, while
> > this one creates a fragment meant to be merged with an existing .config using
> > predefined Kconfig options.
> 
> The relationship is a requirement to be generated before existing
> Kconfig files. The current kconfig fork maintained by linux-kdevops/kconfig [0]
> is maintained as a git subtree inside kdevops so to allow us to not require
> those odd git submodule strategy, ie, we maintain all history inside git
> and for users they just see the git tree one, not two.
> 
> And so the common goal is to be able to try to avoid having to modify
> things which can make kconfig independent from Linux's version of kconfig.
> 
> [0] https://github.com/linux-kdevops/kconfig
> 
> > If I understood dynconfig correctly, then I agree with you. There's no reason
> > to declare these in the current Kconfig, unless we want to avoid generating them
> > every time. Then we'd need the fragment generation -> merge. Also, the fragment
> > generation is much simpler to generate than an entire Kconfig file.

I was thinking about this recently. We could put these Kconfig files in a
template and just replace the y/n/string options very easy with Ansible as in
this patch series. Maybe this was your earlier suggestion in this thread?

	"Essentially we replace the kdevops Kconfigs which rely on scripts
	with ansible calls which can rely on jinja2 template parsing on the
	fly."

Then, we don't need to merge anything. That way, we'd only regenerate the values
each time, not the whole Kconfig.

	"a simple jinja2 processing trick might be nicer to help us scale with more
	symbols, otherwise kconfig will be rather slow."

But here, your suggestion goes more into replacing Kconfig with jinja2. I'm
confused. Can you clarify this too?

> 
> Indeed,
> 
> > > If we're going to do that then we can kill two birds with one stone by
> > > having info-dynconfig and system-info-dynconfig, where the info-dynconfig
> > > consists of targets which won't require root or sudo. This is the part
> > > that might be useful to Masahiro, things like your compiler. Then the
> > > system-info-dynconfig target can be targets which require sudo or root.
> > > In that way, if a user does not want to have local playbooks run sudo
> > > they can simply describe their environment with a yaml file as input.
> > 
> > This works with either option, since both are Ansible playbooks anyway.
> 
> Sure.
> 
> > > Nix users can also then just describe their system info and and be happy
> > > with calling qemu in a simple way, if they don't want libvirt to manage
> > > crap for you.
> > 
> > I'm not sure about this. Can you ellaborate?
> 
> OK so for example kdevops today aims to try infer tons of information
> from a user's system for libvirt uses. For debian based systems we go so
> far as having to use sudo to query the libvirt pools that exist. To many
> this is a security push they wish they could avoid. So an alternative to
> this is to instruct users by defaul to write down their systems's
> information about storage pool information. For nix users the same might

This means, we need an input yaml with variables, right?

> apply so we do not repeat the same history. Instead of "requiring" to
> run sudo to check a system's virsh pool, we just go the other way with
> nix, and just have the user write down on a file manually where their
> virtpool stuff is. Only if they user a special command do we do the
> inference hoop which may require sudo. This sort of informaiton is
> needed before kconfig starts processign Kconfig files, and so we have
> two parts to this, a) new featuers b) existing features. The b) featuers
> are like the libvirt pool stuff. While an example of a) is nix OS
> suport.

I see. Thanks for clarifying.

With all the Kconfig, YAML, Ansible inputs, merges, and templates, it feels like
we’re overcomplicating something that should be relatively simple.

> 
>  Luis

Re: [PATCH RFC 6/6] playbooks: add kconfig support

[Luis Chamberlain]

On Mon, Apr 14, 2025 at 12:58:01PM +0200, Daniel Gomez wrote:
> On Sun, Apr 06, 2025 at 09:03:38PM +0100, Luis Chamberlain wrote:
> > On Fri, Apr 04, 2025 at 10:33:07PM +0200, Daniel Gomez wrote:
> > > On Tue, Apr 01, 2025 at 02:02:23PM +0100, Luis Chamberlain wrote:
> > > > I think this usecase begs for a more basic form of 'make dynconfig', one
> > > > which we always call.
> > > 
> > > The only difference I see is that dynconfig generates a full Kconfig file, while
> > > this one creates a fragment meant to be merged with an existing .config using
> > > predefined Kconfig options.
> > 
> > The relationship is a requirement to be generated before existing
> > Kconfig files. The current kconfig fork maintained by linux-kdevops/kconfig [0]
> > is maintained as a git subtree inside kdevops so to allow us to not require
> > those odd git submodule strategy, ie, we maintain all history inside git
> > and for users they just see the git tree one, not two.
> > 
> > And so the common goal is to be able to try to avoid having to modify
> > things which can make kconfig independent from Linux's version of kconfig.
> > 
> > [0] https://github.com/linux-kdevops/kconfig
> > 
> > > If I understood dynconfig correctly, then I agree with you. There's no reason
> > > to declare these in the current Kconfig, unless we want to avoid generating them
> > > every time. Then we'd need the fragment generation -> merge. Also, the fragment
> > > generation is much simpler to generate than an entire Kconfig file.
> 
> I was thinking about this recently. We could put these Kconfig files in a
> template and just replace the y/n/string options very easy with Ansible as in
> this patch series. Maybe this was your earlier suggestion in this thread?

There are two things being discussed, the fragment stuff, and the
enhancing reliability of scripts used in Kconfig. So while you had
one goal, the first, I was thinking more about the second, with the
hope we can pave the way for both. I am not sure now -- so disregard this
for now with time we can evaluate that other aspect. The reason I say
this is that runs of ansible are typically not used for generating
stdout, whereas scripts in Kconfig want simple bool, tristate, strings
in stdout. I just tried to see how far I can take ansible to the
simplest thing for stdout and I'm not happy about it so I think we can
just stick to python for more robust script calls.

> 	"Essentially we replace the kdevops Kconfigs which rely on scripts
> 	with ansible calls which can rely on jinja2 template parsing on the
> 	fly."
> 
> Then, we don't need to merge anything. That way, we'd only regenerate the values
> each time, not the whole Kconfig.
> 
> 	"a simple jinja2 processing trick might be nicer to help us scale with more
> 	symbols, otherwise kconfig will be rather slow."
> 
> But here, your suggestion goes more into replacing Kconfig with jinja2. I'm
> confused. Can you clarify this too?

I mean more about the usage of scripts in kconfig symbols. A complex
example was the inference for the storage pool path used by users.

> > OK so for example kdevops today aims to try infer tons of information
> > from a user's system for libvirt uses. For debian based systems we go so
> > far as having to use sudo to query the libvirt pools that exist. To many
> > this is a security push they wish they could avoid. So an alternative to
> > this is to instruct users by defaul to write down their systems's
> > information about storage pool information. For nix users the same might
> 
> This means, we need an input yaml with variables, right?

Yes.

> > apply so we do not repeat the same history. Instead of "requiring" to
> > run sudo to check a system's virsh pool, we just go the other way with
> > nix, and just have the user write down on a file manually where their
> > virtpool stuff is. Only if they user a special command do we do the
> > inference hoop which may require sudo. This sort of informaiton is
> > needed before kconfig starts processign Kconfig files, and so we have
> > two parts to this, a) new featuers b) existing features. The b) featuers
> > are like the libvirt pool stuff. While an example of a) is nix OS
> > suport.
> 
> I see. Thanks for clarifying.
> 
> With all the Kconfig, YAML, Ansible inputs, merges, and templates, it feels like
> we’re overcomplicating something that should be relatively simple.

Yeah agreed.

  Luis