configuration: fix round-trip bugs by removing configobj
West has historically relied on the third-party configobj library for writing configuration files instead of using the standard library's configparser module. The reason why is that configobj has round-trip support for comments. However, the public reading API uses configparser. Up until now, we were assuming that the two were compatible for the simple purposes we needed, and indeed they've proven compatible "enough" that the different code paths on read vs. write haven't been an issue. This has become a problem now that we are introducing the manifest.groups configuration option, though, because its value contains commas. The configparser and configobj file formats have a semantic difference between these two options, though: [section] foo = "one,two" bar = one,two The difference is: - in configobj, 'foo' is the string "one,two" and 'bar' is the list ['one', 'two'] - in configparser, 'foo' is the string '"one,two"' and bar is the string 'one,two' Further, the configobj library automatically adds quotes around any string that contains commas to enforce this distinction. This is breaking round-trips, since: west config section.foo one,two # configobj writes "one,two" west config section.foo # configparser reads '"one,two"' Looking at it further, configobj development seems to have stalled in 2014, and the most significant user it claims in its documentation (IPython) has moved on to .py and .json configuration files. This isn't worth the hassle. Just drop the configobj dependency and use configparser everywhere. This will delete comments that users have added to their configuration files and may otherwise reorder sections, but having the round-trip semantics correct is more important than that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This commit is contained in:
parent
91cca37903
commit
36f3f91e27
1
setup.py
1
setup.py
|
@ -44,7 +44,6 @@ setuptools.setup(
|
|||
'colorama',
|
||||
'PyYAML>=5.1',
|
||||
'pykwalify',
|
||||
'configobj',
|
||||
'setuptools',
|
||||
'packaging',
|
||||
],
|
||||
|
|
|
@ -42,8 +42,6 @@ import platform
|
|||
from enum import Enum
|
||||
from typing import Any, Optional, List
|
||||
|
||||
import configobj
|
||||
|
||||
from west.util import west_dir, WestNotFound, PathType
|
||||
|
||||
def _configparser(): # for internal use
|
||||
|
@ -129,11 +127,13 @@ def update_config(section: str, key: str, value: Any,
|
|||
raise ValueError(f'invalid configfile: {configfile}')
|
||||
|
||||
filename = _ensure_config(configfile, topdir)
|
||||
updater = configobj.ConfigObj(filename)
|
||||
if section not in updater:
|
||||
updater[section] = {}
|
||||
updater[section][key] = value
|
||||
updater.write()
|
||||
config = _configparser()
|
||||
config.read(filename)
|
||||
if section not in config:
|
||||
config[section] = {}
|
||||
config[section][key] = value
|
||||
with open(filename, 'w') as f:
|
||||
config.write(f)
|
||||
|
||||
def delete_config(section: str, key: str,
|
||||
configfile: Optional[ConfigFile] = None,
|
||||
|
@ -180,14 +180,16 @@ def delete_config(section: str, key: str,
|
|||
|
||||
found = False
|
||||
for path in to_check:
|
||||
cobj = configobj.ConfigObj(path)
|
||||
if section not in cobj or key not in cobj[section]:
|
||||
config = _configparser()
|
||||
config.read(path)
|
||||
if section not in config or key not in config[section]:
|
||||
continue
|
||||
|
||||
del cobj[section][key]
|
||||
if not cobj[section].items():
|
||||
del cobj[section]
|
||||
cobj.write()
|
||||
del config[section][key]
|
||||
if not config[section].items():
|
||||
del config[section]
|
||||
with open(path, 'w') as f:
|
||||
config.write(f)
|
||||
found = True
|
||||
if stop:
|
||||
break
|
||||
|
|
|
@ -498,7 +498,6 @@ def test_list():
|
|||
assert sorted_list('--local') == ['pytest.bar=what',
|
||||
'pytest.foo=who']
|
||||
|
||||
@pytest.mark.xfail()
|
||||
def test_round_trip():
|
||||
cmd('config pytest.foo bar,baz')
|
||||
assert cmd('config pytest.foo').strip() == 'bar,baz'
|
||||
|
|
Loading…
Reference in New Issue