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

[cmake] Remove need for CMAKE_FIND_PACKAGE_PREFER_CONFIG #2009

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

Conversation

DownerCase
Copy link
Contributor

Description

CMAKE_FIND_PACKAGE_PREFER_CONFIG was recommended to have CMake (on Windows) find the vendored version of protobuf before falling back to the system findProtobuf.cmake module.

However, you can just manually implement this and try and find protobuf first through CONFIG then fallback to a required MODULE if not found.

This is one less thing to get wrong, I myself have forgotten this sharp corner, more than once, and wondered why everything fell over.

Tested on Windows with a local install of the installer and it works great 😄

Related issues

  • Fixes unintuitive and atypical usage causing issues.
  • Adds some missing newlines at the end of files (not a deliberate change, git was just upset they were missing and fixed them)

Cherry-pick to

@KerstinKeller KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Feb 11, 2025
@KerstinKeller
Copy link
Contributor

The issue wasn't so much that we did want to find the vendored version. The issue was rather that FindProtobuf.cmake is buggy with regard to finding static Protobuf builds on Windows (and we never actually bothered to make a PR for that with CMake).

I neved like to have CMAKE_FIND_PACKAGE_PREFER_CONFIG in the samples, but the workaround you proposed is no less hacky 😄
I would propose to remove both the workaround and setting the cmake variable, and then note in the documentation (or elsewhere), that on Windows builds with the installer, the user should pass the variable on the command line from outside.

(For that matter neither am I a fan to provide an installer with an SDK, but totally different matter 😏 )

@DownerCase
Copy link
Contributor Author

DownerCase commented Feb 11, 2025

The issue wasn't so much that we did want to find the vendored version. The issue was rather that FindProtobuf.cmake is buggy with regard to finding static Protobuf builds on Windows (and we never actually bothered to make a PR for that with CMake).

I neved like to have CMAKE_FIND_PACKAGE_PREFER_CONFIG in the samples, but the workaround you proposed is no less hacky 😄 I would propose to remove both the workaround and setting the cmake variable, and then note in the documentation (or elsewhere), that on Windows builds with the installer, the user should pass the variable on the command line from outside.

(For that matter neither am I a fan to provide an installer with an SDK, but totally different matter 😏 )

I'd say the advantage of my change is that it maintains existing promoted behaviour but you can't do it wrong.

The prefer config setting is seldom used elsewhere and could (not saying it ever has) interfere with resolving other packages if set globally by the user.

Another option could be to configure the search paths of find_package to explicitly look for the vendored protobuf library on Windows first, though it looks like that was previously tried through CMAKE_MODULE_PATH. Something along the lines of find_package(protobuf NO_DEFAULT_PATH ${PACKAGE_PREFIX_DIR}/cmake/")

If you still want to stick with CMAKE_FIND_PACKAGE_PREFER_CONFIG being done by the user, I'll at least capture the find_package failure and issue an error saying to try it.

@DownerCase DownerCase force-pushed the prefer-protobuf-config branch from a738dd6 to 06a287a Compare February 13, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants