bgfx.cmake
bgfx.cmake copied to clipboard
widberg
examples excluded from the 'all' target by default
Why are you doing this?
if( BGFX_INSTALL_EXAMPLES )
add_executable( example-${ARG_NAME} WIN32 ${SOURCES} )
else()
add_executable( example-${ARG_NAME} WIN32 EXCLUDE_FROM_ALL ${SOURCES} )
endif()
It's not a bid deal, I can always do -DBGFX_INSTALL_EXAMPLES=ON when I generate, but I'm just trying to understand what you were trying to do. I personally don't care about the install target, but I want to test some changes I do to bgfx on some of those examples to make sure I didn't break anything...
If you include this project in a larger one (so that you build bgfx from source each time) and you want to keep the examples around for reference (so they're still easily built by make or in visual studio), but you don't want them to build in your CI every time, then they need to be excluded from all. The other option is to have your CI build a specific project instead of "all" but that allowed us to easily add things to CI by just making it included in "all" build.
I no longer work at said company so I'm open to change but I suspect most people don't want to build examples every time, but may want to keep them in the project, so exclude from all seemed like a nice compromise. You can also build all examples by building the "examples" target.
I completely understand your point of view here. My concern is more about BGFX_BUILD_EXAMPLES vs BGFX_INSTALL_EXAMPLES. I would simply get rid of the later option. It makes no sense to have an install option drive what gets built by the all target. In Linux/Unix/OSX/FreeBSD, the install target installs (by default) in /usr/local/bin, installs the headers in /usr/local/include and installs the libraries in /usr/local/lib. Personally, I don't think that the examples should ever be installed system-wide.
And also, it creates problems for IDE other than visual studio. I'm on Linux using QtCreator and it complains that it can't find examples-06-bump executable when I try to launch it. QtCreator seems too dumb to realise that examples-06-bump is not part of the all target. :man_shrugging:
but again, it's not a big deal. turning on BGFX_INSTALL_EXAMPLES solves everything for me. I am very pleased with everything that you've done. Your project is very thorough and even takes care of recompiling all of the in-repo shaders for vulkan, metal and glsl. Thank you for sharing your build script with the community :-)
For what it's worth, I've tripped up a few times on this. I'd expect BGFX_BUILD_EXAMPLES to make them part of the build, but instead I also have to set BGFX_INSTALL_EXAMPLES. It's not a big deal to workaround, it's just not immediately clear
We could have BGFX_INSTALL_EXAMPLES on by default? It makes sense to minimize confusion for a new user while still providing the flexibility. I suspect any serious project will disable examples anyway.
It gets tricky to balance between the many use cases of CMake but I want to make it easiest for newcomers to get involved. Many people don't have the patience to shift through code when something doesn't work. 🙂
Perhaps it’s worth just trying to mimic what the main bgfx repo does, which I beleive has the examples included in the “all” target? Of course keeping a the flag so people can turn them off when needed
Yeah, that's what turning BGFX_INSTALL_EXAMPLES on by default would do. It's a conflated flag because the concept is conflated within CMake itself. By including the examples in the "all" build it will be installed.
I have no problem with this change.
This is causing me a ton of headache right now trying to use this in a parent CMake project.
I'm also using my own version of freetype2 and imgui, and would also like to use my own version of meshoptimizer (though that's less important).
Right now, the examples directory is inexplicably always included and it's causing an unrelated copy of freetype to include the bgfx version. Both CMake and Clang are making it nearly impossible to figure out where the CMake directive to add the 3rdparty directory in the examples to the include path, but it needs to not be there. The CMake configuration files are very complicated and I'm wasting hours trying to debug this.
tl;dr this repository is polluting the global include path for unrelated builds.
Where should I look to make sure things like dear-imgui and freetype from the third-party folder don't appear on the include path?
I tried wrapping the aforementioned include() call in an if (BGFX_BUILD_EXAMPLES) line but...
-
-I../ext/bgfx/bgfx/3rdpartyis still inexplicably on the command line - texturev gets
'common.h' file not found- perhaps this is a question for @bkaradzic but why does a non-example tool require a file from the examples directory -- assuming it's referring toexamples/common/common.h?
Current config:
option (BGFX_BUILD_TOOLS "" ON)
option (BGFX_BUILD_EXAMPLES "" OFF)
option (BGFX_INSTALL "" OFF)
option (BGFX_INSTALL_EXAMPLES "" OFF)
option (BGFX_USE_OVR "" OFF)
if (CMAKE_BUILD_TYPE EQUAL "Debug")
option (BGFX_CONFIG_DEBUG "" ON)
endif ()
add_subdirectory (bgfx)
cc @widberg
@Qix-
Your last question is the cause for the global include of examples. My guess is you (and most others) don't need texturev, so it is an unfortunate arrangement right now. Some more options could fix it - perhaps one for each tool?
Your observation that the project pollutes the global include path doesn't seem right to me. I tried to make it so each target used the appropriate target-level functions (target_include_directories, set_target_properties, target_link_libraries, etc) to ensure that did not happen. It is a different story if you link against bgfx, in which case your target inherits settings. Even then, it shouldn't include the bgfx 3rdparty directory since they are marked private. This seems to be the case in my own quick test:
cmake_minimum_required( VERSION 3.0 )
project(test)
add_subdirectory(.. "bgfx")
add_executable(test main.cpp) # int main(){}
target_link_libraries(test bgfx)
-I.../bgfx.cmake/bgfx/include
-I.../bgfx.cmake/.../bgfx.cmake/bx/include # bad include
-I.../bgfx.cmake/bx/include
-I.../bgfx.cmake/bx/3rdparty
-I.../bgfx.cmake/bx/include/compat/osx
-I.../bgfx.cmake/bimg/include
-I.../bgfx.cmake/.../bgfx.cmake/bimg/3rdparty # bad include
-I.../bgfx.cmake/bimg/3rdparty/astc-codec
-I.../bgfx.cmake/bimg/3rdparty/astc-codec/include
-I.../bgfx.cmake/bimg/3rdparty
-I.../bgfx.cmake/bimg/3rdparty/iqa/include
-I.../bgfx.cmake/bimg/3rdparty/nvtt
The project certainly pollutes the global "target" space, but that's because there is only a global "target" space. That is, there can only be one target named dear-imgui. I think most dependency systems (including CMake) struggle with multiple versions of the same dependency. CMake especially starts to fall apart when you want to change a dependency's dependency, so your hopes of replacing meshoptimizer is likely to cause you hassle as well.
My suggestion is to modify bgfx.cmake to remove the inclusion of both examples and texturev entirely and see if that at least gets you moving forward. I would be interested to see if there are more problems after that.
From what I can see, only the dear-imgui and meshoptimizer targets publicly target_include_directories the ${BGFX_DIR}/3rdparty directory. This include should be isolated to the examples and geometryc since those are the only targets in this repository that target_link_libraries either dear-imgui or meshoptimizer. I was unable to reproduce this issue with the Visual Studio generator using the config you posted. My guess is that CMake is using the dear-imgui target specified in this repository instead of the version you want it to use and including the ${BGFX_DIR}/3rdparty directory as a result of that. If that is the case I would recommend changing the name of your imgui target to something other than dear-imgui.
@Qix- Because some common functionality between examples, and graphics tools are shared...
@bkaradzic It breaks modularity, though. Having a tool used by end consumers of BGFX that relies upon example code means you have to compile the example code, which means hours of surgically piecing out all of the example code to figure out what belongs in your application and what doesn't. This is what just spent several hours doing, and here we are now.
What makes more sense is to move the relevant commons code out of the examples folder so that instead of the tool relying upon the examples code, instead the tool and the examples rely on the commons code.
I get the feeling you don't want to place much emphasis on the examples code since every application uses it slightly differently -- which I totally understand and agree with -- but it causes a lot of headache around the build systems portion of it - especially when one doesn't want to vendor it in and instead have a base BGFX submodule untouched, making it easier to do upstream upgrades.
At the very least, could you do a complete relative path to the commons code instead of relying on finicky include paths?
@widberg is correct - not sure how I missed this as this is exactly what I was searching for. "Global" wasn't the right terminology, apologies. 😅
Could those be changed to PRIVATE? Would you accept a PR @widberg / @JoshuaBrookover ?
It doesn't break anything, it's just inside library named libexample-common.a. This might be different with CMake, I'm talking how it's inside bgfx repo. I don't use CMake.
The only libraries you need to use bgfx are:
libbgfxDebug/Release.a
libbimgDebug/Release.a
libbxDebug/Release.a
Everything else is for tools or examples.
We're not talking about libraries here, we're talking about include paths. Perhaps this is simply how CMake is configured in this repository. I still have issues with how mainline BGFX structures its file hierarchy, personally, but that's workable with CMake.
FWIW I agree with your opinions about cmake. It's terrible, but it's the best that exists right now IMO. Not that my opinion matters 🙃
The includes on the dear-imgui and meshoptimizer targets need to be public for the examples and geometryc executables to call the functions in those libraries. I recommend adding your version of imgui as a new target called imgui and target_link_libraries that instad of the dear-imgui target that is specified in this repository. For an example of how to do this take a look at the deps directory in the bigg repository. Using your own version of meshoptimizer and having the other targets in this repository use that instead of the meshoptimizer target specified here will most likely require you to create a fork and replace the meshoptimizer target specification with one that uses the sources in your version.
@bkaradzic I think the issue with texturev is a misunderstanding of how CMake works.
A bit of explanation, just in case you're curious... This project tries to mimic the bgfx build system. The main difference between the two is that people can include the entire build system into their project (meaning bgfx, its tools, and its examples - if enabled - build in someone else's CMake project). There is no output they copy over (like those .a files), CMake rigs everything up for them, at the cost of having to build everything from source (which is a benefit if you want to debug bgfx - the exact reason I made this project).
There are quirks that exist due to this coupling. You don't run into these same quirks with the stock bgfx build system because that build system doesn't allow you to do this.. as far as I know. And, I would guess, for good reason!
Most people adopt these scripts into their own repo, hack them to bits, and never look back at mainline. It's probably the only sane way to do it.
Most people adopt these scripts into their own repo, hack them to bits, and never look back at mainline. It's probably the only sane way to do it.
This is what I'm trying to avoid. I am add_subdirectory-ing this project into mine, as I do most of my other dependencies (sans imgui).
The includes on the dear-imgui and meshoptimizer targets need to be public for the examples and geometryc executables to call the functions in those libraries
Why not just invert the target_include_directories call to have the examples use the directories instead of putting them as PUBLIC on the production dependencies? Examples and tests should come second after the primary library considerations.
Why not just invert the target_include_directories call to have the examples use the directories instead of putting them as PUBLIC on the production dependencies? Examples and tests should come second after the primary library considerations.
What you are saying is already true. We do not put them on production dependencies except for geometryc where the behavior is intentional. geometryc is a binary, not a library. That is why we have both stated that we cannot reproduce your problem and suggested fixes for you to try.
That PUBLIC doesn't mean every target receives the includes, it means every parent target receives the includes. In this case, that should only be geometryc and the examples. That is the behavior I am seeing.
A target becomes a parent target when using target_link_libraries, as is the case here and here. In order for you to receive those includes, you must be using target_link_libraries on either an example, geometryc, dear-imgui, or meshoptimizer. This should not be the case if you are merely linking against bx, bgfx, or bimg. To use bgfx, one needs only to link against bgfx as I did in my test above.
So, to be rather direct, I believe there is something wrong on your end. Unless you can provide a use case we haven't anticipated, I believe everything is working as intended. If you look at my bigg repository you can see a working example of dropping in my own imgui, which if I understand correctly, is what you are trying to do.
This isn't an issue on my end.
Could we add in the conditional for examples again and add the ability to disable (or better yet, fix) texturev? Because I can't include my own meshoptimizer because the targets conflict because the examples can't be disabled, so I have to pull in the third-party one which exposes the public includes which causes include paths to get polluted when compiling freetype.
There's no way to solve this without vendoring in and modifying things.
The targets conflict because you are trying to create 2 targets with the name meshoptimizer not because the examples are enabled. You can add your own version of meshoptimizer and link your project against that instead of the 3rdparty one by giving it a different name. Like @JoshuaBrookover said, if you want to see how this was done with imgui you can look at the bigg repository. The process for doing this with meshoptimizer is identical if you only want your project to use the other version.
This isn't an issue on my end.
You are suggesting fixes we disagree with, to problems we cannot reproduce. If you want to fix something, you first need to prove its broken. We have made every attempt to reproduce it ourselves.
Could we add in the conditional for examples again and add the ability to disable (or better yet, fix) texturev?
I already suggested you to try this:
My suggestion is to modify bgfx.cmake to remove the inclusion of both examples and texturev entirely and see if that at least gets you moving forward. I would be interested to see if there are more problems after that.
Yes, I want to add a conditional to the examples, and I want to be able to disable texturev. I asked you to try these changes yourself to verify it fixes your issue before we make the changes. This is the proper way to verify the solution is good.
(or better yet, fix) texturev
What is the problem with texturev, exactly? I believe your actual problem is with CMake adding in target names you yourself wish to use (meshoptimizer and dear-imgui). That is not a problem with texturev.
Here's the problems as I see it:
- if you build tools, you are forced to build all tools
- bgfx.cmake force includes example dependencies always
- texturev requires examples "common"
The first two can be fixed. The last one cannot be fixed without changing bgfx, which is outside of the scope of this repository.
These are non issues:
- meshoptimizer and dear-imgui targets exist if you build examples
- those same targets have PUBLIC includes
My comments regarding an issue existing on your end pertains only to these last two issues. You wanted to change the way examples build because you thought something in there was causing issues. My only point is, you should not be running into these issues in the first place, and if you are, something has gone wrong.
I think you misunderstood my issues, as how you've described the problems as a whole is exactly how I see it 🙃. I think we're debating the same end.
You can absolutely fix the last one if you wanted to. It's just a matter of adding a private include directory to texture. It's not pretty but it would get the job done.
I think you misunderstood my issues how you've described the problems as a whole is exactly how I see it
If I have described the problem exactly, where is my misunderstanding? I can't help you if I don't understand the problem.
I think we're debating the same end.
I am asking you to try my suggested fix. If that works, we will implement that solution.
You can absolutely fix the last one if you wanted to.
I agree it could be changed (its not broken in my opinion), but you've provided no good reason for doing so.
If I have described the problem exactly, where is my misunderstanding? I can't help you if I don't understand the problem.
Nowhere, it looked like you thought I was arguing for something else. I was agreeing with you. :)
I agree it could be changed (its not broken in my opinion), but you've provided no good reason for doing so.
Because it's philosophically incorrect. A production tool relying on code from demos or examples is not correct. It breaks when users assume examples/demos can be excluded from the build or removed from the filesystem entirely.
I'm looking into fixes now but I'm not well versed with this CMake configuration. It's quite heavily coupled and it's making it tricky to figure out where everything is.
One thing is for certain: wrapping the inclusion of the examples directory with an if( BGFX_BUILD_EXAMPLES ) still causes an external freetype to conflict. I'm looking into it further now.
EDIT: Narrowed it down to geometryc and meshoptimizer -- meshoptimizer will not include itself if there's already another meshoptimizer target (good) but that means the public include directories for 3rdparty are no longer publicly available how geometryc needs them (bad).
target_include_directories( meshoptimizer PUBLIC ${BGFX_DIR}/3rdparty )
This is wrong. Meshoptimizer's include directories are ${BGFX_DIR}/3rdparty/meshoptimizer/src. This makes #include <meshoptimizer.h> work as expected. Upon further look, all of the 3rdparty dependencies are used like this.
The root problem is with BGFX since the include structure is not modular, as I mentioned before.
BGFX needs to change #include <thing_from_3rdparties/includes_directory/header.h> to just #include <header.h>, utilizing the build system to apply each of the third-party header directories individually. That means that if a third party is switched out by a consuming project, the include directories match the expected layout (and further, don't conflict with any other targets).
I'd be happy to submit a patch to BGFX @bkaradzic, and then a subsequent one here @JoshuaBrookover.
Because it's philosophically incorrect. A production tool relying on code from demos or examples is not correct.
I agree with you, that is why I listed it as an issue. But this has nothing to do with the change you suggested:
It's just a matter of adding a private include directory to texture.
I believe this to be a red herring. It should not affect you. Maybe I am misunderstanding, but this is the problem we tried to reproduce. None of your targets should be affected by a PUBLIC include on the meshoptimizer target, so far as I can tell.
I'm fine with the change you suggested, I am just puzzled as to why it matters.
The root problem is with BGFX
I agree with you. You may need to create an issue over in the bgfx repository.
Support granular selection of tools to build #60
Thanks for the PR! There are a few concerns I have with it, but I will leave comments over there when I have time to thoroughly investigate. It looks good for the most part.
I'd be happy to submit a patch to BGFX
Don't submit PR. :)
This whole thing is nonsense, you're mapping some world view you have to other projects. Build system defines what's required, not directory structure.
The reason why this is preferred:
#include <meshoptimizer/src/meshoptimizer.h>
is because there tons of stuff inside meshoptmizer/src directory that has to be added to include path so you can write #include <meshoptimizer.h> there might be bunch of other junk that's undesirable there and suddenly it's in your include search path.
@bkaradzic Designing deployment and CI/CD pipelines as well as working on multiple build systems as a career isn't what I'd consider just "some world view". The way the includes in BGFX are set up for third party libraries makes it very difficult to interface with BGFX on a more advanced and serious level. Not impossible. Just annoying and difficult.
There's only one meshoptimizer.h in that directory so I don't really understand your argument. If you're still very concerned about it, then open a PR with meshoptimizer and move their header into an include directory, like most libraries do (for this exact reason).
Otherwise, your argument has no merit. When you boil it away, you're including things like this because it's easier to configure, not because it's correct.
You can choose to do it this way for laziness reasons or you can have someone submit a PR to spruce it up. Your choice. 🙃
There's only one meshoptimizer.h in that directory so I don't really understand your argument.
It's general rule, there are other projects that don't cleanly split includes and sources.
Designing deployment and CI/CD pipelines as well as working on multiple build systems as a career isn't what I'd consider just "some world view".
bgfx as library was used in some very complex / multiplatform projects (like AAA game production), none of that you talking about was issue.
$ mkdir /tmp/test-bgfx-cmake && cd /tmp/test-bgfx-cmake
$ git init
$ git submodule add https://github.com/JoshuaBrookover/bgfx.cmake.git bgfx
$ git submodule add https://git.savannah.gnu.org/git/freetype/freetype2.git
$ git submodule add https://github.com/zeux/meshoptimizer.git
$ git submodule add https://github.com/harfbuzz/harfbuzz.git
$ git submodule add https://github.com/ocornut/imgui.git
$ git submodule update --init --recursive
project(test)
cmake_minimum_required(VERSION 3.11)
option (FT_WITH_HARFBUZZ "" ON)
option (HB_HAVE_FREETYPE "" ON)
option (HB_BUILD_SUBSET "" OFF)
set (HARFBUZZ_INCLUDE_DIRS harfbuzz/src)
set (HARFBUZZ_LIBRARIES harfbuzz)
add_subdirectory(harfbuzz)
add_subdirectory(freetype2)
add_subdirectory(meshoptimizer)
add_subdirectory(bgfx)
add_executable(my-program test.cc imgui/imgui.cpp imgui/imgui_demo.cpp imgui/imgui_draw.cpp imgui/imgui_widgets.cpp)
target_link_libraries(my-program bgfx freetype2 harfbuzz meshoptimizer)
// test.cc
int main() { return 0; }
The dependency on the tools is normally implicitly defined via using them, so for the sake of example, we'll explicitly add the dependency:
add_dependencies(my-program shaderc texturec texturev geometryc)
This gives me:
CMake Error in bgfx/CMakeLists.txt:
Target "astc-codec" INTERFACE_INCLUDE_DIRECTORIES property contains path:
"/tmp/bgfx-fail/bgfx/"
which is prefixed in the source directory.
three times total.
So I add my usual build options:
option (BGFX_BUILD_EXAMPLES "" OFF)
option (BGFX_INSTALL "" OFF)
option (BGFX_INSTALL_EXAMPLES "" OFF)
option (BGFX_USE_OVR "" OFF)
Configures fine, but compilation gives me this:
../bgfx/bgfx/tools/geometryc/geometryc.cpp:17:10: fatal error: 'meshoptimizer/src/meshoptimizer.h' file not found
#include <meshoptimizer/src/meshoptimizer.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I cannot use an external meshoptimizer in my own project and use the tools to build things without either
- Vendoring in BGFX proper and fixing the include directory, or
- Compiling and installing the tools separately, which breaks rule 2 of the Joel test for example. It's also super annoying when trying to make sure everyone on a project has the same environment. It also makes it very hard to upgrade such tools, as you have to wrangle everyone up to make them upgrade.
And no, switching the add_subdirectory() calls around just causes the second meshoptimizer to fail as a double-target. And even if that project added a if (TARGET meshtarget) return() endif() it would either hide the super-projects (at which point, why bother?) or run right back into this weird inclusion scheme.
Saying AAA game studios have never run into this problem is an anecdotal appeal to authority. They most likely fork and maintain their own diffs themselves, because they can. Or a number of other configurations of internal tooling and builds/deployments. It doesn't make your inclusion scheme correct.
Like I said, if you don't want to fix it, then fine. It's strange that for an OSS project you won't accept non-breaking improvements, even if you don't think they're high priority.
But there is an improvement to be made here whether or not you believe in it.
Do you need geometryc?!
If you need it use provided version of meshoptimizer. If you don't need it exclude it from build.
I don't know how CMake works. With GENie this issue doesn't exist, when mixed with larger projects.
If you need it use provided version of meshoptimizer. If you don't need it exclude it from build.
Hence #60. It doesn't negate the fact the include patterns are whack. 🙃
But there is another issue, this implies in CMake include paths are somehow global, not per project.
No the includes are per target, not global
Unless I'm mistaken, the issue here is just the same one originally reported in this issue before it was hijacked with ridiculous specific use case? i.e. just a bit of a rethink about what BGFX_BUILD_EXAMPLES and BGFX_INSTALL_EXAMPLES enable?
One thing that must be clarified here: using add_subdirectory to pull in dependencies is, as far as I can tell, an antipattern. Yes, it's exactly what I recommend people do when using this project. It's exactly what I wish the CMake ecosystem could be. However, I don't believe it's a recommended solution, because most CMake projects don't consider it, nor is it a "feature" of CMake.
The proper pattern (I think) is to use find_library to fetch prebuilts already on your system. I know that's not the one-step-build you want, but you're going to be cleaning up years of "bad practice" in a variety of projects to make add_subdirectory viable... or at least, cleanly viable.
Even if it were an intended use case, I don't believe replacing a dependency's dependency is ever a clean process. Correct me if I'm wrong, but even over in NPM land, you would have to maintain your own fork to do that.
This error is a completely separate issue and should be fixed with #62
CMake Error in bgfx/CMakeLists.txt: Target "astc-codec" INTERFACE_INCLUDE_DIRECTORIES property contains path: "/tmp/bgfx-fail/bgfx/" which is prefixed in the source directory.three times total.
