BuildSystem: Fix warning behaviour regarding WITH_BOOST and WITH_TBB
ClosedPublic
Actions

Authored by Ray Molenkamp (LazyDodo) on Dec 24 2019, 8:31 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)
Brecht Van Lommel (brecht)

Commits

rB6477fcf40f54: BuildSystem/Cleanup: Fix warning behaviour regarding library dependencies

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

tmp_usd_boost_tbb_warns (branched from master)

Build Status

Buildable 6108
Build 6108: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Dec 24 2019, 8:31 PM

Harbormaster completed remote builds in B6108: Diff 20488.Dec 24 2019, 8:31 PM

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
638	A bit confusing to use macro with same name.

This revision is now accepted and ready to land.Dec 27 2019, 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).

In D6479#152161, @Sybren A. Stüvel (sybren) wrote:

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.

Harbormaster completed remote builds in B6117: Diff 20516.Dec 27 2019, 7:42 PM

Ray Molenkamp (LazyDodo) marked an inline comment as done.Dec 27 2019, 7:43 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

Sybren A. Stüvel (sybren) added inline comments.Jan 2 2020, 12:59 PM

build_files/cmake/macros.cmake
1270 ↗	(On Diff #20516)	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