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

A few CMake quality of life suggestions #35

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andesyv
Copy link

@andesyv andesyv commented Nov 15, 2024

Hello! I stumbled upon a few tiny annoyances while working with the toolkit and I thought I should contribute by making a pull request about it, in case my modifications might be useful for someone else. Mainly the modifications adds a few more checks / verifications in various places. For instance by checking if things like if the specified language is among the available ones in the config.json file or if the incorrect path to the devkit was given in the configuration.

But additionally it also separates the GenerateAddOnProject function into two separate "CMake-like" functions add_addon and target_addon_resources to allow for more customizability in the project setup. This allows for syntaxes like:

add_addon (AddOnTarget
  NAME ${AC_ADDON_NAME}
  DEV_KIT_DIR ${AC_API_DEVKIT_DIR}
  AC_VERSION ${AC_VERSION}
)

file (GLOB_RECURSE headers ${CMAKE_CURRENT_SOURCE_DIR}/MyHeaders/*.hpp)
file (GLOB_RECURSE sources ${CMAKE_CURRENT_SOURCE_DIR}/MySources/*.cpp)

target_sources (AddOnTarget PRIVATE
  ${headers}
  ${sources}
)

target_addon_resources (AddOnTarget
  RESOURCE_ROOT_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Resources
  SOURCES_ROOT_DIR ${sources}
  LANGUAGE_CODE ${AC_ADDON_LANGUAGE}
)

Compatibility with current usages of GenerateAddOnProject should be retained so current projects shouldn't be affected (I think).

Copy link
Contributor

@vhorvath-gs vhorvath-gs left a comment

Choose a reason for hiding this comment

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

According to my understanding the tools here and the public add-on template are provided for the simplest use-case where a 3rd party developer wants next to no involvement with the build system and thus doing things "right" is not a priority (as evidenced by the use of GLOBs) as opposed to making things "just work". To this end, I don't see splitting the functions as a benefit at all.

For any real customization, one does not need any of the code provided here nor the add-on template. This is completely fine, since the support code provided is a really small amount, where the important parts can easily be copied.

What I see as a net positive in this PR are the check and verify functions that narrow the contract and provide guidance in case of false assumptions made by the supporting code. I would very much like to see those changes made into a separate PR and I would welcome those changes.

CMakeCommon.cmake Outdated Show resolved Hide resolved
@andesyv
Copy link
Author

andesyv commented Nov 22, 2024

Thanks for the review! I've reverted the changes relating to splitting up this function while keeping some of the safety checks as suggested.

CMakeCommon.cmake Outdated Show resolved Hide resolved
CMakeCommon.cmake Outdated Show resolved Hide resolved
CMakeCommon.cmake Outdated Show resolved Hide resolved
CMakeCommon.cmake Outdated Show resolved Hide resolved
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.

2 participants