examples excluded from the 'all' target by default #35
Comments
|
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 |
|
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 |
|
but again, it's not a big deal. turning on |
|
For what it's worth, I've tripped up a few times on this. I'd expect |
|
We could have 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 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 tl;dr this repository is polluting the global include path for unrelated builds. Where should I look to make sure things like I tried wrapping the aforementioned
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 |
|
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: 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 |
|
@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 |
|
It doesn't break anything, it's just inside library named |
|
The only libraries you need to use bgfx are: 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 |
|
@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. |
This is what I'm trying to avoid. I am
Why not just invert the |
What you are saying is already true. We do not put them on production dependencies except for That A target becomes a parent target when using 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 |
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.
I already suggested you to try this:
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.
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:
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:
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. |
If I have described the problem exactly, where is my misunderstanding? I can't help you if I don't understand the problem.
I am asking you to try my suggested fix. If that works, we will implement that solution.
I agree it could be changed (its not broken in my opinion), but you've provided no good reason for doing so. |
Nowhere, it looked like you thought I was arguing for something else. I was agreeing with you. :)
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 EDIT: Narrowed it down to target_include_directories( meshoptimizer PUBLIC ${BGFX_DIR}/3rdparty )This is wrong. Meshoptimizer's include directories are The root problem is with BGFX since the include structure is not modular, as I mentioned before. BGFX needs to change I'd be happy to submit a patch to BGFX @bkaradzic, and then a subsequent one here @JoshuaBrookover. |
I agree with you, that is why I listed it as an issue. But this has nothing to do with the change you suggested:
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 I'm fine with the change you suggested, I am just puzzled as to why it matters.
I agree with you. You may need to create an issue over in the bgfx repository.
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. |
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:
is because there tons of stuff inside |
|
@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 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. 🙃 |
It's general rule, there are other projects that don't cleanly split includes and sources. |
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 --recursiveproject(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: 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: I cannot use an external meshoptimizer in my own project and use the tools to build things without either
And no, switching the 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. |
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 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 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
|
Why are you doing this?
It's not a bid deal, I can always do
-DBGFX_INSTALL_EXAMPLES=ONwhen I generate, but I'm just trying to understand what you were trying to do. I personally don't care about theinstalltarget, but I want to test some changes I do to bgfx on some of those examples to make sure I didn't break anything...The text was updated successfully, but these errors were encountered: