-
Notifications
You must be signed in to change notification settings - Fork 147
[draft] Support dropin config files (within .d directories)
#849
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| import argparse | ||
|
|
||
| from west.commands import CommandError, WestCommand | ||
| from west.configuration import ConfigFile | ||
| from west.configuration import ConfigFile, Configuration | ||
|
|
||
| CONFIG_DESCRIPTION = '''\ | ||
| West configuration file handling. | ||
|
|
@@ -42,6 +42,19 @@ | |
| from earlier ones. Local values have highest precedence, and system values | ||
| lowest. | ||
|
|
||
| The path of each configuration file currently being considered can be printed: | ||
| west config --local --print-path | ||
| west config --global --print-path | ||
| west config --system --print-path | ||
|
|
||
| Note that '--print-path' may display default configuration paths for system and | ||
| global configurations. The local configuration must either exist or be specified | ||
| via environment variable, since it cannot be determined otherwise. | ||
|
|
||
| may print a considered default config paths in case | ||
| of system and global configurations, whereby the local configuration must | ||
| either exist or explicitly specified via environment variable. | ||
|
|
||
| To get a value for <name>, type: | ||
| west config <name> | ||
|
|
||
|
|
@@ -68,6 +81,28 @@ | |
|
|
||
| To delete <name> everywhere it's set, including the system file: | ||
| west config -D <name> | ||
|
|
||
| For each configuration type (local, global, and system), an additional | ||
| drop-in config directory is supported. This directory is named as the | ||
| according config file, but with a '.d' suffix, whereby all '.conf' and | ||
| '.ini' files are loaded in alphabetical order. | ||
|
|
||
| All files inside a drop-in directory must use `.conf` extension and are | ||
| loaded in **alphabetical order**. | ||
| For example: | ||
| .west/config.d/basics.conf | ||
|
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 this falls a bit short and misses at least the fact that writing and appending ignores dropins. |
||
|
|
||
| Note: It is not possible to modify dropin configs.via 'west config' commands. | ||
| When config option values are set/appended/deleted via 'west config' commands, | ||
| always the config file is modified (never the dropin config files). | ||
|
|
||
| To list the configuration files that are loaded (both the main config file | ||
| and all drop-ins) in the exact order they were applied (where later values | ||
| override earlier ones): | ||
| west config --list-paths | ||
| west config --local --list-paths | ||
| west config --global --list-paths | ||
| west config --system --list-paths | ||
| ''' | ||
|
|
||
| CONFIG_EPILOG = '''\ | ||
|
|
@@ -99,6 +134,18 @@ def do_add_parser(self, parser_adder): | |
| "action to perform (give at most one)" | ||
| ).add_mutually_exclusive_group() | ||
|
|
||
| group.add_argument( | ||
| '--print-path', | ||
| action='store_true', | ||
| help='print the file path from according west ' | ||
| 'config (--local [default], --global, --system)', | ||
| ) | ||
| group.add_argument( | ||
| '--list-paths', | ||
| action='store_true', | ||
| help='list all config files and dropin files that ' | ||
| 'are currently considered by west config', | ||
| ) | ||
| group.add_argument( | ||
| '-l', '--list', action='store_true', help='list all options and their values' | ||
| ) | ||
|
|
@@ -153,13 +200,17 @@ def do_run(self, args, user_args): | |
| if args.list: | ||
| if args.name: | ||
| self.parser.error('-l cannot be combined with name argument') | ||
| elif not args.name: | ||
| elif not any([args.name, args.print_path, args.list_paths]): | ||
| self.parser.error('missing argument name (to list all options and values, use -l)') | ||
| elif args.append: | ||
| if args.value is None: | ||
| self.parser.error('-a requires both name and value') | ||
|
|
||
| if args.list: | ||
| if args.print_path: | ||
| self.print_path(args) | ||
| elif args.list_paths: | ||
| self.list_paths(args) | ||
| elif args.list: | ||
| self.list(args) | ||
| elif delete: | ||
| self.delete(args) | ||
|
|
@@ -170,6 +221,16 @@ def do_run(self, args, user_args): | |
| else: | ||
| self.write(args) | ||
|
|
||
| def print_path(self, args): | ||
| config_path = self.config.get_path(args.configfile or LOCAL) | ||
| if config_path: | ||
|
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 could this be None or empty? If it were, something went seriously wrong and it would be better to crash.
Contributor
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. It can be Would you expect that it fails in this case, or shall it exit 0 and print no path for the local config (as it is now)?
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. If the user requests the local path while outside any workspace, I would just let the tool crash. This is an obvious misuse of the tool, no need to test for that in the code. I found a In any case I would never exit 0, the tool should not succeed. PS: please do not reply with a question and then hide the thread as "resolved" :-D |
||
| print(config_path) | ||
|
|
||
| def list_paths(self, args): | ||
| config_paths = Configuration().get_paths(args.configfile or ALL) | ||
| for config_path in config_paths: | ||
| print(config_path) | ||
|
|
||
| def list(self, args): | ||
| what = args.configfile or ALL | ||
| for option, value in self.config.items(configfile=what): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,11 @@ class _InternalCF: | |
| # For internal use only; convenience interface for reading and | ||
| # writing INI-style [section] key = value configuration files, | ||
| # but presenting a west-style section.key = value style API. | ||
| # The config file and the drop-in configs need separate | ||
| # configparsers, since west needs to determine options that are | ||
| # set in the config. E.g. if config values are updated, only those | ||
| # values shall be written to the config, which are already present | ||
| # (and not all values that may also come from dropin configs) | ||
|
|
||
| @staticmethod | ||
| def parse_key(dotted_name: str): | ||
|
|
@@ -69,63 +74,91 @@ def parse_key(dotted_name: str): | |
|
|
||
| @staticmethod | ||
| def from_path(path: Path | None) -> '_InternalCF | None': | ||
| return _InternalCF(path) if path and path.exists() else None | ||
| if not path: | ||
| return None | ||
| cf = _InternalCF(path) | ||
| if not cf.path and not cf.dropin_paths: | ||
|
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. Why bother creating an object when neither exists?
Contributor
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. Good point. It can be changed to |
||
| return None | ||
| return cf | ||
|
|
||
| def __init__(self, path: Path): | ||
| self.path = path | ||
| self.cp = _configparser() | ||
| read_files = self.cp.read(path, encoding='utf-8') | ||
| if len(read_files) != 1: | ||
| raise FileNotFoundError(path) | ||
| self.path = path if path.exists() else None | ||
| if self.path: | ||
| self.cp.read(self.path, encoding='utf-8') | ||
|
|
||
| # consider dropin configs | ||
| self.dropin_cp = _configparser() | ||
| self.dropin_dir = None | ||
| self.dropin_paths = [] | ||
| # dropin configs must be enabled in config | ||
| if self.cp.getboolean('config', 'dropins', fallback=False): | ||
| # dropin dir is the config path with .d suffix | ||
| dropin_dir = Path(f'{path}.d') | ||
| self.dropin_dir = dropin_dir if dropin_dir.exists() else None | ||
| if self.dropin_dir: | ||
| # dropin configs are applied in alphabetical order | ||
| for conf in sorted(self.dropin_dir.iterdir()): | ||
| # only consider .conf files | ||
| if conf.suffix in ['.conf', '.ini']: | ||
| self.dropin_paths.append(self.dropin_dir / conf) | ||
thorsten-klein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if self.dropin_paths: | ||
| self.dropin_cp.read(self.dropin_paths, encoding='utf-8') | ||
|
|
||
| def _paths(self) -> list[Path]: | ||
| ret = [p for p in self.dropin_paths] | ||
| if self.path: | ||
| ret.append(self.path) | ||
| return ret | ||
|
|
||
| def _write(self): | ||
| with open(self.path, 'w', encoding='utf-8') as f: | ||
| self.cp.write(f) | ||
|
|
||
| def __contains__(self, option: str) -> bool: | ||
| section, key = _InternalCF.parse_key(option) | ||
|
|
||
| return section in self.cp and key in self.cp[section] | ||
| if section in self.cp and key in self.cp[section]: | ||
| return True | ||
| return section in self.dropin_cp and key in self.dropin_cp[section] | ||
|
|
||
| def get(self, option: str): | ||
| return self._get(option, self.cp.get) | ||
| return self._get(option, self.cp.get, self.dropin_cp.get) | ||
|
|
||
| def getboolean(self, option: str): | ||
| return self._get(option, self.cp.getboolean) | ||
| return self._get(option, self.cp.getboolean, self.dropin_cp.getboolean) | ||
|
|
||
| def getint(self, option: str): | ||
| return self._get(option, self.cp.getint) | ||
| return self._get(option, self.cp.getint, self.dropin_cp.getint) | ||
|
|
||
| def getfloat(self, option: str): | ||
| return self._get(option, self.cp.getfloat) | ||
| return self._get(option, self.cp.getfloat, self.dropin_cp.getfloat) | ||
|
|
||
| def _get(self, option, getter): | ||
| def _get(self, option, config_getter, dropin_getter): | ||
| section, key = _InternalCF.parse_key(option) | ||
|
|
||
| try: | ||
| return getter(section, key) | ||
| except (configparser.NoOptionError, configparser.NoSectionError) as err: | ||
| raise KeyError(option) from err | ||
| if section in self.cp and key in self.cp[section]: | ||
| getter = config_getter | ||
| elif section in self.dropin_cp and key in self.dropin_cp[section]: | ||
| getter = dropin_getter | ||
| else: | ||
| raise KeyError(option) | ||
|
Comment on lines
+139
to
+144
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. There is a subtle difference here, but one nonetheless. Before we were able to determine a difference between a missing key or an entire missing section, we no longer have that difference. |
||
| return getter(section, key) | ||
|
|
||
| def set(self, option: str, value: Any): | ||
| section, key = _InternalCF.parse_key(option) | ||
|
|
||
| if section not in self.cp: | ||
| self.cp[section] = {} | ||
|
|
||
| self.cp[section][key] = value | ||
|
|
||
| with open(self.path, 'w', encoding='utf-8') as f: | ||
| self.cp.write(f) | ||
| self._write() | ||
|
|
||
| def delete(self, option: str): | ||
| section, key = _InternalCF.parse_key(option) | ||
|
|
||
| if section not in self.cp: | ||
| if option not in self: | ||
| raise KeyError(option) | ||
|
|
||
| del self.cp[section][key] | ||
| if not self.cp[section].items(): | ||
| del self.cp[section] | ||
|
|
||
| with open(self.path, 'w', encoding='utf-8') as f: | ||
| self.cp.write(f) | ||
| self._write() | ||
|
|
||
|
|
||
| class ConfigFile(Enum): | ||
|
|
@@ -181,6 +214,28 @@ def __init__(self, topdir: PathType | None = None): | |
| self._global = _InternalCF.from_path(self._global_path) | ||
| self._local = _InternalCF.from_path(self._local_path) | ||
|
|
||
| def get_path(self, configfile: ConfigFile = ConfigFile.LOCAL): | ||
| if configfile == ConfigFile.ALL: | ||
| raise RuntimeError(f'{configfile} not allowed for this operation') | ||
| elif configfile == ConfigFile.LOCAL: | ||
| if not self._local_path: | ||
| raise MalformedConfig('local configuration cannot be determined') | ||
| return self._local_path | ||
| elif configfile == ConfigFile.GLOBAL: | ||
| return self._global_path | ||
| elif configfile == ConfigFile.SYSTEM: | ||
| return self._system_path | ||
|
|
||
| def get_paths(self, configfile: ConfigFile = ConfigFile.ALL): | ||
| ret = [] | ||
| if self._system and configfile in [ConfigFile.SYSTEM, ConfigFile.ALL]: | ||
| ret += self._system._paths() | ||
| if self._global and configfile in [ConfigFile.GLOBAL, ConfigFile.ALL]: | ||
| ret += self._global._paths() | ||
| if self._local and configfile in [ConfigFile.LOCAL, ConfigFile.ALL]: | ||
| ret += self._local._paths() | ||
| return ret | ||
|
|
||
| def get( | ||
| self, option: str, default: str | None = None, configfile: ConfigFile = ConfigFile.ALL | ||
| ) -> str | None: | ||
|
|
@@ -354,13 +409,14 @@ def _copy_to_configparser(self, cp: configparser.ConfigParser) -> None: | |
| # function-and-global-state APIs. | ||
|
|
||
| def load(cf: _InternalCF): | ||
| for section, contents in cf.cp.items(): | ||
| if section == 'DEFAULT': | ||
| continue | ||
| if section not in cp: | ||
| cp.add_section(section) | ||
| for key, value in contents.items(): | ||
| cp[section][key] = value | ||
| for cp in [cf.dropin_cp, cf.cp]: | ||
| for section, contents in cp.items(): | ||
| if section == 'DEFAULT': | ||
| continue | ||
| if section not in cp: | ||
| cp.add_section(section) | ||
| for key, value in contents.items(): | ||
| cp[section][key] = value | ||
|
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. Would you mind expanding a bit the pydoc for this function? It's pretty cryptic to me, sorry. Or just add some relevant references.
Contributor
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. Honestly I am not sure if this function is still neccessary. I have removed it completely (and its usage in
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. The comment at the start of the function would indicate that west extension commands could rely on it? # Internal API for main to use to maintain backwards
# compatibility for existing extensions using the legacy
# function-and-global-state APIs.
Contributor
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. Okay. Is it possible that there is no test existing for this? I am not sure how I can guarantee that I do not break something here, since I do not know the previous behavior.
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. The answers to this sort of questions usually lie in git history.
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. Maybe this? Just spotted by chance:
|
||
|
|
||
| if self._system: | ||
| load(self._system) | ||
|
|
@@ -406,11 +462,12 @@ def _cf_to_dict(cf: _InternalCF | None) -> dict[str, Any]: | |
| ret: dict[str, Any] = {} | ||
| if cf is None: | ||
| return ret | ||
| for section, contents in cf.cp.items(): | ||
| if section == 'DEFAULT': | ||
| continue | ||
| for key, value in contents.items(): | ||
| ret[f'{section}.{key}'] = value | ||
| for cp in [cf.dropin_cp, cf.cp]: | ||
| for section, contents in cp.items(): | ||
| if section == 'DEFAULT': | ||
| continue | ||
| for key, value in contents.items(): | ||
| ret[f'{section}.{key}'] = value | ||
| return ret | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.