Convert the Pouch Golioth SDK to build as an external Zephyr Module/ESP-IDF Component#234
Convert the Pouch Golioth SDK to build as an external Zephyr Module/ESP-IDF Component#234szczys wants to merge 4 commits into
Conversation
9820362 to
d713f91
Compare
| if(NOT _golioth_sdk) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
Can this even be reached with the fatal error above?
There was a problem hiding this comment.
Yes, I think we both.
_golioth_sdkis set to either True or False bygolioth_sdk_config_enabled(_golioth_sdk CONFIG_GOLIOTH)whether it isy,n,"", or not selected.golioth_sdk_require_defined_vars()asserts that `_golioth_sdk' is definedif(NOT _golioth_sdk)returns early if CONFIG_GOLIOTH is not set toy
|
|
||
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| list(APPEND EXTRA_ZEPHYR_MODULES "${CMAKE_CURRENT_LIST_DIR}/../../../golioth_sdk") |
There was a problem hiding this comment.
Could we just put this in the zephyr cmake file? I understand wanting to avoid the upward dependency, but this is a bit of a faff to have to do for the 99% case of using pouch with golioth
There was a problem hiding this comment.
I think it makes more sense in the examples because there is no Pouch dependency on Golioth SDK, the dependency is in the examples that use them. This matches up with the idf_component.yml files in the ESP-IDF examples. If we move these to port/zephyr/CMakeLists.txt it hides the dependency from the user.
However, if we think about the eventual goal of spinning Golioth SDK into a standalone repo, it will then need to go into the west.yml at the Pouch level, which contradicts my first point. But I still think visibility is better at the example level.
There was a problem hiding this comment.
I get the dependency thing, but since the pouch without golioth scenario is a theoretical future scenario at this point, I lean towards going for a pragmatic approach, to be honest.
From the user's point of view, it's not clear why we're doing this, what it really accomplishes, or why it's necessary, and the relative path doesn't really work for real applications, as they're built in a separate repository in a location that doesn't require this repository to sit in a specific relative location.
Without this line, I could copy the example directory into a separate directory somewhere (doesn't even have to be in the west workspace if I use the ZEPHYR_BASE env var!) - and it would build just fine. I could make that directory a repository, and my colleagues would be able to clone it wherever they see fit, and build it. Both these properties break when we force the application cmakefile to sit in a specific location relative to the pouch repo.
If we make the golioth SDK a separate repository in the future, I assume we'd make pouch a dependency in its manifest, which wouldn't require any relative paths or altering of zephyr modules either. Even if we require two separate entries in the manifest, we would still be leaving module resolution to west. From my point of view, ZEPHYR_MODULES is an escape hatch for use cases where west just doesn't work, and should not ever be used directly if it can be avoided, as it's almost impossible to explain the difference between a zephyr module and a west project to people that only want to build their applications.
I like the dependency graph property and the way it mirrors the esp idf module, but I'd be confused and frustrated by the developer experience this introduces.
I'm looking for a compromise by suggesting that we do this from the port/zephyr directory, but to be honest, I'm not sure I understand what problem we're solving by making this a separate zephyr module in the first place. When it has real consequences to the developer experience, I want to push back.
There was a problem hiding this comment.
It seems like there is no appetite for building golioth_sdk as a module unless it is in its own repository and we are not ready to make that split. I've posted #240 as an alternative that isolates the build from pouch, but includes it as a subdirectory instead of as a module. If that PR is a reasonable solution we should merge it and close this one.
| zephyr_library_sources(zcbor_utils.c) | ||
| if(ESP_PLATFORM) | ||
| include(${CMAKE_CURRENT_LIST_DIR}/cmake/esp_idf_component.cmake) | ||
| elseif(COMMAND zephyr_library_named) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the reference. I'm pretty torn on the best approach. I hadn't considered using Zephyr_DIR but I did think about using ZEPHYR_BASE but thought keying on env vars might be brittle. Glad to have a source of truth from the Zephyr tree on what to do.
| endif() | ||
|
|
||
| zephyr_library() | ||
| get_registered_linker_files(_golioth_sdk_linker_files) |
There was a problem hiding this comment.
This file is the only place where the linker files are set and fetched. Can we just create the list locally? This kinda applies to all the global properties - they could just be regular variables here, as far as I can tell?
There was a problem hiding this comment.
Thanks for the callout on this. The registration system is a big hammer for a small problem. I removed all of the helper functions for this and updated CMakeLists.txt to simply append the file paths without extension. These are then processed in the port files to add the extension. I think this is better because the platform-specific extensions are now handled by the platform and not by the utils file.
Update golioth_sdk to isolate it from Pouch and build it as an ESP-IDF component. Signed-off-by: Mike Szczys <michael.szczys@canonical.com>
Add the zephyr/module.yml necessary to use golioth_sdk as a Zephyr module. Since it still lives in the Pouch repository, we need to append the directory to the EXTRA_ZEPHYR_MODULES list in cmake of each example app. Signed-off-by: Mike Szczys <michael.szczys@canonical.com>
This file was left empty after we removed the lib directory. Remove the file and references to it. Signed-off-by: Mike Szczys <michael.szczys@canonical.com>
- Ignore the dependencies.lock files generated by IDF component manager. - Add section headers to keep gitignore organized. Signed-off-by: Mike Szczys <michael.szczys@canonical.com>
d713f91 to
c4b31f8
Compare
Modularize the Golioth SDK so that it can be conditionally built as a module/component of an application that uses Pouch.
Note that the Golioth SDK is still fully dependent on Pouch as it uses core functions and elements of the port layer. However, once this PR merges, it could live in a separate repository from Pouch and build without issue.
Implementation Challenges
While the main goal was to convert this to a module, the implementation goal was to use one CMake path that presents all sources, header files, and linker files in one place. The following quirks should be noted as a result of this goal:
target_sources()instead oflist(APPENDbut due to how ESP-IDF works, we do not have access to the target name until afteridf_register_componentis called and we cannot runtarget_sourcesduring early expansion (see above). If we callidf_register component()without sources the build defaults to INTERFACE only. If we call it with a dummy file we get around that, but by that time it's too late for linker fragments to be added. Creating the lists and then consuming them from the port-specific cmake files is by far the cleanest solution available.resolves: https://github.com/golioth/firmware-issue-tracker/issues/967
resolves: https://github.com/golioth/firmware-issue-tracker/issues/990
resolves: https://github.com/golioth/firmware-issue-tracker/issues/991