Skip to content
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

Enable tristate Kconfig options #67105

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Enable tristate Kconfig options #67105

merged 2 commits into from
Jan 11, 2024

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Jan 2, 2024

kconfiglib.py has a hard dependency on CONFIG_MODULES to support 'm' values for tristate Kconfig options. It's a logical companion for but can also be used with other configurations.

pillo79
pillo79 previously approved these changes Jan 2, 2024
Copy link
Collaborator

@pillo79 pillo79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the potential of having tristate options in Zephyr, and it's OK to keep the CONFIG_MODULES / =m names as they are widely used in Kconfig-based projects.
Minor tip: I would add a depends on LLEXT so that it is hidden unless dynamic code loading is actually used in the build.

tejlmand
tejlmand previously approved these changes Jan 4, 2024
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

As this setting is currently intended for llext then current location is acceptable,
but has been considered if subsys/Kconfig is the right location for this setting?
Another option could be under Build and Link features where other settings related to build and configuration is controlled, ref:

Approved for now, as we can always move the setting later if desired.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 8, 2024

Thanks for the reviews so far! In principle I'm not against any of the proposals - neither against adding a CONFIG_LLEXT dependency, nor against moving to a different location, but I would feel more confident making those changes if I had more than one vote in favour of either of them :-) Otherwise of course, as has been mentioned, those changes can be added later

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 9, 2024

The Compliance Check is complaining about:

Boolean option 'MODULES' prompt must not start with 'Enable...'

Indeed, the CONFIG_MODULES prompt in this commit begins with "enable" but I don't find a better description. Any ideas or can we keep it?

@nashif
Copy link
Member

nashif commented Jan 9, 2024

would be great and very helpful to have a usecase and a sample of this kconfig being used. Just throwing in a kconfig in a PR is usually not enough....

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 9, 2024

would be great and very helpful to have a usecase and a sample of this kconfig being used. Just throwing in a kconfig in a PR is usually not enough....

@nashif the use-case is LLEXT and a sample is SOF thesofproject/sof@614ec42#diff-44c49245e3507ea76cafeca22e652e4f30a24495a147d0beb5681a7bad5b0b71

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 9, 2024

would be great and very helpful to have a usecase and a sample of this kconfig being used. Just throwing in a kconfig in a PR is usually not enough....

Same problem in related:

I think the biggest llext gap right now is around CMake.

I understand brand new features are made of many different parts and that they can't all be merged at the same time. On the other hand, there also seems to be a common misconception that the main branch is the only way to share incomplete bits still in progress.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 10, 2024

Same problem in related:

* https://github.com/zephyrproject-rtos/zephyr/pull/66573

@marc-hb I don't think it's the same "problem" - @nashif commented on "Just throwing in a kconfig in a PR" and that other PR doesn't add any Kconfig options.

I think the biggest llext gap right now is around CMake.

I agree that CMake for llext is a bit difficult now, but I wouldn't relate it to this PR. Again: this is purely to trigger scripts/kconfig/kconfiglib.py to accept tristate Kconfig options, nothing else. As is it's not directly related to llext or to any other subsystem.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 10, 2024

twister seems to have experienced a melt down on this PR, should I re-push to have a clean CI run?

kconfiglib.py has a hard dependency on CONFIG_MODULES to support 'm'
values for tristate Kconfig options. It's a logical companion for
but can also be used with other configurations.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add a tristate Kconfig option to the llext hello-world twister test.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@fabiobaltieri fabiobaltieri assigned tejlmand and unassigned teburd Jan 10, 2024
@fabiobaltieri
Copy link
Member

Switch to assignee just to confirm about the option in the top level subsys Kconfig. @tejlmand can you reconfirm please?

@nashif nashif merged commit aee2d1a into zephyrproject-rtos:main Jan 11, 2024
21 checks passed
@lyakh lyakh deleted the modules branch January 11, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: llext Linkable Loadable Extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants