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

cmake(enhance):enhance NuttX cmake target_dependencies and link_library modules #14907

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

xuxin930
Copy link
Contributor

Summary

After #14747, we have a preliminary extensions design : )
I have two enhance ideas here:

1.link_library

In the Nuttx CMake build system, we cannot call target_link_library
Why?
this is because we need to consider different build MODES,
different static libs need to be connected to the kernel or user blob,
which results in the link target of nuttx to be manually controlled.
Refer to the implementation of add_library
This creates a problem that goes against the intuition of using CMake.
target_link_library cannot be called, otherwise the problem of multi defines will occur during linking.
ref: #14757 and apache/nuttx-apps#2846

So I enhance target_link_library to nuttx_link_library,
It only inherits the public properties of the linked one without actually connecting it.

2.add_dependencies

The NuttX CMake build system executes sequentially when traversing all subdirectories.
This will also cause a problem, that is, when using add_dependencies(targetA targetB), targetB may not have been traversed yet.

So I enhanced nuttx_add_dependencies, which only records dependencies and mappings when called,
and actually executes dependencies after all targets are traversed.

3.other shortcut calls

set scope is all the APPS include search path, compile_options, definitions.

nuttx_include_directories_for_all_apps()
nuttx_compile_options_for_all_apps()
nuttx_compile_definitions_for_all_apps()

Impact

Enhanced functions, fully compatible with existing cases

Testing

#  use extensions call in apps lvgl CMakeLists.txt

-  set_property(
-    TARGET nuttx
-    APPEND
-    PROPERTY NUTTX_INCLUDE_DIRECTORIES ${CMAKE_CURRENT_LIST_DIR}
-             ${CMAKE_CURRENT_LIST_DIR}/lvgl)
-
-  set_property(
-    TARGET nuttx
-    APPEND
-    PROPERTY NUTTX_COMPILE_OPTIONS -DLV_USE_DEV_VERSION=1)
+
+  nuttx_include_directories_for_all_apps(${CMAKE_CURRENT_LIST_DIR} ${CMAKE_CURRENT_LIST_DIR}/lvgl)
+  nuttx_compile_definitions_for_all_apps(LV_USE_DEV_VERSION=1)

test build with:

cmake -B build -DBOARD_CONFIG=qemu-armv7a:full -GNinja

build pass

…ry modules

Enhance CMake's add_dependencies for Nuttx so that
different targets can call dependencies without errors when they are not traversed.

In addition, since we do not call link_library directly,
we increment nuttx_link_library to inherit the PUBLIC property

Signed-off-by: xuxin19 <[email protected]>
@github-actions github-actions bot added Area: Build system Size: M The size of the change in this PR is medium labels Nov 22, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 22, 2024

[Experimental Bot, please feedback here]

This PR description could be improved by providing more specific details and adhering more closely to the requested format. Here's a revised version:

Summary

This PR enhances the NuttX CMake build system to address limitations related to linking and dependencies, improving usability and compatibility with standard CMake practices.

  • Why change is necessary (fix, update, new feature)? This PR fixes limitations in the NuttX CMake build system that prevented the use of standard CMake commands like target_link_library and add_dependencies due to NuttX's specific build process and multi-mode build requirements. It also adds convenient shortcut functions for setting include directories, compile options, and definitions across all apps.
  • What functional part of the code is being changed? The cmake/ directory, specifically by introducing new CMake functions: nuttx_link_library, nuttx_add_dependencies, nuttx_include_directories_for_all_apps, nuttx_compile_options_for_all_apps, and nuttx_compile_definitions_for_all_apps.
  • How does the change exactly work (what will change and how)?
    • nuttx_link_library mimics target_link_library but only inherits public properties without directly linking, avoiding multiple definition errors.
    • nuttx_add_dependencies records dependencies and applies them after all targets are processed, addressing issues with build order.
    • The shortcut functions provide a concise way to apply settings to all apps.
  • Related NuttX Issue reference if applicable. cmake(bugfix):remove no need target_link_library in static target #14757 (mentioning the original problem)
  • Related NuttX Apps Issue / Pull Request reference if applicable. cmake(bugfix):add INCLUDE_DIRECTORIES for nimble nuttx-apps#2846 (also referencing the original problem)

Impact

  • Is new feature added? Is existing feature changed? New features are added (nuttx_link_library, nuttx_add_dependencies, and the shortcut functions). Existing build behavior is not changed for projects not using these new features.
  • Impact on user (will user need to adapt to change)? YES. Users can now leverage nuttx_link_library and nuttx_add_dependencies, and the shortcut functions simplify common tasks. Existing code using workarounds for the previous limitations can be simplified.
  • Impact on build (will build process change)? NO for existing projects. The underlying build process remains the same. The changes streamline how dependencies and linking are handled within CMake.
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
  • Impact on documentation (is update required / provided)? YES. Documentation should be updated to explain the new CMake functions and their usage. This PR should include the updated documentation.
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? Backward compatible. Existing CMakeLists.txt files will continue to work.
  • Anything else to consider? These changes improve the developer experience by aligning the NuttX build system closer to standard CMake practices.

Testing

  • Build Host(s): [Specify your build host details: OS, CPU, Compiler and version] e.g., Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
  • Target(s): qemu-armv7a:full

Testing logs before change:

[Provide logs demonstrating the issue encountered before your changes, such as linker errors due to multiple definitions or incorrect build order.]

Testing logs after change:

[Show successful build logs after applying your changes.  Highlight the absence of previous errors and correct build order.]

This improved description provides more specific details about the changes, their impact, and the testing performed, making it easier for reviewers to understand and approve the PR. Remember to fill in the bracketed placeholders with your specific information.

@acassis acassis merged commit cfe78ad into apache:master Nov 22, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build system Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants