-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Mbed OS 5.15: Warn on unrecognised error #13031
Mbed OS 5.15: Warn on unrecognised error #13031
Conversation
@dgreen-arm, thank you for your changes. |
We discussed this internally, will be updated to provide better backward compatible way on master and 5.15 |
This PR will be updated with the new approach. The purpose of this PR was to prevent errors when using the EXPERIMENTAL_API flag, and there are multiple ways to accomplish that. |
Use "not a supported feature" instead of "not a supported features".
a33b45a
to
4f991d0
Compare
raise ConfigException( | ||
"Feature '%s' is not a supported features" % feature) | ||
print("[WARNING] Feature '%s' is not a supported feature" % feature) | ||
self.target.features.remove(feature) |
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.
Removing this feature will prevent that feature from being defined or having any effect, which is a nice guarantee.
@@ -1354,10 +1354,10 @@ def get_features(self): | |||
self.cumulative_overrides['features']\ | |||
.update_target(self.target) | |||
|
|||
for feature in self.target.features: | |||
for feature in list(self.target.features): |
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.
You might consider adding a comment why this needs to be a list (because we are modifying the features list and need to iterate on a copy).
Give a warning rather than error if an unrecognised feature is used. This will help compatibility when new features are added. Signed-off-by: Darryl Green <darryl.green@arm.com>
4f991d0
to
cd1b59b
Compare
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.
LGTM
I'll start CI once the other job 6.0 rc2 completes |
CI started |
Test run: FAILEDSummary: 1 of 10 test jobs failed Failed test jobs:
|
CI started |
Test run: FAILEDSummary: 2 of 10 test jobs failed Failed test jobs:
|
CI restarted |
Test run: SUCCESSSummary: 10 of 10 test jobs passed |
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.
Approved
Summary of changes
Mbed OS will be adding features that are experimental under a FEATURE_EXPERIMENTAL_API folder. This caused compatibility issues, as the EXPERIMENTAL_API feature is not recognised in all examples. This PR changes the error that's raised when a feature is unrecognised to a warning.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers