-
Notifications
You must be signed in to change notification settings - Fork 3k
[build tools] Added better support for features and recursive configs #1940
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
ce0606a
d9749b0
6da324f
e70efe3
f45dc15
41daccf
99ed4e0
0d9d463
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 |
---|---|---|
|
@@ -40,6 +40,7 @@ def __init__(self, name, data, unit_name, unit_kind): | |
self.value = data.get("value", None) | ||
self.required = data.get("required", False) | ||
self.macro_name = data.get("macro_name", "MBED_CONF_%s" % self.sanitize(self.name.upper())) | ||
self.config_errors = [] | ||
|
||
# Return the full (prefixed) name of a parameter. | ||
# If the parameter already has a prefix, check if it is valid | ||
|
@@ -147,6 +148,11 @@ class Config: | |
"application": set(["config", "custom_targets", "target_overrides", "macros", "__config_path"]) | ||
} | ||
|
||
# Allowed features in configurations | ||
__allowed_features = [ | ||
"UVISOR", "BLE", "CLIENT", "IPV4", "IPV6" | ||
] | ||
|
||
# The initialization arguments for Config are: | ||
# target: the name of the mbed target used for this configuration instance | ||
# top_level_dirs: a list of top level source directories (where mbed_abb_config.json could be found) | ||
|
@@ -176,7 +182,9 @@ def __init__(self, target, top_level_dirs = []): | |
self.processed_configs = {} | ||
self.target = target if isinstance(target, str) else target.name | ||
self.target_labels = Target.get_target(self.target).get_labels() | ||
self.target_instance = Target.get_target(self.target) | ||
self.added_features = set() | ||
self.removed_features = set() | ||
self.removed_unecessary_features = False | ||
|
||
# Add one or more configuration files | ||
def add_config_files(self, flist): | ||
|
@@ -212,44 +220,59 @@ def _process_config_parameters(self, data, params, unit_name, unit_kind): | |
params[full_name] = ConfigParameter(name, v if isinstance(v, dict) else {"value": v}, unit_name, unit_kind) | ||
return params | ||
|
||
# Add features to the available features | ||
def remove_features(self, features): | ||
for feature in features: | ||
if feature in self.added_features: | ||
raise ConfigException("Configuration conflict. Feature %s both added and removed." % feature) | ||
|
||
self.removed_features |= set(features) | ||
|
||
# Remove features from the available features | ||
def add_features(self, features): | ||
for feature in features: | ||
if (feature in self.removed_features | ||
or (self.removed_unecessary_features and feature not in self.added_features)): | ||
raise ConfigException("Configuration conflict. Feature %s both added and removed." % feature) | ||
|
||
self.added_features |= set(features) | ||
|
||
# Helper function: process "config_parameters" and "target_config_overrides" in a given dictionary | ||
# data: the configuration data of the library/appliation | ||
# params: storage for the discovered configuration parameters | ||
# unit_name: the unit (library/application) that defines this parameter | ||
# unit_kind: the kind of the unit ("library" or "application") | ||
def _process_config_and_overrides(self, data, params, unit_name, unit_kind): | ||
self.config_errors = [] | ||
self._process_config_parameters(data.get("config", {}), params, unit_name, unit_kind) | ||
for label, overrides in data.get("target_overrides", {}).items(): | ||
# If the label is defined by the target or it has the special value "*", process the overrides | ||
if (label == '*') or (label in self.target_labels): | ||
# Parse out cumulative attributes | ||
for attr in Target._Target__cumulative_attributes: | ||
attrs = getattr(self.target_instance, attr) | ||
|
||
if attr in overrides: | ||
del attrs[:] | ||
attrs.extend(overrides[attr]) | ||
del overrides[attr] | ||
|
||
if attr+'_add' in overrides: | ||
attrs.extend(overrides[attr+'_add']) | ||
del overrides[attr+'_add'] | ||
|
||
if attr+'_remove' in overrides: | ||
for a in overrides[attr+'_remove']: | ||
attrs.remove(a) | ||
del overrides[attr+'_remove'] | ||
|
||
setattr(self.target_instance, attr, attrs) | ||
# Parse out features | ||
if 'target.features' in overrides: | ||
features = overrides['target.features'] | ||
self.remove_features(self.added_features - set(features)) | ||
self.add_features(features) | ||
self.removed_unecessary_features = True | ||
del overrides['target.features'] | ||
|
||
if 'target.features_add' in overrides: | ||
self.add_features(overrides['target.features_add']) | ||
del overrides['target.features_add'] | ||
|
||
if 'target.features_remove' in overrides: | ||
self.remove_features(overrides['target.features_remove']) | ||
del overrides['target.features_remove'] | ||
|
||
# Consider the others as overrides | ||
for name, v in overrides.items(): | ||
# Get the full name of the parameter | ||
full_name = ConfigParameter.get_full_name(name, unit_name, unit_kind, label) | ||
# If an attempt is made to override a parameter that isn't defined, raise an error | ||
if not full_name in params: | ||
raise ConfigException("Attempt to override undefined parameter '%s' in '%s'" % (full_name, ConfigParameter.get_display_name(unit_name, unit_kind, label))) | ||
params[full_name].set_value(v, unit_name, unit_kind, label) | ||
if full_name in params: | ||
params[full_name].set_value(v, unit_name, unit_kind, label) | ||
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 did you remove the error? Attempting to override a non-existing configuration parameter is supposed to raise an error. 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's for the config-in-feature-enabled-by-app issue. I'm currently looking into where to appropriately insert the config check. 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. Should be fixed in most recent patch, tested with config_test.py |
||
else: | ||
self.config_errors.append(ConfigException("Attempt to override undefined parameter '%s' in '%s'" | ||
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. Not sure why you need this? You need the config to continue going even if it finds an error? 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. Every pass it resets the errors (here). It could just save one error, but a list seemed more idomatic/potentially useful. 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. OK, but if that's the case, why did you do this change only here? There are still loads of places in config.py that raise ConfigException directly. Is there something that I'm missing? |
||
% (full_name, ConfigParameter.get_display_name(unit_name, unit_kind, label)))) | ||
return params | ||
|
||
# Read and interpret configuration data defined by targets | ||
|
@@ -345,3 +368,23 @@ def get_config_data_macros(self): | |
params, macros = self.get_config_data() | ||
self._check_required_parameters(params) | ||
return macros + self.parameters_to_macros(params) | ||
|
||
# Returns any features in the configuration data | ||
def get_features(self): | ||
params, _ = self.get_config_data() | ||
self._check_required_parameters(params) | ||
features = ((set(Target.get_target(self.target).features) | ||
| self.added_features) - self.removed_features) | ||
|
||
for feature in features: | ||
if feature not in self.__allowed_features: | ||
raise ConfigException("Feature '%s' is not a supported features" % feature) | ||
|
||
return features | ||
|
||
# Validate configuration settings. This either returns True or raises an exception | ||
def validate_config(self): | ||
if self.config_errors: | ||
raise self.config_errors[0] | ||
return True | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"target_overrides": { | ||
"*": { | ||
"target.features": ["IPV4", "IPV6"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# Testing basic features | ||
|
||
expected_results = { | ||
"K64F": { | ||
"desc": "test basic features", | ||
"expected_features": ["IPV4", "IPV6"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "lib1", | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV4"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV6"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# Testing when adding two features | ||
|
||
expected_results = { | ||
"K64F": { | ||
"desc": "test composing features", | ||
"expected_features": ["IPV4", "IPV6"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "lib1", | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV4"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "lib2", | ||
"target_overrides": { | ||
"*": { | ||
"target.features_remove": ["IPV4"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV6"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# Testing when two features collide | ||
|
||
expected_results = { | ||
"K64F": { | ||
"desc": "test feature collisions", | ||
"exception_msg": "Configuration conflict. Feature IPV4 both added and removed." | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "lib1", | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV6"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "lib2", | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["UVISOR"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"target_overrides": { | ||
"*": { | ||
"target.features_add": ["IPV4"] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks suspicious. Is the exception propagating correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3/reference/simple_stmts.html#raise
Propagating the exception carries the backtrace forward and helps significantly when debuggin the tools.