CPM.cmake icon indicating copy to clipboard operation
CPM.cmake copied to clipboard

if CPM_LOCAL_PACKAGES_ONLY, make `find_package` fail if it failed to find.

Open Arniiiii opened this issue 1 year ago • 3 comments

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 .

Arniiiii avatar Aug 19 '24 16:08 Arniiiii

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.

https://github.com/cpm-cmake/CPM.cmake/blob/011d3ddd1770e077d8cc8d5c0ebbcb6c00d006e8/cmake/CPM.cmake#L719-L734 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.

Avus-c avatar Aug 19 '24 20:08 Avus-c

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.

https://github.com/cpm-cmake/CPM.cmake/blob/011d3ddd1770e077d8cc8d5c0ebbcb6c00d006e8/cmake/CPM.cmake#L719-L734

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 avatar Aug 20 '24 01:08 Arniiiii

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

Arniiiii avatar Aug 20 '24 01:08 Arniiiii