Skip to content

Remove unused CMake functions and default.cmake #172

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

Open
devin-ai-integration wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

Remove unused CMake functions and default.cmake #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12604c6
Build Fixes (#5280)
LeStarch Jun 11, 2026
5b89ff1
Remove unused CMake functions and default.cmake
devin-ai-integration[bot] Jun 11, 2026
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
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ cmake_minimum_required(VERSION 3.18)
project(FPrime C CXX)
set(FPRIME_INCLUDE_FRAMEWORK_CODE OFF)
find_package(FPrime REQUIRED PATHS "${CMAKE_CURRENT_LIST_DIR}")
register_fprime_project()

# Set default warning flags for all builds
# Specific build modules that do not comply with these flags can disable one or more of them
Expand Down
7 changes: 6 additions & 1 deletion cmake/implementation.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ include(config_assembler)
function(fprime_target_implementations BUILD_SYSTEM_TARGET)
append_list_property("${ARGN}" TARGET "${BUILD_SYSTEM_TARGET}" PROPERTY FPRIME_CHOSEN_IMPLEMENTATIONS)
fprime__internal_choose_implementations("${BUILD_SYSTEM_TARGET}" INTERNAL_ALL_IMPLEMENTATIONS)
fprime_target_dependencies("${BUILD_SYSTEM_TARGET}" PRIVATE "${INTERNAL_ALL_IMPLEMENTATIONS}")

set(LINK_SCOPE)
if (NOT FPRIME_USE_PLAIN_LINK_SIGNATURE)
set(LINK_SCOPE PRIVATE)
endif()
fprime_target_dependencies("${BUILD_SYSTEM_TARGET}" "${LINK_SCOPE}" "${INTERNAL_ALL_IMPLEMENTATIONS}")
Comment on lines +36 to +40

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔴 Empty LINK_SCOPE passed as empty string "" to target_link_libraries when FPRIME_USE_PLAIN_LINK_SIGNATURE is ON

When FPRIME_USE_PLAIN_LINK_SIGNATURE is set to a truthy value, LINK_SCOPE remains empty (set(LINK_SCOPE)). It is then passed quoted as "${LINK_SCOPE}" to fprime_target_dependencies, which propagates it through fprime_target_link_libraries (cmake/utilities.cmake:775) → fprime__internal_target_interceptor (cmake/utilities.cmake:746) → and finally to cmake_language(CALL "target_link_libraries" "${BUILD_SYSTEM_TARGET}" "" ${ARGN}) at cmake/utilities.cmake:735. The empty string "" is not a valid scope keyword (PUBLIC/PRIVATE/INTERFACE) and CMake will interpret it as a library name in the "plain" signature, causing a build error or invalid link. The intent was presumably to call target_link_libraries(target dep1 dep2...) without a scope keyword, but the interceptor at line 735 always forwards "${SCOPE}" as a separate argument even when empty.

Prompt for agents 
The problem is that when FPRIME_USE_PLAIN_LINK_SIGNATURE is ON, LINK_SCOPE is empty and passed as an empty string through the chain of fprime_target_dependencies -> fprime_target_link_libraries -> fprime__internal_target_interceptor. The interceptor at cmake/utilities.cmake:735 always passes "${SCOPE}" quoted, so an empty scope becomes an empty string argument to target_link_libraries rather than being omitted.

To fix this, there are two approaches:
1. Change the interceptor (cmake/utilities.cmake:735) to use unquoted ${SCOPE} instead of "${SCOPE}", so an empty scope gets omitted. This also requires similar changes in fprime_target_link_libraries (line 746) and fprime_target_dependencies (line 775).
2. Alternatively, bypass the interceptor chain entirely when LINK_SCOPE is empty and call target_link_libraries directly with just the target and dependencies (no scope keyword).

Note that fprime_target_include_directories was already updated to use unquoted ${SCOPE} at line 759, but the link libraries path was not similarly updated, creating an inconsistency.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is from the upstream commit Build Fixes (#5280), not from this PR's changes. See nasa#5284 for the actual upstream PR.

Comment on lines +36 to +40

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 FPRIME_USE_PLAIN_LINK_SIGNATURE is never defined anywhere in the codebase

The variable FPRIME_USE_PLAIN_LINK_SIGNATURE is only referenced at cmake/implementation.cmake:37 and is never defined or set anywhere in the repository. In CMake, if (NOT FPRIME_USE_PLAIN_LINK_SIGNATURE) evaluates to TRUE for an undefined variable, so LINK_SCOPE will always be set to PRIVATE. This means the current runtime behavior is identical to the old code (fprime_target_dependencies(... PRIVATE ...)). The variable appears to be a hook for future/external configuration, but since the code path when it IS set is broken (reported as BUG-0001), this feature is not usable.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

These three findings are all from the upstream commit Build Fixes (#5280) (12604c6) which was included in this fork PR's diff after rebasing onto upstream/devel. None of them are related to the changes in this PR (which only deletes default.cmake and removes unused functions from utilities.cmake). The real PR targeting upstream is nasa#5284.

endfunction()

####
Expand Down
50 changes: 0 additions & 50 deletions cmake/target/default.cmake
View file
Open in desktop

This file was deleted.

130 changes: 2 additions & 128 deletions cmake/utilities.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,115 +247,7 @@ function(linker_only OUTPUT_VAR TOKEN)
endif()
endfunction()

####
# build_relative_path:
#
# Calculate the path to an item relative to known build paths. Search is performed in the following order erring if the
# item is found in multiple paths.
#
# INPUT_PATH: input path to search
# OUTPUT_VAR: output variable to fill
####
function(build_relative_path INPUT_PATH OUTPUT_VAR)
# Implementation assertion
if (NOT DEFINED FPRIME_BUILD_LOCATIONS)
message(FATAL_ERROR "FPRIME_BUILD_LOCATIONS not set before build_relative_path was called")
endif()
normalize_paths(FPRIME_LOCS_NORM ${FPRIME_BUILD_LOCATIONS})
normalize_paths(INPUT_PATH ${INPUT_PATH})
foreach(PARENT IN LISTS FPRIME_LOCS_NORM)
string(REGEX REPLACE "${PARENT}/(.*)$" "\\1" LOC_TEMP "${INPUT_PATH}")
if (NOT LOC_TEMP STREQUAL INPUT_PATH AND NOT LOC_TEMP MATCHES "${LOC}$")
message(FATAL_ERROR "Found ${INPUT_PATH} at multiple locations: ${LOC} and ${LOC_TEMP}")
elseif(NOT LOC_TEMP STREQUAL INPUT_PATH AND NOT DEFINED LOC)
set(LOC "${LOC_TEMP}")
endif()
endforeach()
if (LOC STREQUAL "")
message(FATAL_ERROR "Failed to find location for: ${INPUT_PATH}")
endif()
set(${OUTPUT_VAR} ${LOC} PARENT_SCOPE)
endfunction(build_relative_path)

####
# on_any_changed:
#
# Sets VARIABLE to true if any file has been noted as changed from the "on_changed" function. Will create cache files
# in the binary directory. Please see: on_changed
#
# INPUT_FILES: files to check for changes
# ARGN: passed into execute_process via on_changed call
####
function (on_any_changed INPUT_FILES VARIABLE)
foreach(INPUT_FILE IN LISTS INPUT_FILES)
on_changed("${INPUT_FILE}" TEMP_ON_CHANGED ${ARGN})
if (TEMP_ON_CHANGED)
set(${VARIABLE} TRUE PARENT_SCOPE)
return()
endif()
endforeach()
set(${VARIABLE} FALSE PARENT_SCOPE)
endfunction()

####
# on_changed:
#
# Sets VARIABLE to true if and only if the given file has changed since the last time this function was invoked. It will
# create "${INPUT_FILE}.prev" in the binary directory as a cache from the previous invocation. The result is always TRUE
# unless a successful no-difference is calculated.
#
# INPUT_FILE: file to check if it has changed
# ARGN: passed into execute_process
####
function (on_changed INPUT_FILE VARIABLE)
get_filename_component(INPUT_BASENAME "${INPUT_FILE}" NAME)
set(PREVIOUS_FILE "${CMAKE_CURRENT_BINARY_DIR}/${INPUT_BASENAME}.prev")

execute_process(COMMAND "${CMAKE_COMMAND}" -E compare_files "${INPUT_FILE}" "${PREVIOUS_FILE}"
RESULT_VARIABLE difference OUTPUT_QUIET ERROR_QUIET)
# Files are the same, leave this function
if (difference EQUAL 0)
set(${VARIABLE} FALSE PARENT_SCOPE)
return()
endif()
set(${VARIABLE} TRUE PARENT_SCOPE)
# Update the file with the latest
if (EXISTS "${INPUT_FILE}")
execute_process(COMMAND "${CMAKE_COMMAND}" -E copy "${INPUT_FILE}" "${PREVIOUS_FILE}" OUTPUT_QUIET)
endif()
endfunction()

####
# read_from_lines:
#
# Reads a set of variables from a newline delimited test base. This will read each variable as a separate line. It is
# based on the number of arguments passed in.
####
function (read_from_lines CONTENT)
# Loop through each arg
foreach(NAME IN LISTS ARGN)
string(REGEX MATCH "^([^\r\n]+)" VALUE "${CONTENT}")
string(REGEX REPLACE "^([^\r\n]*)\r?\n(.*)" "\\2" CONTENT "${CONTENT}")
set(${NAME} "${VALUE}" PARENT_SCOPE)
endforeach()
endfunction()

####
# Function `full_path_from_build_relative_path`:
#
# Creates a full path from the shortened build-relative path.
# -**SHORT_PATH:** build relative path
# Return: full path from relative path
####
function(full_path_from_build_relative_path SHORT_PATH OUTPUT_VARIABLE)
foreach(FPRIME_LOCATION IN LISTS FPRIME_BUILD_LOCATIONS)
if (EXISTS "${FPRIME_LOCATION}/${SHORT_PATH}")
set("${OUTPUT_VARIABLE}" "${FPRIME_LOCATION}/${SHORT_PATH}" PARENT_SCOPE)
return()
endif()
endforeach()
set("${OUTPUT_VARIABLE}" "" PARENT_SCOPE)
endfunction(full_path_from_build_relative_path)

####
# Function `get_nearest_build_root`:
Expand Down Expand Up @@ -575,25 +467,7 @@ function(append_list_property NEW_ITEM)
set_property(${ARGN} "${LOCAL_COPY}")
endfunction()

####
# Function `filter_lists`:
#
# Filters lists set in ARGN to to ensure that they are not in the exclude list. Sets the <LIST>_FILTERED variable in
# PARENT_SCOPE with the results
# **EXCLUDE_LIST**: list of items to filter-out of ARGN lists
# **ARGN:** list of list names in parent scope to filter
####
function (filter_lists EXCLUDE_LIST)
foreach(SOURCE_LIST IN LISTS ARGN)
set(${SOURCE_LIST}_FILTERED "")
foreach(SOURCE IN LISTS ${SOURCE_LIST})
if (NOT SOURCE IN_LIST EXCLUDE_LIST)
list(APPEND ${SOURCE_LIST}_FILTERED "${SOURCE}")
endif()
endforeach()
set(${SOURCE_LIST}_FILTERED "${${SOURCE_LIST}_FILTERED}" PARENT_SCOPE)
endforeach()
endfunction(filter_lists)


####
# Function `get_fprime_library_option_string`:
Expand Down Expand Up @@ -882,7 +756,7 @@ endfunction()
#
####
function(fprime_target_include_directories BUILD_TARGET_NAME SCOPE)
fprime__internal_target_interceptor("target_include_directories" "${BUILD_TARGET_NAME}" "${SCOPE}" ${ARGN})
fprime__internal_target_interceptor("target_include_directories" "${BUILD_TARGET_NAME}" ${SCOPE} ${ARGN})
endfunction()
Comment on lines 758 to 760

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 Asymmetric quoting between fprime_target_include_directories and fprime_target_link_libraries

At cmake/utilities.cmake:759, the SCOPE variable is now passed unquoted (${SCOPE}) to fprime__internal_target_interceptor, while the parallel function fprime_target_link_libraries at cmake/utilities.cmake:746 still uses quoted "${SCOPE}". This is functionally equivalent for the current single caller (cmake/API.cmake:471 which always passes PUBLIC), but creates an inconsistency in the API contract. If fprime_target_include_directories is ever called with an empty SCOPE, the unquoted expansion would cause the first ARGN element to be captured as SCOPE in the interceptor. This appears to be an intentional change to support the plain signature pattern (matching the FPRIME_USE_PLAIN_LINK_SIGNATURE intent in implementation.cmake), but only this one function was updated while the rest of the chain was not.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This asymmetry is from the upstream commit Build Fixes (#5280), not from this PR's changes.


####
Expand Down
Loading