-
Notifications
You must be signed in to change notification settings - Fork 256
Support granular selection of tools to build #60
Conversation
Just for clarity this is almost completely unrelated to #35. I don't see what this fixes or what the point of it is, but It also won't really break anything, so I don't feel strongly either way |
I agree, it has nothing to do with the original issue in #35. It does relate to the discussion taking place in there. It's a good change, especially since texturev forces you to pull in example's dependencies. |
Sorry, did not mean to close. |
Probably worth adding a warning/error when trying to build texturev without the examples too so it’s clear? |
option( BGFX_BUILD_TOOL_TEXTUREV "Build bgfx tool: texturev" ${BGFX_BUILD_TOOLS} ) | ||
option( BGFX_BUILD_TOOL_TEXTUREC "Build bgfx tool: texturec" ${BGFX_BUILD_TOOLS} ) | ||
option( BGFX_BUILD_TOOL_SHADERC "Build bgfx tool: shaderc" ${BGFX_BUILD_TOOLS} ) | ||
option( BGFX_BUILD_TOOL_GEOMETRYC "Build bgfx tool: geometryc" ${BGFX_BUILD_TOOLS} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the idiomatic thing to do here is using cmake_dependent_option
:
cmake_dependent_option(BGFX_BUILD_TOOL_TEXTUREV "Build bgfx tool: texturev" ON
"BGFX_BUILD_TOOLS" OFF)
cmake_dependent_option(BGFX_BUILD_TOOL_TEXTUREC "Build bgfx tool: texturec" ON
"BGFX_BUILD_TOOLS" OFF)
cmake_dependent_option(BGFX_BUILD_TOOL_SHADERC "Build bgfx tool: shaderc" ON
"BGFX_BUILD_TOOLS" OFF)
cmake_dependent_option(BGFX_BUILD_TOOL_GEOMETRYC "Build bgfx tool: geometryc" ON
"BGFX_BUILD_TOOLS" OFF)
https://cmake.org/cmake/help/v3.15/module/CMakeDependentOption.html#module:CMakeDependentOption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to bump the minimum CMake version in order to use that. If @JoshuaBrookover is okay with that, then this is indeed the correct way to do this.
Disregard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supported by CMake 3.0 as well, no reason to bump version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @pezcode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to remove the submodule update from this PR
if( BGFX_BUILD_TOOLS ) | ||
include( cmake/tools.cmake ) | ||
if( BGFX_CUSTOM_TARGETS ) | ||
add_custom_target( tools ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@widberg why was this closed? |
It is unrelated to the issue you referenced. You seem to be the only person that needs this. You changed the submodule remotes. If you want I’ll leave it open for others to comment, but the problem you are trying to solve does not appear to exist. I have no intention of merging this. |
Is nobody else using this in their project? Shaderc recompiles almost every time we do an incremental rebuild. Relinking it takes almost 20 seconds. There's something very wrong with the cmake config as-is. |
It wasn't going to be merged with the changes to .gitmodules pointing to a different branch than upstream. Not to mention suggestions to fix the PR being ignored for months. As for your rebuild issue, have you tried setting the target property EXCLUDE_FROM_ALL from your host projet? https://cmake.org/cmake/help/latest/prop_tgt/EXCLUDE_FROM_ALL.html#prop_tgt:EXCLUDE_FROM_ALL |
Guys, I have already mentioned I understood this. This isn't what I'm arguing for.
No, I'm not going to respond to this, this is an innane discussion. I'll fork and maintain my own. |
Ref #35.
This is one step closer to solving the issue; it allows you to disable/enable tools granularly.
By disabling
geometryc
andtexturev
and only buildingshaderc
andtexturec
, I can now successfully pull in an externalmeshoptimizer
,stb
,freetype2
andharfbuzz
without any issues.The reason for moving
cmake/tools.cmake
's contents into the root was that if I didn't, I'd have to check each of the tool's options in anif()
statement, which would be prone to error if a tool was added later on (adding an option, putting it intools.cmake
but not including it in the if statement). It felt more correct.The edit at the bottom of #35 (comment) explains what needs to be done to have
geometryc
andtexturev
play nicely with parent projects, but those aren't covered by this PR.