Fresh CMakeLists.txt #2912

pfeatherstone · 2024-02-06T21:30:24Z

This is an attempt at refactoring dlib's CMakeLists.txt files.
The objectives are:

Modern cmake, so no globals, very few variables, use the target_ functions
Use best practices where possible (i'm not a cmake expert so please shout out if you see a better way of doing something)
Reduce lines of code as much as possible
All deps are imported using find_package()
Use CMake's FindCUDAToolkit and others.

…f there is a more modern way of doing this.

- fix some weird thing in pybind11 CMakeLists.txt file - update setup.py script

- fixing actions

pfeatherstone · 2024-02-06T22:37:34Z

2 problems:

The implementation of BLAS in external/cblas is broken. Missing symbols
Cmake's findBLAS isn't detecting any blas libraries. So it's trying to use dlib's own version. Go to last point
I'll have a look tomorrow.

…rk but oh well

pfeatherstone · 2024-02-07T22:31:27Z

So added the missing BLAS files in external/blas. I didn't know there was a difference between cblas and blas. I have no idea how this was working before. We would have got errors like: "undefined reference to dgemm_" in file cblas_dgemm.c

Otherwise, I think the FindBLAS module in cmake is broken. Shame. It doesn't always detect perfectly valid BLAS libraries. So I'll quickly write a custom one tomorrow (steal quite a bit from dlib's old file)

Otherwise, the Python tests are failing in test_global_optimization.py
It has to do with translating exceptions between c++ and Python. No idea how to attack that one and don't know what's caused that.

pfeatherstone · 2024-02-08T08:45:40Z

The problem with test_global_optimization.py is DLIB_CASSERT(). The second time the assert gets hit, the program aborts. That's expected behaviour.

So I'm not sure how:

with raises(Exception):
    find_max_global(lambda a, b: 0, [0, 0, 0], [1, 1, 1], 10)
with raises(Exception):
    find_min_global(lambda a, b: 0, [0, 0, 0], [1, 1, 1], 10)
with raises(Exception):
    find_max_global(lambda a, b, c, d, *args: 0, [0, 0, 0], [1, 1, 1], 10)
with raises(Exception):
    find_min_global(lambda a, b, c, d, *args: 0, [0, 0, 0], [1, 1, 1], 10)

in test_global_optimization.py is supposed to work with DLIB_CASSERT() being used.

I've replaced a couple DLIB_CASSERT() statements with if(!(condition)) throw std::invalid_argument(msg);

…time you hit this, the program will abort

… an issue on windows build - only enable BLAS if either a valid BLAS library is found or fortran is enabled - correctly set DLIB_DISABLE_ASSERTS

arrufat

Yes, a bit confusing. It used to, though. The dependency was removed in #2273.

pfeatherstone · 2024-03-01T15:15:39Z

I've looked at some online tutorials on how to write cmake Config files for installable targets. It's crazy how complicated it is. It's laughable. There should be a better tool. I'm surprised there isn't a repo with some cmake helper scripts out there that figure out how to package a target.

davisking · 2024-03-02T04:03:13Z

What's currently there doesn't work? I mean it works on master. But you are finding that something about the new cmake setup breaks it I guess? The current thing is fairly simple 🤷‍♂️

pfeatherstone · 2024-03-02T07:16:33Z

Yeah if you look at the GitHub Action Runners, you'll see I've added one which demonstrates what I see. It builds and installs dlib then builds one of the examples using the installed version of dlib, i.e using find_package(dlib)
It doesn't recognise the imported targets.

pfeatherstone · 2024-03-02T07:17:41Z

It's because we are using imported targets now, not XXX_LINK_LIBRARIES and XXX_INCLUDE_DIRECTORIES

pfeatherstone · 2024-03-02T13:16:17Z

pkg-config is broken too since it uses the old dlib_needed_libraries variables and we've done away with that in this library in favour of imported targets. Probably need another runner that checks pkg-config functionality too.

pfeatherstone · 2024-03-02T14:56:31Z

Right, made some progress, while reading https://cmake.org/cmake/help/latest/command/install.html#export, I found the EXPORT_PACKAGE_DEPENDENCIES option which nearly fixes it by automatically adding statements like:

include(CMakeFindDependencyMacro)
find_dependency(Threads REQUIRED)
find_dependency(JPEG REQUIRED)
find_dependency(FFMPEG REQUIRED)
find_dependency(CUDAToolkit REQUIRED)
find_dependency(CUDNN REQUIRED)
find_dependency(MKL REQUIRED)

to the generated dlib.cmake file while running install(EXPORT dlib ...)
It's quite clever. It scans the dlib target and finds all its imported target dependencies.
However, this option is only available in cmake version 3.29. So this is bleeding edge.
Also, some of those options like find_dependency(MKL REQUIRED) won't work because it assumes there is a globally available FindMKL.cmake file somewhere, whereas that was in dlib's repository used at build time.

So I think any custom cmake modules, like FindMKL.cmake etc need to be installed along with dlib.cmake and dlibConfig.cmake.

Also i need to add find_dependency() statements to dlibConfig.cmake.in file.

Again, this isn't a problem on master, since it doesn't use imported targets, just plain old compile options and linker flags.
This PR is all about using modern cmake, which means using imported targets, which means all the above should be done.

Unless someone can come up with a better option.

I don't really like how we now have to maintain two lists of dependencies, one using find_package(...) in CMakeLists.txt and one using find_dependency(...) in `dlibConfig.cmake.in.

Is Cmake really this bad?

pfeatherstone · 2024-03-02T15:16:25Z

https://buildmedia.readthedocs.org/media/pdf/bcm/stable/bcm.pdf
this looks cool.

pfeatherstone · 2024-03-02T15:37:28Z

I have a working solution but now dlibConfig.cmake.in duplicates all the find_package() statements. I'm not keen on this.

pfeatherstone · 2024-03-03T12:48:14Z

I've added a test to CI/CD which attempts to build an example using pkg-config. So this needs fixing.
I'm of two minds:

Don't use cmake imported targets and go back to using XXX_INCLUDE_DIR and XXX_LIBRARIES for everything. This should fix all our problems with the generated dlib.cmake and dlib.pc files. But it's old school cmake. Apparently that's not best practice anymore. I don't know.
Use some sophisticated cmake to make all the imported targets work during install. Supposedly best practice, but you have to write either duplicated code, or disgusting cmake macros, or use third-party libraries that help with creating packages.

My opinion now is cmake is a pile of ***t

EDIT:
Also, generator expressions will be a problem when building dlib.pc. So it's possible all the generator expressions would have to be replaced with if-else expressions, as before. Unless there's something I've missed, I don't see how "modern cmake" actually solves anything anymore.

davisking · 2024-03-03T14:22:01Z

I've added a test to CI/CD which attempts to build an example using pkg-config. So this needs fixing. I'm of two minds:

Don't use cmake imported targets and go back to using XXX_INCLUDE_DIR and XXX_LIBRARIES for everything. This should fix all our problems with the generated dlib.cmake and dlib.pc files. But it's old school cmake. Apparently that's not best practice anymore. I don't know.

Use some sophisticated cmake to make all the imported targets work during install. Supposedly best practice, but you have to write either duplicated code, or disgusting cmake macros, or use third-party libraries that help with creating packages.

My opinion now is cmake is a pile of ***t

EDIT: Also, generator expressions will be a problem when building dlib.pc. So it's possible all the generator expressions would have to be replaced with if-else expressions, as before. Unless there's something I've missed, I don't see how "modern cmake" actually solves anything anymore.

Well, every year "modern cmake" is different. Which is fine. The only big change to cmake that I really think of as "the modern one" was when you started to be able to do target_link_libraries(my_app dlib::dlib) and that was all you needed. Whereas before you had to screw around with a bunch of other stuff as a client of a library that cmake itself was building.

I mean there are other things too. But the other big one is different platforms and their related tools keep changing so older versions of cmake accumulate a bunch of magic in user cmake scripts to deal with that stuff. And those things eventually get folded into cmake proper and everyone goes "see, new cmake is simple, you don't need that stuff!". But that is a never ending battle since other people keep screwing up/changing/over-complicating/whatevering their systems and tools in new and never ending ways.

Anyway, do you mean that stuff like target_link_libraries(dlib PUBLIC CUDA::cudart CUDA::cublas CUDA::curand CUDA::cusolver CUDNN::CUDNN) is not working with the install scripts? Because that sucks if so. That's been part of cmake for years and years now (well, not for cuda but the whole blah::blah target stuff). Got to be a way for that to work nicely.

davisking · 2024-03-03T14:29:46Z

dlib/cmake_utils/dlibConfig.cmake.in

-# ===================================================================================
-#  The dlib CMake configuration file
-#
-#             ** File generated automatically, do not modify **
-#
-#  Usage from an external project:
-#    In your CMakeLists.txt, add these lines:
-#
-#    find_package(dlib REQUIRED)
-#    target_link_libraries(MY_TARGET_NAME dlib::dlib)
-#
-# ===================================================================================


Totally keep the instructions. People see this stuff and then go "oh right, I'll do that" rather than do a bunch of confusing hacks.

davisking · 2024-03-03T14:31:42Z

dlib/cmake_utils/dlibConfig.cmake.in


+include(CMakeFindDependencyMacro)


Yeah it's screwy that these deps are repeated but it's simple so it's not the end of the world.

I'm just so dissatisfied with this. You would think cmake understands enough about a target that by the time you've built it it understands how to install it with one line of code.
Libraries like https://github.com/boost-cmake/bcm.git and https://github.com/robotology/ycm sort of provide this if you use their custom functions.

Yeah, it is annoying. It's also a bit of a regression compared to how it was before. But it's not a big deal at the end of the day, especially compared to how much other stuff is simpler so ¯_(ツ)_/¯

Though it doesn't fix dlib.pc for pkg-config. That needs specific compile and linker options. It there was a magic cmake macro/function which took all your imported targets, generator expressions, and resolved everything down to concrete compile and linker options, then everything would be good. It must do this anyway since it has to generate make/ninja files. If there was a way to reuse those functions then it would be great.

Ah, yeah you haven't figured out how to make it generate a dlib.pc file yet?

No sorry. I mean it generates it fine but it references libraries like blah::blah

I think I'll have to use something like file(GENERATE) as a hack

Doesn't work.

pfeatherstone · 2024-03-12T10:51:27Z

Shame this can't make it into the release but until dlib.pc.in is fixed...
I've searched everywhere and there is no simple solution.
The only thing to do is never use cmake imported targets and just use variables. Bit crap.
So not really worth merging if that's the solution. Let's just keep this open until a good one pops up.

davisking · 2024-03-12T12:33:13Z

Shame this can't make it into the release but until dlib.pc.in is fixed... I've searched everywhere and there is no simple solution. The only thing to do is never use cmake imported targets and just use variables. Bit crap. So not really worth merging if that's the solution. Let's just keep this open until a good one pops up.

Yeah that's annoying. Thanks for trying it though. At some point cmake will presumably make this possible. I'm also not super sure how popular pkg-config is. I don't use it, but I get asked about it often enough and before this stuff was in dlib there were complaints about it. So maybe popular? It's hard to tell the difference between a loud tiny minority and a bigger thing sometimes. Maybe not enough people care about pkg-config anymore for cmake to support this in a reasonable way?

pfeatherstone · 2024-03-12T13:06:47Z

What's nice about pkg-config is that it's portable. Multiple IDEs can us it (Eclipse, Netbeans), other c++ builders like Meson support it. But yeah, cmake is making it hard.
I imagine 90% of your users only care about the face detector, particularly in python, and pkg-config is irrelevant there.

davisking · 2024-03-13T12:14:31Z

What's nice about pkg-config is that it's portable. Multiple IDEs can us it (Eclipse, Netbeans), other c++ builders like Meson support it. But yeah, cmake is making it hard. I imagine 90% of your users only care about the face detector, particularly in python, and pkg-config is irrelevant there.

Heh, yeah, that face detector example is unreasonably popular 😅

arrufat · 2024-03-13T12:27:09Z

Heh, yeah, that face detector example is unreasonably popular 😅

I don't think it's unreasonable. Here we're complaining about how hard it is to make foolproof C++ builds, but it's still better than all the venv, conda, etc. nightmare.

Honestly. I am always surprised about how many people have problems building dlib. I guess that a lot of the issues are caused by conda replacing system libraries and leaving the system in a broken state.

pfeatherstone · 2024-03-13T12:28:55Z

Heh, yeah, that face detector example is unreasonably popular 😅

I don't think it's unreasonable. Here we're complaining about how hard it is to make foolproof C++ builds, but it's still better than all the venv, conda, etc. nightmare.

Honestly. I am always surprised about how many people have problems building dlib. I guess that a lot of the issues are caused by conda replacing system libraries and leaving the system in a broken state.

I don't know why people use conda. The only times i've used it it's caused me problems. What's wrong with pip install ... ?

arrufat · 2024-03-13T12:36:36Z

I don't know why people use conda. The only times i've used it it's caused me problems. What's wrong with pip install ... ?

Yeah, same here, I hate conda. I usually do python -m venv venv and then pip install everything there. Never had issues with that.

davisking · 2024-03-13T13:22:28Z

Yeah my experience with pip is pretty solid and that's what I always use. My main experience with conda is in all the people messaging me and telling me how conda broke their computer's ability to compile software.

pfeatherstone · 2024-03-13T22:41:53Z

Pinching the code from https://bcm.readthedocs.io/en/latest/ might be the best bet. It looks really cool.

davisking · 2024-03-14T12:13:00Z

Yeah but then that's one more dependency that gets out of date and needs updating. Might as well just keep what dlib has then.

…

On Wed, Mar 13, 2024 at 6:42 PM pfeatherstone ***@***.***> wrote: Pinching the code from https://bcm.readthedocs.io/en/latest/ might be the best bet. It looks really cool. — Reply to this email directly, view it on GitHub <#2912 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPYFRZDYFB5PJCWRGJCQODYYDI4NAVCNFSM6AAAAABC4W44TWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJWGAYTGNZZG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

davisking · 2024-03-14T12:13:52Z

Like if we aren't putting the responsibility on the cmake team to handle this stuff but just having cmake scripts in dlib's repo, what's the point? Since that's already the state of how things are now.

…

On Thu, Mar 14, 2024 at 8:12 AM Davis King ***@***.***> wrote: Yeah but then that's one more dependency that gets out of date and needs updating. Might as well just keep what dlib has then. On Wed, Mar 13, 2024 at 6:42 PM pfeatherstone ***@***.***> wrote: > Pinching the code from https://bcm.readthedocs.io/en/latest/ might be > the best bet. It looks really cool. > > — > Reply to this email directly, view it on GitHub > <#2912 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABPYFRZDYFB5PJCWRGJCQODYYDI4NAVCNFSM6AAAAABC4W44TWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJWGAYTGNZZG4> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

pfeatherstone · 2024-05-03T09:25:09Z

Cmake and pkg-config issue tracker

https://gitlab.kitware.com/cmake/cmake/-/issues/22621

arrowd · 2024-10-26T08:54:28Z

It is sad to see this work not merged in. Dlib is a pain to package properly due to homegrown find modules that do not work for non standard configurations.

pfeatherstone · 2024-10-26T12:26:04Z

Unfortunately if we want to maintain support for pkgconfig then we have to keep the current CMakeList.txt.
However, I've never seen a problem with it.

ich123 and others added 14 commits February 4, 2024 08:27

WIP

d1d265b

correct headers

8bb207d

find neon

2e294d4

SIMD

60254ff

use static_assert, not the silly macro

462e182

SIMD

30cd3df

install targets. This is a copy paste from old script. I don't know i…

8584073

…f there is a more modern way of doing this.

- build python in main CMakeLists.txt file

394d91e

- fix some weird thing in pybind11 CMakeLists.txt file - update setup.py script

disable LTO on python module

95b9210

wip

6d73a19

- build unit test in normal CMakeLists.txt file

ae5380e

- fixing actions

wip

6ff13c0

wip

8ec8b1c

install blas for now while we try fix the version of blas in external

02f35a0

Your Name and others added 8 commits February 7, 2024 08:13

use pkg-config

2d1fad2

window runner. don't know why ${{ github.workspace }}/dlib doesn't wo…

f59af43

…rk but oh well

missing blas fortran code

2bf1bad

missing blas

e65ed6b

mssing blas

404b5c4

missing blas

4cba233

deprecated

284fcbc

make it use our own blas

cb9c9b3

Your Name added 4 commits February 8, 2024 08:54

replace DLIB_CASSERT with normal throw statement. otherwise, the 2nd …

56b9fe8

…time you hit this, the program will abort

- check there is a fortran compiler before trying to enable it. Fixes…

9909cbd

… an issue on windows build - only enable BLAS if either a valid BLAS library is found or fortran is enabled - correctly set DLIB_DISABLE_ASSERTS

don't strip on windows.

ccf1356

confusingly, gui_widgets/fonts.cpp doesn't depend on X11

2031e45

arrufat reviewed Feb 8, 2024

View reviewed changes

pfeatherstone added 2 commits March 2, 2024 15:38

make installation work

ad66b68

test dlib installation through pkg-config

60a7461

davisking reviewed Mar 3, 2024

View reviewed changes

davisking added the enhancement label Mar 12, 2024

pfeatherstone mentioned this pull request Sep 18, 2024

Migrate to FindCUDAToolkit.cmake #3004

Closed

Fresh CMakeLists.txt #2912

Are you sure you want to change the base?

Fresh CMakeLists.txt #2912

Conversation

pfeatherstone commented Feb 6, 2024 • edited Loading

pfeatherstone commented Feb 6, 2024

pfeatherstone commented Feb 7, 2024 • edited Loading

pfeatherstone commented Feb 8, 2024 • edited Loading

arrufat left a comment

Choose a reason for hiding this comment

pfeatherstone commented Mar 1, 2024

davisking commented Mar 2, 2024

pfeatherstone commented Mar 2, 2024

pfeatherstone commented Mar 2, 2024

pfeatherstone commented Mar 2, 2024

pfeatherstone commented Mar 2, 2024 • edited Loading

pfeatherstone commented Mar 2, 2024

pfeatherstone commented Mar 2, 2024

pfeatherstone commented Mar 3, 2024 • edited Loading

davisking commented Mar 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfeatherstone commented Mar 12, 2024

davisking commented Mar 12, 2024

pfeatherstone commented Mar 12, 2024

davisking commented Mar 13, 2024

arrufat commented Mar 13, 2024

pfeatherstone commented Mar 13, 2024

arrufat commented Mar 13, 2024

davisking commented Mar 13, 2024

pfeatherstone commented Mar 13, 2024

davisking commented Mar 14, 2024 via email

davisking commented Mar 14, 2024 via email

pfeatherstone commented May 3, 2024

arrowd commented Oct 26, 2024

pfeatherstone commented Oct 26, 2024

pfeatherstone commented Feb 6, 2024 •

edited

Loading

pfeatherstone commented Feb 7, 2024 •

edited

Loading

pfeatherstone commented Feb 8, 2024 •

edited

Loading

pfeatherstone commented Mar 2, 2024 •

edited

Loading

pfeatherstone commented Mar 3, 2024 •

edited

Loading