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

Optimize cmake targets #1186

Merged
merged 2 commits into from
Dec 22, 2023
Merged

Optimize cmake targets #1186

merged 2 commits into from
Dec 22, 2023

Conversation

xiaozhuai
Copy link

@xiaozhuai xiaozhuai commented Dec 22, 2023

Allow the following usage

find_package(glm CONFIG REQUIRED)
target_link_libraries(main PRIVATE glm::glm)

# Or use the header-only version
find_package(glm CONFIG REQUIRED)
target_link_libraries(main PRIVATE glm::glm-header-only)

Like most header-only library did. Such as fmt and spdlog.

@christophe-lunarg
Copy link

Nice!
I was hoping to add CMake find_package. Great! :)

@xiaozhuai
Copy link
Author

xiaozhuai commented Dec 22, 2023

Nice!
I was hoping to add CMake find_package. Great! :)

Yeah, I'm so happy you approve of this change.

The following will use shared/static library

find_package(glm CONFIG REQUIRED)
target_link_libraries(main PRIVATE glm::glm)

And the following will use header-only version

find_package(glm CONFIG REQUIRED)
target_link_libraries(main PRIVATE glm::glm-header-only)

@christophe-lunarg
Copy link

find_package(glm CONFIG REQUIRED)
target_link_libraries(main PRIVATE glm::glm)

one thing that I don't understand or that is important with this. Can we choose to link with GLM as a static library xor dynamic library with this change? If yes, how?

It's specifically important because :

  • static library are annoying with compiler options that may be imcompatible with the project that links with this static library.
  • dynamic libraries are annoying because they imply distributing a .so or .dll.

So developers will want to choose across all these posibilities, even if the header only is in most cases the most convenient.

I would also enjoy feedback from other people because all these install stuff, is beyond my understanding.

Thanks!

@xiaozhuai
Copy link
Author

xiaozhuai commented Dec 22, 2023

Can we choose to link with GLM as a static library xor dynamic library with this change? If yes, how?

Yes.
target_link_libraries(main PRIVATE glm::glm) will use static or dynamic library.
It is up to the user to choose to compile a static or dynamic library at compile time.
If user choose to build static glm via cmake -DBUILD_SHARED_LIBS=OFF ..., it will link to static glm library.
If user choose to build dynamic glm via cmake -DBUILD_SHARED_LIBS=ON ..., it will link to dynamic glm library.

target_link_libraries(main PRIVATE glm::glm-header-only) will use header-only library. It is always available.

@xiaozhuai
Copy link
Author

xiaozhuai commented Dec 22, 2023

And I add an option GLM_BUILD_LIBRARY just now.
When set it to ON, both glm::glm and glm::glm-header-only are available.
When set it to OFF, only glm::glm-header-only is available, and glm::glm also works as an alias of glm::glm-header-only.

@xiaozhuai
Copy link
Author

I hope this PR can be merged before 1.0.0 is released.
Because under a lot of package management, such as vcpkg, it will work better.

@christophe-lunarg
Copy link

cmake -DBUILD_STATIC_LIBRARY=TRUE

But in the PR, BUILD_SHARED_LIBS and BUILD_STATIC_LIBS were removed? BUILD_STATIC_LIBRARY and BUILD_SHARED_LIBRARY don't look like a CMake built-in variable.

@xiaozhuai
Copy link
Author

xiaozhuai commented Dec 22, 2023

But in the PR, BUILD_SHARED_LIBS and BUILD_STATIC_LIBS were removed? BUILD_STATIC_LIBRARY and BUILD_SHARED_LIBRARY don't look like a CMake built-in variable.

It's built-in variable.

See https://cmake.org/cmake/help/latest/command/add_library.html

If no type is given explicitly the type is STATIC or SHARED based on whether the current value of the variable BUILD_SHARED_LIBS is ON.


Sorry, I have typo on my previous comments. It should be BUILD_SHARED_LIBS, not BUILD_STATIC_LIBRARY and BUILD_SHARED_LIBRARY

@christophe-lunarg
Copy link

christophe-lunarg commented Dec 22, 2023

oh, ok, all good then! :)

Would you mind to add a section on readme.md to explain how to use the GLM with CMake, find_package and building with BUILD_SHARED_LIBS and BUILD_STATIC_LIBS?

I bet other people don't know about this...

@xiaozhuai
Copy link
Author

Would you mind to add a section on readme.md to explain how to...

Done~
I also add usage for vcpkg users.

@christophe-lunarg christophe-lunarg merged commit 89850e6 into g-truc:master Dec 22, 2023
@christophe-lunarg
Copy link

Thanks for contributing!

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

Successfully merging this pull request may close these issues.

2 participants