Page MenuHome
Differential D15696

CMake: support building with musl libc
ClosedPublic
Actions

Authored by listout (listout) on Aug 15 2022, 8:27 PM.

Details

Reviewers
Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)
Sybren A. Stüvel (sybren)
Summary

Instead of using macros like GLIBC we can use the CMake build
systems internal functions to check if some header or functions are
present on the running system's libc.




Add ./build_files/cmake/have_features.cmake to add checks for

platform features which can be used to set defines for source

files that require them.
Diff Detail
Repository 
rB Blender 
Event Timeline
listout (listout) requested review of this revision.Aug 15 2022, 8:27 PM
listout (listout) created this revision.
listout (listout) added a subscriber: Jesse Yurkovich (deadpin).EditedAug 15 2022, 8:36 PM
I seem have redefined a macro, I'll update the patch. Thanks to @Jesse Yurkovich (deadpin).



Update



Done
listout (listout) updated this revision to Diff 54695.Aug 15 2022, 9:01 PM
Changed the definition from HAVE_MALLOC_STATS to HAVE_MALLOC_STATS_H to avoid redefinition, as HAVE_MALLOC_STATS is already defined.
Campbell Barton (campbellbarton) requested changes to this revision.Aug 16 2022, 3:14 AM
The approach here seems reasonable, details noted inline.
intern/guardedalloc/CMakeLists.txt


9
add_compile_definitions was added in 3.12, where Blender's minimum supported CMake is 3.10.

Use add_definitions instead.


source/blender/blenlib/CMakeLists.txt


4–10
Better avoid unnecessary checks on windows, suggest to put this inside an if(NOT MSVC) check.


10
By convention we've moved away from including content within the endif.


source/blender/blenlib/intern/system.c


68
Does checking defined(__APPLE__) make sense now?


source/creator/CMakeLists.txt


4–5
Bad capitalization, also, finish all sentences with a full stop (same for other comments in this patch).


4–10
Better avoid unnecessary checks on non-Linux systems, suggest to put this inside an if(CMAKE_SYSTEM_NAME STREQUAL "Linux") check.


6–10
Use lowercase CMake commands & no space between the command and the bracket, also for HAVE_MALLOC_STATS check.
This revision now requires changes to proceed.Aug 16 2022, 3:14 AM
listout (listout) added a subscriber: Ray Molenkamp (LazyDodo).EditedAug 16 2022, 7:56 AM
@Campbell Barton (campbellbarton) I have the changes ready, just before submitting them wanted to ask one last thing that @Ray Molenkamp (LazyDodo) pointed out yesterday on chat. Do you think that keeping the (defined(__FreeBSD_kernel__) && !defined(__FreeBSD__)) from #if defined(HAVE_MALLOC_STATS_H) || (defined(__FreeBSD_kernel__) && !defined(__FreeBSD__)) makes sense?



To be more precise, is there any condition where HAVE_MALLOC_STATS_H is false but the second half of that conditional helping out there?



I think it's much like the #if defined(__APPLE__) check.
listout (listout) updated this revision to Diff 54706.EditedAug 16 2022, 8:17 AM
Changes suggested by Campbell Barton



    

  • Patch comment formatting (capitalization and ending period)
    • 

  • Not using caps for CMake function names
    • 

  • Removing spaces between if and (
    • 

  • Using add_definitions instead of add_compile_definitions
    • 

  • Make sure some of the checks in the patch are done only if necessary, for example not on Windows or OS other than Linux.
    • 

listout (listout) marked 6 inline comments as done.Aug 16 2022, 11:10 AM
listout (listout) marked an inline comment as done.Aug 16 2022, 5:06 PM
Brecht Van Lommel (brecht) added a comment.Aug 16 2022, 7:23 PM
You can remove (defined(__FreeBSD_kernel__) && !defined(__FreeBSD__)). It should not be necessary, and seems to have been added for kFreeBSD which is a dead project now.



Otherwise this looks good to me.
listout (listout) added a comment.Aug 17 2022, 8:48 AM


In D15696#426530, @Brecht Van Lommel (brecht) wrote:


You can remove (defined(__FreeBSD_kernel__) && !defined(__FreeBSD__)). It should not be necessary, and seems to have been added for kFreeBSD which is a dead project now.



Otherwise this looks good to me.





Thanks, I'll remove it.
Campbell Barton (campbellbarton) accepted this revision.Aug 17 2022, 9:18 AM
This revision is now accepted and ready to land.Aug 17 2022, 9:18 AM
listout (listout) updated this revision to Diff 54743.Aug 17 2022, 9:20 AM
Removing checks for macros __FreeBSD_kernel__ and __FreeBSD__
listout (listout) added a comment.Aug 17 2022, 9:23 AM
@Campbell Barton (campbellbarton) I just made a change on the suggestion of @Brecht Van Lommel (brecht), please have a look. (Just two mins after you accepted)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Aug 17 2022, 11:39 AM
Campbell Barton (campbellbarton) retitled this revision from [PATCH] Other musl build fixes. to Linux: support building with musl libc.
Campbell Barton (campbellbarton) requested changes to this revision.EditedAug 17 2022, 12:11 PM
Testing this patch I'm getting warnings:



In file included from intern/guardedalloc/intern/leak_detector.cc:11:                                                                                                                                                                                                                                                                                     
intern/guardedalloc/intern/mallocn_intern.h:35:19: note: ‘#pragma message: We don't know how to use malloc_usable_size on your platform’                                                                                                                                                                                                                  
   35 | #  pragma message "We don't know how to use malloc_usable_size on your platform"                                                                                                                                                                                                                                                                               
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~








This is caused by mallocn.c and related files being used in makesdna & makesrna, suggest a shared file to include checks for compiler/library features, see P3147 for an updated patch that resolves the warnings.
This revision now requires changes to proceed.Aug 17 2022, 12:11 PM
Campbell Barton (campbellbarton) retitled this revision from Linux: support building with musl libc to CMake: support building with musl libc.Aug 17 2022, 12:32 PM
listout (listout) added a comment.Aug 17 2022, 3:34 PM
This comment was removed by listout (listout).
listout (listout) added a comment.Aug 17 2022, 6:09 PM
This comment was removed by listout (listout).
listout (listout) updated this revision to Diff 54759.Aug 17 2022, 6:21 PM
listout (listout) edited the summary of this revision. (Show Details)
Add ./build_files/cmake/have_features.cmake to add checks for

platform features which can be used to set defines for source

files that require them.
Campbell Barton (campbellbarton) accepted this revision.Aug 17 2022, 11:44 PM
This revision is now accepted and ready to land.Aug 17 2022, 11:44 PM
Campbell Barton (campbellbarton) mentioned this in rBf197b1a1f1bb: CMake: support building with musl libc.Aug 18 2022, 12:47 AM
Brecht Van Lommel (brecht) closed this revision.Aug 18 2022, 4:39 PM
Committed in rBf197b1a1f1bbc0334310fb1c911327246767a1a3.
Revision Contents
Changeset List
PathSize
intern/
guardedalloc/
8 lines
intern/
3 lines
source/
blender/
blenlib/
8 lines
intern/
10 lines
creator/
8 lines
2 lines
Diff 54694
View Options
intern/guardedalloc/CMakeLists.txt
Loading...
View Options
intern/guardedalloc/intern/mallocn_intern.h
Loading...
View Options
source/blender/blenlib/CMakeLists.txt
Loading...
View Options
source/blender/blenlib/intern/system.c
Loading...
View Options
source/creator/CMakeLists.txt
Loading...
View Options
source/creator/creator_signals.c
Loading...