Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Support granular selection of tools to build #60

Closed
wants to merge 3 commits into from

Conversation

Qix-
Copy link

@Qix- Qix- commented Sep 18, 2019

Ref #35.

This is one step closer to solving the issue; it allows you to disable/enable tools granularly.

By disabling geometryc and texturev and only building shaderc and texturec, I can now successfully pull in an external meshoptimizer, stb, freetype2 and harfbuzz 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 an if() statement, which would be prone to error if a tool was added later on (adding an option, putting it in tools.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 and texturev play nicely with parent projects, but those aren't covered by this PR.

@JonnyPtn
Copy link
Contributor

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

@JoshuaBrookover
Copy link
Collaborator

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.

@JoshuaBrookover
Copy link
Collaborator

Sorry, did not mean to close.

@JonnyPtn
Copy link
Contributor

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} )
Copy link
Contributor

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

Copy link
Author

@Qix- Qix- Jan 11, 2020

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.

Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

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

Good catch @pezcode.

Copy link
Contributor

@JonnyPtn JonnyPtn left a 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.

@widberg widberg closed this Jun 11, 2020
@Qix-
Copy link
Author

Qix- commented Jun 11, 2020

@widberg why was this closed?

@widberg
Copy link
Owner

widberg commented Jun 11, 2020

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.

@Qix-
Copy link
Author

Qix- commented Jun 11, 2020

but the problem you are trying to solve does not appear to exist

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.

@bwrsandman
Copy link
Contributor

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.
Your quick reply suggests that you are active on github.
I don't mind the issue you're trying to fix and if the PR is cleaned, then I'd very much like to see it merged.

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

@Qix-
Copy link
Author

Qix- commented Jun 12, 2020

It wasn't going to be merged with the changes to .gitmodules

Guys, I have already mentioned I understood this. This isn't what I'm arguing for.

As for your rebuild issue, have you tried setting the target property EXCLUDE_FROM_ALL from your host projet?

No, shaderc is not part of my own project. It's part of this project.

I'm not going to respond to this, this is an innane discussion. I'll fork and maintain my own.

@Qix- Qix- deleted the granular-tooling branch June 12, 2020 15:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants