Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app: config: Add support for appending to the config string #768

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions src/west/app/config.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2019, Nordic Semiconductor ASA

Check notice on line 1 in src/west/app/config.py

View workflow job for this annotation

GitHub Actions / Check file src/west/app/config.py

Unformatted file

Consider running 'ruff format src/west/app/config.py' See https://github.com/zephyrproject-rtos/west/actions/runs/12559801115 for more details
#
# SPDX-License-Identifier: Apache-2.0

Expand Down Expand Up @@ -48,6 +48,14 @@
To set a value for <name>, type:
west config <name> <value>

To append to a value for <name>, type:
west config -a <name> <value>
A value must exist in the selected configuration file in order to be able
to append to it. The existing value can be empty.
Examples:
west config -a build.cmake-args -- " -DEXTRA_CFLAGS='-Wextra -g0' -DFOO=BAR"
carlescufi marked this conversation as resolved.
Show resolved Hide resolved
west config -a manifest.group-filter ,+optional

To list all options and their values:
west config -l

Expand All @@ -64,7 +72,7 @@

CONFIG_EPILOG = '''\
If the configuration file to use is not set, reads use all three in
precedence order, and writes use the local file.'''
precedence order, and writes (including appends) use the local file.'''
marc-hb marked this conversation as resolved.
Show resolved Hide resolved

ALL = ConfigFile.ALL
SYSTEM = ConfigFile.SYSTEM
Expand All @@ -88,12 +96,18 @@
description=self.description,
epilog=CONFIG_EPILOG)

parser.add_argument('-l', '--list', action='store_true',
help='list all options and their values')
parser.add_argument('-d', '--delete', action='store_true',
help='delete an option in one config file')
parser.add_argument('-D', '--delete-all', action='store_true',
help="delete an option everywhere it's set")
group = parser.add_argument_group(
"action to perform (give at most one)"
).add_mutually_exclusive_group()

group.add_argument('-l', '--list', action='store_true',
help='list all options and their values')
group.add_argument('-d', '--delete', action='store_true',
help='delete an option in one config file')
carlescufi marked this conversation as resolved.
Show resolved Hide resolved
group.add_argument('-D', '--delete-all', action='store_true',
help="delete an option everywhere it's set")
group.add_argument('-a', '--append', action='store_true',
help='append to an existing value')
carlescufi marked this conversation as resolved.
Show resolved Hide resolved

group = parser.add_argument_group(
"configuration file to use (give at most one)"
Expand Down Expand Up @@ -121,20 +135,20 @@
if args.list:
if args.name:
self.parser.error('-l cannot be combined with name argument')
elif delete:
self.parser.error('-l cannot be combined with -d or -D')
elif not args.name:
self.parser.error('missing argument name '
'(to list all options and values, use -l)')
elif args.delete and args.delete_all:
self.parser.error('-d cannot be combined with -D')
elif args.value is None and args.append:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following this combinatorial is a bit demanding... I think the following "stylistic" change would really help:

   elif args.append:
       if args.value is None:
           error '-a requires both name and value'

I think it helps because it preserves the current pattern:

    elif valid_use_case1:
         if not sanity_check1:
            error1
    elif valid_use_case2:
         if sanity_check2:
            error2

It also stays more similar to the next switch/case immediately after this one.

self.parser.error('-a requires both name and value')

if args.list:
self.list(args)
elif delete:
self.delete(args)
elif args.value is None:
self.read(args)
elif args.append:
self.append(args)
else:
self.write(args)

Expand Down Expand Up @@ -179,6 +193,16 @@
self.dbg(f'{args.name} is unset')
raise CommandError(returncode=1)

def append(self, args):
self.check_config(args.name)
where = args.configfile or LOCAL
value = self.config.get(args.name, configfile=where)
if value is None:
self.die(f'option {args.name} not found in the {where.name.lower()} '
'configuration file')
args.value = value + args.value
self.write(args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you don't need where here?


def write(self, args):
self.check_config(args.name)
what = args.configfile or LOCAL
Expand Down
39 changes: 39 additions & 0 deletions tests/test_config.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2019, Nordic Semiconductor ASA

Check notice on line 1 in tests/test_config.py

View workflow job for this annotation

GitHub Actions / Check file tests/test_config.py

Unformatted file

Consider running 'ruff format tests/test_config.py' See https://github.com/zephyrproject-rtos/west/actions/runs/12559801115 for more details
#
# SPDX-License-Identifier: Apache-2.0

Expand Down Expand Up @@ -229,6 +229,45 @@
assert 'pytest' not in cfg(f=GLOBAL)
assert cfg(f=LOCAL, topdir=str(topdir))['pytest']['key'] == 'val'

def test_append():
update_testcfg('pytest', 'key', 'system', configfile=SYSTEM)
update_testcfg('pytest', 'key', 'global', configfile=GLOBAL)
update_testcfg('pytest', 'key', 'local', configfile=LOCAL)
# Appending with no configfile specified should modify the local one
cmd('config -a pytest.key ,bar')

# Only the local one will be modified
assert cfg(f=SYSTEM)['pytest']['key'] == 'system'
assert cfg(f=GLOBAL)['pytest']['key'] == 'global'
assert cfg(f=LOCAL)['pytest']['key'] == 'local,bar'

# Test a more complex one, and at a particular configfile level
update_testcfg('build', 'cmake-args', '-DCONF_FILE=foo.conf', configfile=GLOBAL)
assert cfg(f=GLOBAL)['build']['cmake-args'] == '-DCONF_FILE=foo.conf'

# Use a list instead of a string to get quoting right
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Use a list instead of a string to get quoting right
# Use a list instead of a string to avoid one level of nested quoting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this comment is necessary but I don't mind.

cmd(['config', '--global', '-a', 'build.cmake-args', '--',
' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, use outer " quotes so you don't need to escape the inner ' ones?

Suggested change
' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR'])
" -DEXTRA_CFLAGS='-Wextra -g0' -DFOO=BAR"])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it like that before, but I personally actually prefer it this way, because it is consistent with the rest of the elements in the list.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use " for inner quotes... unless it fails on Windows? As long as cmake does not run Windows should be fine...

I find backslashes a bit less readable but this is just test code so I don't actually care here.


assert cfg(f=GLOBAL)['build']['cmake-args'] == \
'-DCONF_FILE=foo.conf -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR'

def test_append_novalue():
with pytest.raises(subprocess.CalledProcessError) as exc_info:
cmd('config -a pytest.foo', stderr=subprocess.STDOUT)
# Get the output into a variable to simplify pytest error messages
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment adds a lot of value. No big deal.

err_msg = exc_info.value.output.decode("utf-8")
assert '-a requires both name and value' in err_msg

def test_append_notfound():
update_testcfg('pytest', 'key', 'val', configfile=LOCAL)
with pytest.raises(subprocess.CalledProcessError) as exc_info:
cmd('config -a pytest.foo bar', stderr=subprocess.STDOUT)
# Get the output into a variable to simplify pytest error messages
err_msg = exc_info.value.output.decode("utf-8")
assert 'option pytest.foo not found in the local configuration file' in err_msg


def test_delete_basic():
# Basic deletion test: write local, verify global and system deletions
# don't work, then delete local does work.
Expand Down
Loading