-
Notifications
You must be signed in to change notification settings - Fork 150
app: config: Add support for appending to the config string #768
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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
|
||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
|
|
@@ -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" | ||
| west config -a manifest.group-filter ,+optional | ||
|
|
||
| To list all options and their values: | ||
| west config -l | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -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)" | ||
|
|
@@ -121,20 +135,21 @@ | |
| 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.append: | ||
| if args.value is None: | ||
| 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) | ||
|
|
||
|
|
@@ -179,6 +194,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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come you don't need
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The doc says: we provide
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should have looked at the |
||
|
|
||
| def write(self, args): | ||
| self.check_config(args.name) | ||
| what = args.configfile or LOCAL | ||
|
|
||
| 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
|
||||||
| # | ||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||
|
|
||||||
|
|
@@ -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 avoid one level of nested quoting | ||||||
| cmd(['config', '--global', '-a', 'build.cmake-args', '--', | ||||||
| ' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR']) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, use outer
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use 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 | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does, if you don't do this the error message thrown by pypi is unreadable.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I misunderstood and thought this comment was referring to the code itself, not to its output.
Suggested change
Not important. |
||||||
| 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. | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.