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

Replaced CMAKE_SOURCE_DIR, so WickedEngine can be used as submodule #789

Merged
merged 3 commits into from
Dec 26, 2023

Conversation

mlavik1
Copy link
Contributor

@mlavik1 mlavik1 commented Dec 23, 2023

Suggested fix for #788

Test case:

  1. Create a new CMake-based project
  2. Add WickedEngine as a subdirectory
  3. Add add_subdirectory(WickedEngine) to your CMakeLists.txt, and also link to WickedEngine (target_link_libraries(MyProject WickedEngine))
  4. Build your project.

Of course, feel free to reject if you can think of a better fix, or if you don't feel this is a necessary change. Though, I can imagine there are others out there who also wish to use WickedEngine like this :)

@portaloffreedom
Copy link
Collaborator

I like the idea! I need first to test if this breaks the cmake install flow, otherwise it's good to go for me!

CMakeLists.txt Outdated
@@ -1,5 +1,6 @@
cmake_minimum_required(VERSION 3.8)

set(WICKED_ROOT_DIR ${CMAKE_CURRENT_SOURCE_DIR} CACHE INTERNAL "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be a CACHE entry, as we don't want to expose this variable to the user configuring the project

Copy link
Contributor Author

@mlavik1 mlavik1 Dec 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. i originally did that to make it acceasible from parent directories, but realise now that's silly, since the path should be known anyway. I'll fix that today or tomorrow 😃

@mlavik1
Copy link
Contributor Author

mlavik1 commented Dec 24, 2023

I like the idea! I need first to test if this breaks the cmake install flow, otherwise it's good to go for me!

Ok, cool! I'll test that as well 😃 I suppose that shouldn't be affected, but it definitely needs testing😅

@mlavik1
Copy link
Contributor Author

mlavik1 commented Dec 25, 2023

Hi @portaloffreedom, I updated my PR. WICKED_ROOT_DIR is no longer a cached variable.
Feel free to let me know if there's anything else.
Have a great week! :)

@turanszkij turanszkij merged commit 9c66159 into turanszkij:master Dec 26, 2023
3 checks passed
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.

None yet

3 participants