-
Notifications
You must be signed in to change notification settings - Fork 3k
eclipse_gcc_arm export improvement #10295
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
eclipse_gcc_arm export improvement #10295
Conversation
@AndriiLishchynskyi, thank you for your changes. |
tools/export/cdt/__init__.py
Outdated
if configuration in target_specific[tgt.name]: | ||
eclipse_config.update(target_specific[tgt.name][configuration]) | ||
aliases = self.toolchain.target.resolution_order_names \ | ||
+ self.toolchain.target.extra_labels |
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.
How is this different from self.toolchain.target.labels
? Is that something that you could use instead?
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.
It is different a bit, but it is also ok, so I switched to self.toolchain.target.labels
tools/export/cdt/__init__.py
Outdated
if target_alias: | ||
eclipse_config.update(target_specific[target_alias]['generic']) | ||
if configuration in target_specific[target_alias]: | ||
eclipse_config.update(target_specific[target_alias][configuration]) |
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.
I would write this differently, as I generally avoid using the next
function:
for target_alias in aliases:
if target_alias in target_specific:
eclipse_config.update(target_specific[target_alias]['generic'])
if configuration in target_specific[target_alias]:
eclipse_config.update(target_specific[target_alias][configuration])
return eclipse_config
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.
Applied your fix, thanks.
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 dropped (or mis-indented) the return, which makes the code apply all possible target specific settings from most specific to least specific, as opposed to what it used to do which is apply the first match or most specific match.
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.
Missed that. I added a break instead of return, because we still have to return defaults, even if specific settings does not exist.
Get rid of 'next' function
CI started |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
Description
Current implementation of cdt exporter requires modification of
cdt_definitions.json
in order to support launch configuration for new kit (double-core). Usually launch configuration does not change if devices are derived from the same family. This pull request reduces maintenance burden of new kits support by allowing them to be based on extra_labels and not necessarily defined for each kit separately.Also this pull request fixes:
cdt_definitions.json
clean-upAll internal tests are passed
Pull request type
Reviewers
@theotherjimmy
Release Notes