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

No need for a FindHWY.cmake #1114

Closed
malaterre opened this issue Jan 17, 2023 · 9 comments
Closed

No need for a FindHWY.cmake #1114

malaterre opened this issue Jan 17, 2023 · 9 comments

Comments

@malaterre
Copy link
Contributor

HWY is build by cmake there should not be a need to write a FindHWY.cmake which are needed only for other build tools (non-cmake type).

@jan-wassenberg
Copy link
Member

Can you expand on how FindHWY would be used by non-CMake build tools?

My understanding is that FindHWY would still be useful for CMake builds that wish to use a prebuilt/system Highway for whatever reason. @kfjahnke any thoughts?

@malaterre
Copy link
Contributor Author

cmake is able to generate those *Config.cmake files that are needed by cmake users. find_package is able to search for HWYConfig.cmake when installed in the correct location.

@jan-wassenberg
Copy link
Member

hm, FindHWY also invokes find_package. It's not clear to me that find_package by itself is sufficient, other projects have actually found it useful to add their own FindHWY. It seems to me they wouldn't do that if plain CMake were sufficient?

@malaterre
Copy link
Contributor Author

It is equivalent...except that generating a HWYConfig.cmake is integrated in cmake directly, no need to manually tweak the FindHWY.cmake anymore (since derived info). Typical steps:

% cat CMakeLists.txt
project(p)
find_package(HWY REQUIRED)

Trigger:

CMake Error at CMakeLists.txt:2 (find_package):
  By not providing "FindHWY.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "HWY", but
  CMake did not find one.

  Could not find a package configuration file provided by "HWY" with any of
  the following names:

    HWYConfig.cmake
    hwy-config.cmake

  Add the installation prefix of "HWY" to CMAKE_PREFIX_PATH or set "HWY_DIR"
  to a directory containing one of the above files.  If "HWY" provides a
  separate development package or SDK, be sure it has been installed.

Since current hwy Debian package does not provide neither FindHWY.cmake nor HWYConfig.cmake...

@malaterre
Copy link
Contributor Author

malaterre commented Jan 17, 2023

For reference:

Not tested but writing HWYConfig.cmake is just a matter of a single cmake line:

write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/HWYConfigVersion.cmake COMPATIBILITY SameMajorVersion)

@malaterre
Copy link
Contributor Author

hm, FindHWY also invokes find_package. It's not clear to me that find_package by itself is sufficient, other projects have actually found it useful to add their own FindHWY. It seems to me they wouldn't do that if plain CMake were sufficient?

chicken and egg situation...until hwy install a HWYConfig.cmake the cmake-outside world needs to retrieve info through a findhwy.cmake (just as any non-cmake based build tool).

@malaterre
Copy link
Contributor Author

Not tested but writing HWYConfig.cmake is just a matter of a single cmake line:

Nevermind this was a bit more work than just a single line.

malaterre added a commit to malaterre/highway that referenced this issue Jan 17, 2023
This will be needed by cmake user consuming HWY.

Also remove FindHWY.cmake since not needed in a cmake-based build tool
setup.

Fixes google#1114
malaterre added a commit to malaterre/highway that referenced this issue Jan 17, 2023
This will be needed by cmake user consuming HWY.

Also remove FindHWY.cmake since not needed in a cmake-based build tool
setup.

Fixes google#1114
malaterre added a commit to malaterre/highway that referenced this issue Jan 17, 2023
This will be needed by cmake user consuming HWY.

Also remove FindHWY.cmake since not needed in a cmake-based build tool
setup.

Fixes google#1114
@kfjahnke
Copy link

Hi!
I'd like to refer you to my last post to #1084
The issue was closed, but I thought my answer would still be seen. I enclosed a patch against your CMakeLists.txt which should make the find file unnecessary.

@jan-wassenberg
Copy link
Member

@malaterre I think I understand the chicken and egg situation. Thanks for explaining and sending the pull request!

@kfjahnke oops, I had indeed missed your post, sorry about that!
Looks like your patch also does something very similar to the pull request here. Let's go with that :)

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

No branches or pull requests

3 participants