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

if CPM_LOCAL_PACKAGES_ONLY, make find_package fail if it failed to find. #589

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

Conversation

Arniiiii
Copy link

I've found that if using CPM_LOCAL_PACKAGES_ONLY , it will make message(SEND_ERROR ...) about failure.

https://cmake.org/cmake/help/latest/command/message.html

  1. message(SEND_ERROR ...) not fails immediately, which makes CPM go further and download the package, though not allowing to generate
  2. It doesn't show error generated by find_package .

Firstly I thought to change message(SEND_ERROR ... to message(FATAL_ERROR ...)

But my real problem was that find_package have found a package with specified version, but not some of its components.

IDK how to get error message from find_package, that's why I've made that if CPM_LOCAL_PACKAGES_ONLY to just ask find_package to fail if it failed, and show its error logs by itself. And that's just by adding REQUIRED .

@Avus-c
Copy link
Contributor

Avus-c commented Aug 19, 2024

But my real problem was that find_package have found a package with specified version, but not some of its components.

IDK how to get error message from find_package, that's why I've made that if CPM_LOCAL_PACKAGES_ONLY to just ask find_package to fail if it failed, and show its error logs by itself. And that's just by adding REQUIRED

It seems like you called CPMFindPackage with FIND_PACKAGE_ARGS to specify components. You could have simply prepended REQUIRED to FIND_PACKAGE_ARGS to trigger an error. I may be wrong here, haven't used that feature alot. :)

I'm not a fan of forcing REQUIRED on all users, if they have the option to set it themself already.

message(SEND_ERROR ...) not fails immediately, which makes CPM go further and download the package, though not allowing to generate

I think this behavior comes down to personal preference. I prefer CPM to download all dependencies while I make ☕, regardless of whether an error occurs.

CPM.cmake/cmake/CPM.cmake

Lines 719 to 734 in 011d3dd

if(CPM_USE_LOCAL_PACKAGES)
cpm_find_package(${CPM_ARGS_NAME} "${CPM_ARGS_VERSION}" ${CPM_ARGS_FIND_PACKAGE_ARGUMENTS})
if(CPM_PACKAGE_FOUND)
cpm_export_variables(${CPM_ARGS_NAME})
return()
else()
string(REPLACE " " ";" EDITED_CPM_ARGS_FIND_PACKAGE_ARGUMENTS
"${CPM_ARGS_FIND_PACKAGE_ARGUMENTS}"
)
message(
WARNING
"${CPM_INDENT} ${CPM_ARGS_NAME} not found via find_package(${CPM_ARGS_NAME} ${CPM_ARGS_VERSION} ${EDITED_CPM_ARGS_FIND_PACKAGE_ARGUMENTS})"
)
endif()
endif()

If a local package isn't found and CPM falls back to FetchContent, we still print a warning. However, since this fallback is intentional and part of CPM's functionality, the warning might confuse users.

I`m not a maintainer, just interested in helping out wherever I can.

@Arniiiii
Copy link
Author

But my real problem was that find_package have found a package with specified version, but not some of its components.
IDK how to get error message from find_package, that's why I've made that if CPM_LOCAL_PACKAGES_ONLY to just ask find_package to fail if it failed, and show its error logs by itself. And that's just by adding REQUIRED

It seems like you called CPMFindPackage with FIND_PACKAGE_ARGS to specify components. You could have simply prepended REQUIRED to FIND_PACKAGE_ARGS to trigger an error. I may be wrong here, haven't used that feature alot. :)

I've called CPMAddPackage , but when asked CMake to generate I've added CPM_LOCAL_PACKAGES_ONLY=1, so I expected that on every failure of find_package it will fail. For everything else that has to be downloaded in any case, There's FORCE option for CPMAddPackage .

I'm not a fan of forcing REQUIRED on all users, if they have the option to set it themself already.

I thought that adding the keyword is logically correct ( with that PR ) :

  1. CPM_DOWNLOAD_ALL=1 (default behaviour) : download all packages, not find
  2. CPM_USE_LOCAL_PACKAGES=1 : if failed to use find_package , download. Just print a warning if failed
  3. CPM_LOCAL_PACKAGES_ONLY=1 : if failed to use find_package, error.

How it is now:

  1. CPM_DOWNLOAD_ALL=1 (default behaviour) : download all packages, not find
  2. CPM_USE_LOCAL_PACKAGES=1 : if failed to use find_package , download. No warning.
  3. CPM_LOCAL_PACKAGES_ONLY=1 : if failed to use find_package, use message(SEND_ERROR ...) to express a warning and fail at the end, not allowing to generate. Also if failed find_package and there's instructions of how to download the package, download it, though still fail at generation phase.

FYI :
from https://cmake.org/cmake/help/latest/command/message.html :

Record the specified message text in the log. If more than one message string is given, they are concatenated into a single message with no separator between the strings.

The optional keyword determines the type of message, which influences the way the message is handled:

FATAL_ERROR

CMake Error, stop processing and generation.

The cmake(1) executable will return a non-zero exit code.

SEND_ERROR

CMake Error, continue processing, but skip generation.

WARNING

CMake Warning, continue processing.

That's why message(SEND_ERROR ...) still makes CMake to fail, but in a way that you just can not generate. That's why even if you download a package with CPM_LOCAL_PACKAGES_ONLY=1 , you still fail. message(FATAL_ERROR ...) seems more appropriate here, because if you failed, why download it if you still will fail?

message(SEND_ERROR ...) not fails immediately, which makes CPM go further and download the package, though not allowing to generate

I think this behavior comes down to personal preference. I prefer CPM to download all dependencies while I make ☕, regardless of whether an error occurs.

In such case, use not CPM_LOCAL_PACKAGES_ONLY=1 , but CPM_USE_LOCAL_PACKAGES=1 . That's how it should be.

CPM.cmake/cmake/CPM.cmake

Lines 719 to 734 in 011d3dd

if(CPM_USE_LOCAL_PACKAGES)
cpm_find_package(${CPM_ARGS_NAME} "${CPM_ARGS_VERSION}" ${CPM_ARGS_FIND_PACKAGE_ARGUMENTS})
if(CPM_PACKAGE_FOUND)
cpm_export_variables(${CPM_ARGS_NAME})
return()
else()
string(REPLACE " " ";" EDITED_CPM_ARGS_FIND_PACKAGE_ARGUMENTS
"${CPM_ARGS_FIND_PACKAGE_ARGUMENTS}"
)
message(
WARNING
"${CPM_INDENT} ${CPM_ARGS_NAME} not found via find_package(${CPM_ARGS_NAME} ${CPM_ARGS_VERSION} ${EDITED_CPM_ARGS_FIND_PACKAGE_ARGUMENTS})"
)
endif()
endif()

If a local package isn't found and CPM falls back to FetchContent, we still print a warning. However, since this fallback is intentional and part of CPM's functionality, the warning might confuse users.

Yep, I agree that the warning is now still not good enough. I will add right now to the warning message something like "The warning emitted bacause CPM_USE_LOCAL_PACKAGES is set to ${CPM_USE_LOCAL_PACKAGES} . Falling back to downloading the package."

@Arniiiii
Copy link
Author

Also thanks for reminding about CPMFindPackage . There's has to be similar fix.

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.

2 participants