Skip to content

llext: make EDK Kconfig menu visible only when LLEXT is enabled #84850

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kartben
Copy link
Collaborator

@kartben kartben commented Jan 29, 2025

Properly guard LLEXT EDK menu to prevent associated options from bleeding into project conf even when LLEXT is disabled

Properly guard LLEXT EDK menu to prevent associated options
from bleeding into project conf even when LLEXT is disabled

Signed-off-by: Benjamin Cabé <[email protected]>
@kartben kartben added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Jan 29, 2025
@zephyrbot zephyrbot added the area: llext Linkable Loadable Extensions label Jan 29, 2025
@zephyrbot zephyrbot requested review from lyakh, pillo79 and teburd January 29, 2025 09:54
@pillo79
Copy link
Collaborator

pillo79 commented Jan 29, 2025

Pinging @edersondisouza - IIRC there were initially reasons for not pinning the EDK to CONFIG_LLEXT?

@kartben kartben removed the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Jan 29, 2025
Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

This was done by design as the EDK and LLEXT while a great pair are not necessarily both required to be functional. It's perfectly possible to build multiple images with a well defined API with the EDK that are already fully linked and placed without needing the LLEXT ELF loader.

@kartben
Copy link
Collaborator Author

kartben commented Jan 29, 2025

mmmh ok but feels kind of weird to end up with CONFIG_LLEXT_EDK_NAME="llext-edk" in zephyr/.config even when user has not "opted-in" for anything LLEXT related

@@ -113,8 +113,6 @@ module = LLEXT
module-str = llext
source "subsys/logging/Kconfig.template.log_config"

endif

menu "Linkable loadable Extension Development Kit (EDK)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Turn this into a menuconfig?

Copy link
Collaborator

@pillo79 pillo79 Jan 29, 2025

Choose a reason for hiding this comment

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

This was initially the case; then AFAICS this review comment resulted in CONFIG_LLEXT_EDK being removed during a review cycle.
(FWIW I'm all for re-adding it or making it dependent on CONFIG_LLEXT... just recalling history here).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@tejlmand tejlmand Feb 14, 2025

Choose a reason for hiding this comment

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

I guess you're looking for this comment: #69831 (comment)
Notice it was just an observation, not a request for change 😉

Main reason for that comment was because the llext-edk build target was a custom one, which needs to be called explicitly, thus indirectly calling this target means the user wants the output from llext-edk.

But I see the point that having llext config settings available if user has no intention of using llext-edk can be confusing.
The early assumption of just being able to call an llext-edk build target when needed might not make as much sense anymore if the user anyway has to make several decisions regarding llext-edk export options, which seems to be the case.

I'm fine with adding back the config part, and if doing so, then I also think the llext-edk target creation should be safeguarded, so that it's clear to users that there are extra settings to consider.
Of course that will imply that one cannot just call the llext-edk target on any build, but must remember to enable the config first.
I don't know how commen that is, @pillo79 any input ?

Updated: correct writing llext-edk instead of llext

Copy link
Collaborator

@pillo79 pillo79 Feb 14, 2025

Choose a reason for hiding this comment

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

As Ederson said just below, LLEXT and the EDK are somehow independent of each other.
Actually, in my PR I set the default to y if LLEXT since that was the de-facto situation, but I would also have preferred keeping that off unless explicitly asked for. There's already documentation on the EDK, I think if you are looking for that functionality, shouldn't be difficult to find out you have to enable it. @edersondisouza @teburd would you be OK with this?

I also have no problem on renaming LLEXT_EDK to EDK to make that extra clear; will patch #85624 to see how that looks like EDIT: approved, so will push a new PR with that change 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

As #84850 (comment) just below, LLEXT and the EDK are somehow independent of each other.

Sorry @pillo79 , I of course meant llext-edk in my earlier message, as only llext-edk has a dedicated build target.
So the question I tried to ask, how common is it to just call llext-edk on a build, and not having to adjust any of the llext-edk Kconfig settings for some reason.
I guess it will be ok for users to have to first set a CONFIG_LLEXT_EDK=y before being able to execute the target.

@edersondisouza
Copy link
Collaborator

edersondisouza commented Jan 29, 2025

Pinging @edersondisouza - IIRC there were initially reasons for not pinning the EDK to CONFIG_LLEXT?

Yeah, as @teburd explained, they are actually orthogonal. A menuconfig or even dropping the LLEXT bit from EDK - so configs become CONFIG_EDK_* - would be better (although I guess that for the later, the ship has sailed).

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: llext Linkable Loadable Extensions Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants