-
Notifications
You must be signed in to change notification settings - Fork 25
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
configure: Don't emit _PRESENT macros #78
Conversation
bbe966d
to
3e28dba
Compare
@@ -47,7 +47,7 @@ def _render_mbed_config_cmake_template( | |||
""" | |||
env = jinja2.Environment(loader=jinja2.PackageLoader("mbed_tools.build", str(TEMPLATES_DIRECTORY)),) | |||
template = env.get_template(TEMPLATE_NAME) | |||
options = list(config.options.values()) | |||
options = [x for x in config.options.values() if not x.key.endswith(".present")] |
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.
Would it not be better to apply this filtering where we create the config "options" here
for key, value in source.config.items(): |
TODO
to remove this when we refactor the mbed 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.
I'm not sure what refactoring of mbed config you have in mind here. Perhaps we should raise a GitHub issue for it?
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 meant refactoring of the mbed configuration system to remove these macros from the mbed_lib.json files.
3e28dba
to
d2e6418
Compare
Do not put macros like MBED_CONF_FILESYSTEM_PRESENT or MBED_CONF_EVENTS_PRESENT into the applicaiton's generated mbed_config.cmake file. We will depend on Mbed OS CMake files for each of these components to define the _PRESENT macro in a target_compile_definitions() block for the e.g. filesystem or events CMake targets.
d2e6418
to
9ffb7db
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
Description
Do not put macros like MBED_CONF_FILESYSTEM_PRESENT or
MBED_CONF_EVENTS_PRESENT into the applicaiton's generated
mbed_config.cmake file. We will depend on Mbed OS CMake files for each
of these components to define the _PRESENT macro in a
target_compile_definitions() block for the e.g. filesystem or events
CMake targets.
This should be merged immediately before the Mbed OS companion PR is ready. Then, we should merge this PR, do a tools release, merge Mbed OS example PRs, and merge the Mbed OS PR. Without Mbed OS having updated CMake files, these tool changes will break some builds.
Test Coverage