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.

Sign up for GitHub

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: mcuboot: remove obsolete check for west to sign #86439

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+0 −5
Open

cmake: mcuboot: remove obsolete check for west to sign #86439

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions cmake/mcuboot.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@ function(zephyr_mcuboot_tasks)
endif()
endif()

if(NOT WEST)
# This feature requires west.
message(FATAL_ERROR "Can't sign images for MCUboot: west not found. To fix, install west and ensure it's on PATH.")
endif()

foreach(file keyfile keyfile_enc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the WEST_TOPDIR parts below I would assume would be blank if west is not used

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch.

Perhaps we should treat relative paths relative to Zephyr base ?
Though the problem with that would be that it's a breaking change for those who might depends on the path relative to west topdir.

But having a different relative to west topdir when west is present and relative to zephyr base when west is not present is even worse.

For relative files only, then perhaps the best option is to check for the file relative to ZEPHYR_BASE and if found there, then use that file.
If not found there and west is installed, then check relative to west topdir, and it found there, use that file but also print a deprecation warning that relative to west topdir will be removed in the future.

@nordicjm toughts ?

Copy link
Author

@ckhardin ckhardin Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

for what it is worth - from the this cmake code - we are a custom zephyr project so we set absolute paths when we call into zephyr to avoid relative issues like the concern. So, the lack of west doesn't hurt us and the code for WEST_TOPDIR doesn't get invoked.

From cmake/modules/west.cmake

  execute_process(
    COMMAND ${WEST} topdir
    OUTPUT_VARIABLE WEST_TOPDIR
    ERROR_QUIET
    RESULT_VARIABLE west_topdir_result
    OUTPUT_STRIP_TRAILING_WHITESPACE
    WORKING_DIRECTORY ${ZEPHYR_BASE}
    )

  if(west_topdir_result)
    # west topdir is undefined.
    # That's fine; west is optional, so could be custom Zephyr project.
    set(WEST WEST-NOTFOUND CACHE INTERNAL "West")
  endif()

I would argue that if a change was desired just allow someone to set "WEST_TOPDIR" as a config output instead of calling "west topdir" and leave the rest of the logic as is. That seems to fit a lot of the style/patterns in the cmake setup so far - something like the following.

diff --git a/firmware/zephyr/cmake/modules/west.cmake b/firmware/zephyr/cmake/modules/west.cmake
index f35e7a3c..cf7a46e9 100644
--- a/firmware/zephyr/cmake/modules/west.cmake
+++ b/firmware/zephyr/cmake/modules/west.cmake
@@ -80,18 +80,20 @@ else()
   # will still work even after output is one line.
   message(STATUS "Found west (found suitable version \"${west_version}\", minimum required is \"${MIN_WEST_VERSION}\")")
 
-  execute_process(
-    COMMAND ${WEST} topdir
-    OUTPUT_VARIABLE WEST_TOPDIR
-    ERROR_QUIET
-    RESULT_VARIABLE west_topdir_result
-    OUTPUT_STRIP_TRAILING_WHITESPACE
-    WORKING_DIRECTORY ${ZEPHYR_BASE}
-    )
+  if(NOT DEFINED WEST_TOPDIR)
+    execute_process(
+      COMMAND ${WEST} topdir
+      OUTPUT_VARIABLE WEST_TOPDIR
+      ERROR_QUIET
+      RESULT_VARIABLE west_topdir_result
+      OUTPUT_STRIP_TRAILING_WHITESPACE
+      WORKING_DIRECTORY ${ZEPHYR_BASE}
+      )
 
-  if(west_topdir_result)
-    # west topdir is undefined.
-    # That's fine; west is optional, so could be custom Zephyr project.
-    set(WEST WEST-NOTFOUND CACHE INTERNAL "West")
+    if(west_topdir_result)
+      # west topdir is undefined.
+      # That's fine; west is optional, so could be custom Zephyr project.
+      set(WEST WEST-NOTFOUND CACHE INTERNAL "West")
+    endif()
   endif()
 endif()

if(NOT "${${file}}" STREQUAL "")
if(NOT IS_ABSOLUTE "${${file}}")
Expand Down
Loading