CMake cleanup

All,

It’s a long one, target audience: anyone who ever edited a blender CMakeLists.txt, no tl;dr, as this IS the tl;dr

I was planning to send an update about this once the work was completed, but several community members have reached out with various levels of confusion and/or venom, so I’ll just go ahead and send an update now.

as you have possibly noticed, I have been making some changes to our build system, some new and foreign concepts to some may have shown up. And things may work just a tiny bit different than before.


Why?

Our build system is in a pretty bad state, to be frank, it’s a bit off a mess and it’s a miracle we are not having problems more often.

Headers are used without linking the required libs causing fun link ordering issues, for instance compare the INC section of bf_draw to the LIB section. It relies on many many more libraries than it declares. (I’m not picking on bf_draw here, it is the rule, not the exception)

Until last week the default scope of libraries in the LIB section was set to INTERFACE (explanation on why interface is a poor choice later on in this post) causing things to build out of order, which we then fixed by adding dependencies back in by adding add_dependencies between the libraries so we wouldn’t have to deal with missing headers.

And then there’s the ever barrage of ../’s in INC sections seriously does anyone know where these files actually live when looking at a path like this?

there’s additional nuisances like the never ending and often duplicated if(WITH_SOMETHING) blocks that all seemingly do the same thing, but are somehow copied to a gazillion files anyhow


It doesn’t have to be this way

Thankfully, CMake is here to help us…. if we let it. CMake allows you define certain things such as include directories, other libraries, or defines that any downstream user will need on targets.

Lets take bf_intern_guardedalloc as this one is virtually used everywhere. Take a look a bf_blenloaders usage of it (links to a commit from before I fixed it) and validate the problems it has:

  • Line 22 INC section, ../../../intern/guardedalloc fun relative path, where do the exactly headers live? we could guess at it, but hard to say with 100% certainty.

  • Line 64 LIB section, we use it, but do we tell the build system we need it to link? We don’t.

  • and it’s not just guardedalloc, there’s 10+ other libs that are also not communicated to the build system (the build system uses this information to figure out in which order to link things)

This is all a whole lot of manual labor to keep this in sync, and seemingly the kind of labor we’re not very good at, CMake to the rescue!

In what is coined “Modern CMake” it is possible for bf_intern_guardedalloc to communicate some things to its downstream users, ie hey if you use me, this is the include folder where you can find all my headers for instance.

What this effectively means is that if you add this information to bf_intern_guardedalloc (yes it really is that easy, just tag on PUBLIC and you’re done!) and you add bf_intern_guardedalloc to the LIB section of any other subproject, you do not have to worry about the INC section, even better if there was an include in the INC section it can be removed! Less to maintain! Less opportunity for things to get out of sync!


Hold up, that’s not what I have seen in the changes you landed so far! you used bf::intern::guardedalloc not bf_intern_guardedalloc in the LIB sections

Correct, i did use bf::intern::guardedalloc, this what is called an alias library, which as the name implies is just another name you can use to refer to bf_intern_guardedalloc and it is defined right where you define the original library

Sounds dumb, two names for the same thing

At first sight yeah it somewhat does, on closer look the alias target has a few benefits. Any target with :: in its name is expected to be a something known to cmake (as opposed some third party .lib/.a file from an external library, ie svn libs)

when you accidentally add bf::intern::guardedmalloc (notice the extra m there) to a LIB section, cmake will give you an error saying, “I have never heard about this, what are you talking about?”

while if you have used the original name with the same typo bf_intern_guardedmalloc (notice again the extra m) CMake would have gone “I don’t know what this is, i assume it’s some library on disk, i’m just gonna pass this on to the generated make/visual studio/ninja build files, and if it ends not up existing, that is clearly not my problem, make will crap out, or the compiler, or the linker, who knows, either way not my problem, good luck!”

the name spaced name allows us to catch typo’s early, which is nice, since they are much easier to deal with than random make/gcc/linker errors.

Added benefits

Given we cannot fix the mess that is our build system in one go as it is just too much work, there’s going to be a transition period where some targets will tell the world their needs and some will not.

for that reason, any target name you see with :: in their name you can assume has been updated, so no need to worry about messing with include folders, just add it to LIB and you’re good to go, cmake will take care off the rest. However if a :: name is not available, and you have to use the _ name of a library, you know things are as they were before, are responsible for updating both the LIB and INC sections to use this library.

What’s this PUBLIC/PRIVATE/INTERFACE business ?

To share its needs a library has a few options, and this is done though scope keywords that you can use in both the INC and LIB sections of our CMakeLists.txt files. if you do not put on a scope keyword PRIVATE is used.

the tl;dr here is (there’s much more nuance to it, but this will do for now, if you want to know more, there’s literally books written about it)

  • PRIVATE: I need this
  • INTERFACE : You need this
  • PUBLIC: the combination of the above two - we both need this

now what does this effectively mean? lets take a look at the INC section from bf_blenlib with additional comments :

set(INC
  PUBLIC .   # Anyone linking to bf_blenlib is going to need this folder in 
             # their includes, as this is where my headers live, also given
             # I use my own headers, I need this too. 

  ../../../intern/eigen # no scope set, which tells us : 
                        #  - Default PRIVATE scope applies, bf_blenlib will get 
                        # the eigen folder in its includes, but anyone linking to 
                        # bf_blenlib will not. 
                        #
                        #  - this hasn't gotten a closer look, someone should 
                        # look at how this is used and supply the correct scope
)

What does this effectively mean for the LIB section?

same rules apply as above, but with additional benefits.

lets take a look at the LIB section of bf_blenlib with additional comments:

set(LIB
  PUBLIC bf::dna    # Blenlib uses the dna headers, and more importantly 
                    # it uses them in its own headers (as opposed 
                    # to just in some internal .c/.cc` file) so if anyone links 
                    # to blenlib, they not just need the blenlib include folder
                    # passed to them, they will need the dna include folders
                    #  as well
  bf_intern_eigen 
  bf_intern_guardedalloc
  extern_wcwidth
  PRIVATE bf::intern::atomic # blenlib uses the atomic headers internally 
                             # only
  ${ZLIB_LIBRARIES}
  ${ZSTD_LIBRARIES}
)

INTERFACE makes no logical sense, why would a library make others depend on an include folder, but not require it themselves?

Yeah this does seem a bit obscure on first sight , but it has valid use cases! for instance bf_intern_atomic which is a header only library, it doesn’t have any c/cc files, so it really doesn’t need the include folder itself as no code is being build for it.

The work you are currently seeing is just the beginning of the initial pass at fixing the build system

Right now I’m just cleaning up the INC sections by advertising the include folder in the libraries, and adding any missing libraries to the LIB sections. Future work will go after the IF(WITH_SOMETHING) blocks and likely take a pass at the 3rd party dependencies as well. Ideally depending on optional third party library should be as easy as adding bf::dependencies::optional::libsomething to your LIB section and you’ll get everything you need like the -DWITH_SOMETHING passed to the compiler if it’s on, the include folders, and any .a/.so/.lib files you may need to link etc, without having to worry about any of it, wouldn’t that be something? :slight_smile:

Question time!

go at it! :slight_smile:

18 Likes

Yay for the direction and effort!

Please ping me when something needs fixing on mac!

2 Likes

Update: some of the work got reverted, the bf_blenkernel cleanup added many dependencies that should have been there, but weren’t. Which sounds like a good thing, however since blenkernel links essentially all of blender, this did bring out link order issues for which there were too many to resolve in any reasonable amount of time. Given these issues broke the gpu’s team shader builder and main ought to always be in a buildable state I chose to revert that change (and the few that were stacked on top of it)

I’ll continue to make improvements, but will work from the libraries with few dependencies to the larger ones hopefully resolving most of the problems before they occur when we tackle the bigger fish.

For clarity all changes were tested locally , and run though the buildbots before landing, however WITH_GPU_BUILDTIME_SHADER_BUILDER is off on the bots, and i do not have all platforms locally so testing coverage sadly isn’t where i’d like it to be.

3 Likes