Add west.configuration.Configuration class #570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbolivar-nordic
force-pushed
the
add-configuration-class
branch
from
29050c6
to
83298e6
Compare
The argparse module changed some implementation details in in a way that's causing a false failure in testing. Handle it. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic
force-pushed
the
add-configuration-class
branch
from
83298e6
to
7d8fab6
Compare
tejlmand
approved these changes
Mar 18, 2022
| @@ -702,7 +704,7 @@ def set_zephyr_base(args, manifest, topdir): | |||
| pass | |||
| if projects: | |||
| zephyr = projects[0] | |||
| config.update_config('zephyr', 'base', zephyr.path) | |||
| config.set('zephyr.base', zephyr.path) | |||
There was a problem hiding this comment.
with the new Zephyr LTS 2.7 out, that has Zephyr CMake package, should we remove zephyr.base from .west/config instead ?
There was a problem hiding this comment.
with the new Zephyr LTS 2.7 out, that has Zephyr CMake package, should we remove
zephyr.basefrom.west/configinstead ?
Yes, but not for this PR, I think.
Tweak some incidental details and add a couple of extra asserts. Prep work for adding another test later on. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Clear up expected behavior. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Add a kwarg that makes _location() skip the workspace search if the local configuration file path is requested. No behavioral changes expected. This is prep work for reusing this helper in a new configuration API. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is a high-level class that lets you load and interact with the
configuration files in an object-oriented way. It's similar to how
west.manifest.Manifest lets you interact with the manifest, but for
configuration files.
The new API uses 'config.get("some.option")' style methods instead of
the configparser style 'config.get("some", "option")' with a separated
section and key. This makes the code match the options as they are
documented, making it easier to read and grep for.
Having an object around will allow us to deprecate the current
implementation, which relies on global state. It will also make it
easier to override default configuration behavior in certain
circumstances that will be useful in later patches.
Part of the road towards resolving zephyrproject-rtos#149 and at least one other issue.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Switch configuration file parsing to use a Configuration object. This prserves existing functionality, but allows us to start more easily passing around the configuration to other places that might want it pre-loaded. Retain existing behavior for backwards compatibility, but move the initialization of the global west.configuration.config object over to using the new API. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The requires_installation constructor kwarg and property have been deprecated for some time now, and they aren't in use in either Zephyr v1.14 LTS or v2.7 LTS. It should be fine to remove them at this point. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Add a 'config' property to WestCommand. This is a west.configuration.Configuration accessible for the duration of do_run(). For the same reason as the 'WestCommand.manifest' property exists the way it does, we cannot add a new constructor argument to capture the configuration. Handle this as a kwarg to run(). Take a Configuration in extension_commands(). Handle the necessary changes in main.py. These changes should be invisible to all the extension commands I am aware of in the wild. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Update built-in commands to use the recently-added config attribute. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
All built-in west commands are using the new WestCommand.config state to access the configuration values. Extension commands which are reading configuration values should migrate to using this interface as well, as should any other out of tree users of the west.configuration API. Deprecation, rather than removal, is necessary since 'west build' and other important Zephyr extensions rely on the existing API. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic
force-pushed
the
add-configuration-class
branch
from
7d8fab6
to
d8853a4
Compare
marc-hb
reviewed
Mar 18, 2022
There was a problem hiding this comment.
Cosmetic comments only, sorry not familiar enough for more.
Is there any rationale for this change outside testing? I couldn't tell for sure from the commit messages. In more concrete terms: do you ever expect a real-world use case to ever instantiante two different Configuration objects? I would assume Configuration to be immutable (which I think explain why it could stay a global for so long).
| @@ -70,6 +127,278 @@ class ConfigFile(Enum): | |||
| GLOBAL = 3 | |||
| LOCAL = 4 | |||
|
|
|||
| class Configuration: | |||
There was a problem hiding this comment.
class Configurations, plural?
Before reading the pydoc carefully I wrongly guessed that there would be 3 separate instances: local, global and system.
| config.read_config(topdir=self.topdir) | ||
| self.config = west.configuration.Configuration(topdir=self.topdir) | ||
|
|
||
| # Also set up the global configuration object to match, for |
There was a problem hiding this comment.
Suggested change
| # Also set up the global configuration object to match, for | |
| # Also set up the deprecated global configuration object to match, for |
Is it actually deprecated? Not sure yet.
|
@marc-hb thanks for review. It's not immutable -- it has setters, actually :). Maybe you meant some other word. And there are two use cases to keep in mind:
For 1., what we have now is enough. For 2., it is not, and west is also meant to be used directly via its APIs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is basically a prep work PR which: