Page MenuHome

BuildSystem: Fix warning behaviour regarding WITH_BOOST and WITH_TBB
AcceptedPublic

Authored by LazyDodo (LazyDodo) on Tue, Dec 24, 8:31 PM.

Details

Summary

Adding USD to a lite build

  • set 'WITH_USD' to 'On'
  • rebuild, build errors regarding boost
  • set 'WITH_BOOST' to 'On'
  • rebuild, still build errors regarding boost
  • double check, 'WITH_BOOST' is really on.
  • re-run cmake manually
  • rebuild, hoping for a different result, but why would it... still build errors regarding boost
  • Look in CMakeLists.txt finally find the bit that silently disables boost
  • Curse the one who thought that was a good idea just a tiny bit.
  • Fix the issue
  • Rebuild, TBB errors now
  • Turn WITH_TBB on
  • Glorious success!

This patch fixes

  • The Silent disabling of boost
  • Add a check that USD is is not on before doing that
  • Add similar checks for TBB this already existed for windows, but should have been done for all platforms.
  • move the TBB checks to a central location rather than the individual platform files

Diff Detail

Repository
rB Blender
Branch
arcpatch-D6479 (branched from master)
Build Status
Buildable 6117
Build 6117: arc lint + arc unit

Event Timeline

As with current behavior of Blender's CMake think it's fine.

But on a more global scale would expect this behave differently: only have WITH_FOO for actual Blender's features (such as OpenImageDenoiser, USD, ect) and all the libraries like Boost and TBB enabled when any of those features are requested by a developer/user.
If a feature is requested, there is no other way than to requite it's dependencies. If someone doesn't want a particular dependency just disable features which are using it.

CMakeLists.txt
627

A bit confusing to use macro with same name.

This revision is now accepted and ready to land.Fri, Dec 27, 9:45 AM

What I don't understand is why this is Windows-specific behaviour in the first place. In platform_unix.cmake and platform_apple.cmake there is no such "nobody is using Boost so let's turn it off" code.

I agree with @Sergey Sharybin (sergey)'s bigger picture (removing the WITH_xxx for dependencies, and just enabling them when they are needed). That should IMO be done in a place that's not platform-dependent, though (unless feature X only requires library Y on a single platform).

What I don't understand is why this is Windows-specific behaviour in the first place. In platform_unix.cmake and platform_apple.cmake there is no such "nobody is using Boost so let's turn it off" code.

It is not, turning off boost was done globally for all platforms in the main CMakelists.txt , TBB was what I assumed was windows only, however i was wrong and this code was duplicated in both the unix and apple platform files as well, i should clean that up before landing this patch.

I agree with @Sergey Sharybin (sergey)'s bigger picture (removing the WITH_xxx for dependencies, and just enabling them when they are needed). That should IMO be done in a place that's not platform-dependent, though (unless feature X only requires library Y on a single platform).

If we transition to modern cmake targets rather than sprinkling DEP_INCLUDE and DEP_LIBRARIES all over the place we can also get rid of the madness that is our sorted libs in one swift move, this is however a story for a different diff.

  • De-duplicate macro code
  • move macro to macros.cmake
  • cleanup TBB from apple and unix platform files.
LazyDodo (LazyDodo) marked an inline comment as done.Fri, Dec 27, 7:43 PM
LazyDodo (LazyDodo) edited the summary of this revision. (Show Details)Fri, Dec 27, 7:47 PM

adding @Brecht Van Lommel (brecht) as reviewer since he added the checks in the platform files, not sure if this was an oversight or if there was a good reason to control this per platform

build_files/cmake/macros.cmake
1270

This really needs a comment about what _dep and _setting are. Probably better to avoid abbreviations and name it _dependency instead?

I'm thinking about a comment like this:

# When $_dep is disabled, forces $_setting = $_val