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

Check path existence #246

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

BenArtes
Copy link

Resolves #232

Implementation Details: For REQUIRED COMPONENTS listed in a project's find_package, the existence of paths are checked after all possible ENV or default substitutions.

Background: BSP, CMSIS, FreeRTOS, and HAL support are implemented as CMake modules that are found using CMake's find_package. The find_package call specifies whether the package and any listed components are REQUIRED. The list of components is passed to the module using the <package>_FIND_COMPONENTS variable, and whether they are required can be checked using the <package>_FIND_REQUIRED_<component> variable.

The first commits implemented just the existence check, which broke the tests in /test. The readme suggests the following usage of COMPONENTS:

You can specify STM32 family or even specific device (STM32F407VG) in COMPONENTS or omit COMPONENTS totally - in that case stm32-cmake will find ALL sources for ALL families and ALL chips (you'll need ALL STM32Cube packages somewhere).

Details about find_package and COMPONENTS:

  • find_package(package) - All components are included by default and set to OPTIONAL
  • find_pacakge(package COMPONENTS REQUIRED) - All components are included by default and set to OPTIONAL; This seems weird as the package is marked as required but the path isn't checked for existence because there aren't any components to be checked / they are optional by default?
  • find_package(package COMPONENTS STM32... REQUIRED) - Listed Components are marked as REQUIRED and the existence of _PATHS is checked.

Checklist:

  • Are any paths that should be checked missing?
  • How to handle find_package(package REQUIRED)?
  • How to handle find_package(package COMPONENTS REQUIRED)?
  • Do the tests need to be adjusted?
    • Should tests list TEST_FAMILIES COMPONENTS or should REQUIRED be removed?
    • Should tests check that non-fetched packges fail if REQUIRED?
    • Should tests check that non-fetched packages DON'T fail if OPTIONAL?

List of Paths checked:

  • FindBSP
    • STM32_CUBE_${FAMILY}_PATH
  • FindCMSIS
    • STM32_CMSIS_${FAMILY}_PATH
    • STM32_CUBE_${FAMILY}_PATH
  • FindFreeRTOS
    • FREERTOS_PATH
  • FindHAL
    • STM32_HAL_${FAMILY}_PATH
    • STM32_CUBE_${FAMILY}_PATH

Risks with this PR: Due to previous silent failing and usage of REQUIRED in find_package this PR could break previously working user code.

BenArtes added 3 commits July 23, 2021 09:29
…default PATHs, else FATAL_ERROR

(cherry picked from commit 59217734eada4e8661736ed994b44ae184bea5b8)
(cherry picked from commit 8246a97f86830396fde9d16e285089ff6da135c3)
@BenArtes
Copy link
Author

After further investigation on find_package and REQUIRED I've found a few important references:

1

find_package(package REQUIRED components) is alternate syntax for find_package(package COMPONENTS components REQUIRED); so find_package(package REQUIRED) is equivalent to find_package(package COMPONENTS REQUIRED)?

2

CMake documentation states:

... The REQUIRED option stops processing with an error message if the package cannot be found.

A package-specific list of required components may be listed after the COMPONENTS option (or after the REQUIRED option if present). Additional optional components may be listed after OPTIONAL_COMPONENTS. Available components and their influence on whether a package is considered to be found are defined by the target package.

This is further backed up by Professional CMake 9th Ed which says on page 316:

If a package defines components but no components are given to find_package(), it is up to the
module or config definition how this is handled. For some packages, it may be treated as though all
components were listed, for others it may be interpreted as no components are required (basic
details of the package may still be defined though, such as base libraries, package version, etc.).
Another possibility is that the lack of components could be treated as an error. Given the variation
in behavior, developers should consult the documentation for the package they wish to find

There are three possible behaviors for find_package with no components listed:

  1. Try all packages and do nothing if no path exists
  2. Try all packages and do nothing if at least one package's path exists; else ERROR
  3. Try all packages and do nothing if ALL package paths exists; else ERROR

I assume that find_package(package) should do 1. find_package(package REQUIRED) and find_package(package COMPONENTS REQUIRED) currently also do 1; it needs to be decided whether we want to change this behavior to 2 or 3.

@BenArtes
Copy link
Author

Final Comments:

CMake documentation recommends using package_NOT_FOUND_MESSAGE and package_NOT_FOUND to indicate failure rather than FATAL_ERROR. While trying to implement this I noticed that these modules are using the FindPackageHandleStandardArgs module with the HANDLE_COMPONENTS argument to automatically handle setting of package_NOT_FOUND based on success of finding required COMPONENTS.

My final suggestion so to revert the FATAL_ERROR to WARNING and allow FindPackageHandleStandardArgs to handle the errors accordingly.

  • For find_package(package),find_package(package REQUIRED), and find_package(package COMPONENTS REQUIRED) it will silently fail on non-existent paths, but the build will succeed if atleast one exists.
  • For find_package(package REQUIRED components) and find_package(package COMPONENTS components REQUIRED) it will fail if ANY of the components is missing with a WARNING that the PATH does not exist.

This doesn't change behavior or tests, but does indicate to users the cause of the failure / error.

Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

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

Thank you for the explanations, this is all very clear.

We discussed with @Hish15 and your final proposal seems to be best choice:

For find_package(package),find_package(package REQUIRED), and find_package(package COMPONENTS REQUIRED) it will silently fail on non-existent paths, but the build will succeed if atleast one exists.

To recap find_package(package REQUIRED) will fail only if no package is found and will success if at least one package is found.

@@ -77,9 +77,13 @@ foreach(COMP ${CMSIS_FIND_COMPONENTS})

if((NOT STM32_CMSIS_${FAMILY}_PATH) AND (NOT STM32_CUBE_${FAMILY}_PATH))
set(STM32_CUBE_${FAMILY}_PATH /opt/STM32Cube${FAMILY} CACHE PATH "Path to STM32Cube${FAMILY}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

here the idea of #232 is to check if the path exists before using default.

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

cmake/FindCMSIS.cmake Show resolved Hide resolved
@atsju
Copy link
Collaborator

atsju commented Aug 10, 2021

Hello @BenArtes Sorry for delay.
Could you please resolve conflict with main branch ?
Is this PR complete for review or are there still some changes needed ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check all default paths existence before setting them
2 participants