Details

Reviewers

Ray Molenkamp (LazyDodo)
Brecht Van Lommel (brecht)

Commits

rC6816b6f547f1: CMake/macOS: Remove _LIBPATH, avoid link_directories
rB1f6b7387ad01: CMake/macOS: Remove _LIBPATH, avoid link_directories.

Summary

NOTE: this patch depends on D8917, D8918, D8919.

After tests were bundled in a single executable and cycles and libmv
created their own tests, the warnings on macOS have gone over 800.
The reason is setting *_LIBRARIES to single word names of the library
and later using link_directories to link them.

https://cmake.org/cmake/help/latest/command/link_directories.html

Note This command is rarely necessary and should be avoided where
there are other choices. Prefer to pass full absolute paths to
libraries where possible, since this ensures the correct library
will always be linked. The find_library() command provides the
full path, which can generally be used directly in calls to
target_link_libraries().

Warnings like the following popup for every target for every library
it links to.

ld: warning: directory not found for option
'-L/Users/me/blender-build/blender/../lib/darwin/jpeg/lib/Debug'

The patch uses absolute paths to link libraries and removes
all *_LIBPATHs except PYTHON_LIBPATH from
platform_apple.cmake file.

APPLE platform now no longer needs setup_libdirs() and
cycles_link_directories.

Diff Detail

Repository

rB Blender

Branch

linkdir (branched from master)

Build Status

Buildable 10636
Build 10636: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Remove find packages

Harbormaster completed remote builds in B10095: Diff 28708.Sep 9 2020, 8:44 PM

Ankit Meel (ankitm) planned changes to this revision.Sep 9 2020, 8:44 PM

Built Blender full with tests. #warnings = 77.

TO DO:

Remove other _LIBPATHS.
Test other build configurations.
Remove link_directories from blender target, that's 1 warning.

Harbormaster completed remote builds in B10139: Diff 28779.Sep 11 2020, 4:36 PM

There are still renames of variables output by standard cmake modules (ie BOOST_INCLUDE_DIRS)

Ankit Meel (ankitm) planned changes to this revision.Sep 11 2020, 4:42 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

In D8855#217317, @Ray Molenkamp (LazyDodo) wrote:

There are still renames of variables output by standard cmake modules (ie BOOST_INCLUDE_DIRS)

Could you comment on the relevant code line ? There's no BOOST_INCLUDE_DIRS.
If it's about the following, the change is to keep the name same as before as well as what the other platforms use.

-  set(BOOST_INCLUDE_DIR ${BOOST}/include)
+  set(BOOST_INCLUDE_DIR ${Boost_INCLUDE_DIRS})

Urgh you're right on the boost DIR vs DIRS, this has always been "wrong" our cmake is a giant mess.

build_files/cmake/macros.cmake
476	function called `SETUP_LIBDIRS` that doesn't setup any directories, seems iffy
build_files/cmake/platform/platform_apple.cmake
113	What is this doing here? either rely on find_package or don't, calling it and then redoing it your self is not the way to go. same comment applies to all other instances where it's clear the contents of a findXXXX have been pasted in
intern/cycles/cmake/macros.cmake
89–94	function called `cycles_link_directories` that doesn't setup any directories, seems iffy

Status: Blender lite, full and release build with GTests on.
Number of warnings vary on each, but are less than 90.

CMake/PCRE: remove unused PCRE_INCLUDE_DIRS.
CMake/PCRE: search in OpenCollada folder also.
CMake/XML2: search in OpenCollada folder for Windows and macOS.
CMake/FLAC: search in sndfile folder for macOS.
CMake/Blosc: search in OpenVDB folder for macOS.
CMake/LLVM: Remove global _LIBPATH, add _include_dirs.
Cleanup: CMake/Embree: Fix typo in comments.
CMake/FFmpeg: Add FFmpeg find module.
CMake/macOS: Remove most _LIBPATHs, use find modules.

Harbormaster completed remote builds in B10158: Diff 28808.Sep 13 2020, 1:09 AM

Ankit Meel (ankitm) planned changes to this revision.Sep 13 2020, 1:11 AM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) marked an inline comment as done.

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Sep 13 2020, 1:16 AM

target_link_libraries is not a replacement for link_directories. We are already linking to all those libraries. What needs to be done is making link_directories obsolete by making paths to libraries absolute. That way link_directories and setup_libdirs can be removed, not replaced.
Find*.cmake files should not have all kinds of changes specific to the way Blender precompiled libraries are organized. These are supposed to work for arbitrary Linux distributions. Put macOS specific changes in platform_apple.cmake.

Ankit Meel (ankitm) added a parent revision: D8919: CMake/LLVM: Add INCLUDE_DIRS to find package..Sep 17 2020, 1:46 PM

Ankit Meel (ankitm) added a parent revision: D8918: CMake/Find packages: add FindFFmpeg.cmake module..

Ankit Meel (ankitm) added a parent revision: D8917: CMake/OpenSubdiv: Rename INCLUDE_DIR -> INCLUDE_DIRS..

Ankit Meel (ankitm) mentioned this in D8917: CMake/OpenSubdiv: Rename INCLUDE_DIR -> INCLUDE_DIRS..

Remove unrelated changes, add parent revisions.

We are already linking to all those libraries.

I had been missing this for so long somehow. :(
The patch is now simpler than I imagined.

Harbormaster completed remote builds in B10253: Diff 28960.Sep 17 2020, 1:50 PM

Exclude link_directories for PNG and ZLIB.

Re python, rB9e62a5250946e2 commented out PYTHON_LIBRARY and set only
PYTHON_LIBPATH in certain condition.
https://developer.blender.org/diffusion/B/browse/master/build_files/cmake/platform/platform_apple.cmake$100-115

Need to test if it will link.

Harbormaster completed remote builds in B10254: Diff 28961.Sep 17 2020, 2:17 PM

Ankit Meel (ankitm) retitled this revision from [WIP] Remove link_directories and LIBPATHs from Blender. to CMake/macOS: Remove _LIBPATH, avoid link_directories..Sep 17 2020, 2:17 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) added a reviewer: Brecht Van Lommel (brecht).

Ankit Meel (ankitm) mentioned this in D8919: CMake/LLVM: Add INCLUDE_DIRS to find package..Sep 18 2020, 3:41 PM

Ankit Meel (ankitm) mentioned this in rBb746179d0add: CMake/OpenSubdiv: Rename INCLUDE_DIR -> INCLUDE_DIRS..Sep 18 2020, 3:42 PM

Ankit Meel (ankitm) mentioned this in rB402a4cadba49: CMake/LLVM: Add INCLUDE_DIRS to find package..

Add FFMPEG_FIND_COMPONENTS.
Fix typo in library name.
Fix boost thread not found error.

Harbormaster completed remote builds in B10435: Diff 29326.Sep 28 2020, 2:33 PM

Ankit Meel (ankitm) mentioned this in D8918: CMake/Find packages: add FindFFmpeg.cmake module..Sep 28 2020, 2:42 PM

Set boost libraries depending on "Threads" without using FindBoost.

As noted in the comments in the Boost finding code, wave depends on thread (See the official find module code),
and thread also depends on "Threads::Threads" (docs). That gives the error:

-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Could NOT find Threads (missing: Threads_FOUND) 
-- Found Boost: <stuff>/darwin/boost/include (found version "1.70.0") found components: date_time filesystem regex system wave serialization chrono atomic missing components: thread

A hack that worked was renaming libboost_thread.a to libboost_thread-mt.a and
then find wave and thread-mt via FindBoost as usual. But that's for later.

Also, BOOST_INCLUDE_DIR is used in a lot of places, it can also be corrected like D8917: CMake/OpenSubdiv: Rename INCLUDE_DIR -> INCLUDE_DIRS. later on.

Harbormaster completed remote builds in B10441: Diff 29338.Sep 28 2020, 4:01 PM

Don't link with TBB directory. All dir not found warnings in blender full + gtests removed.

~90 out of ~95 total warnings now are of deprecations.

Harbormaster completed remote builds in B10447: Diff 29344.Sep 28 2020, 5:11 PM

Ankit Meel (ankitm) mentioned this in rB2fdbe4d05011: CMake/Find packages: add FindFFmpeg.cmake module..Sep 28 2020, 9:04 PM

Add pretty-print function; fix some mistakes

Harbormaster completed remote builds in B10600: Diff 29676.Oct 7 2020, 12:23 PM

Brecht Van Lommel (brecht) requested changes to this revision.Oct 7 2020, 2:42 PM

Brecht Van Lommel (brecht) added inline comments.

build_files/cmake/platform/platform_apple.cmake
50–51	To avoid issues with finding system libraries instead of native Blender libraries, I think `without_system_libs_begin/end()` like `platform_unix()` should also be used.
106	`else(NOT JACK_FRAMEWORK)` -> `else()`.
117–119	This assumes that WITH_CODEC_FFMPEG is always enabled when WITH_CODEC_SNDFILE is enabled.
170	`REQUIRED`
171	Use `REQUIRED` like `platform_unix.cmake`
172	`REQUIRED`
228	Is setting `SDL2_LIBRARY` necessary? `platform_unix.cmake` does not have it.
231–235	All 3 `REQUIRED`.
237–254	The way `Boost_LIBRARIES` / `BOOST_LIBRARIES` is constructed is strange to me. `Boost_LIBRARIES` is used before `find_package`, so presumably it does not exist at that point yet? Options like `WITH_INTERNATIONAL` should probably influence the components passed to `find_package` instead. See how `platform_unix.cmake` does this. I also don't understand why there would be NOT_FOUND error, what is that error exactly? We don't have it on Linux. Is it a matter of passing the COMPONENTS in the right order?
287	Can you use `find_package(Blosc)`?
298–299	This `NOT LLVM_STATIC` code can be removed, it is never used and we are long past LLVM 3.4.

This revision now requires changes to proceed.Oct 7 2020, 2:42 PM

Ankit Meel (ankitm) added inline comments.Oct 7 2020, 3:09 PM

build_files/cmake/platform/platform_apple.cmake
237–254	Will fix the weird way of finding components by creating a list first. Boost_LIBRARIES is used before find_package, so presumably it does not exist at that point yet? There are two `find_package` calls. The first one finds required components which is stored in `BOOST_LIBRARIES`. The second calls appends to `Boost_LIBRARIES` (which is not going to be used). I also don't understand why there would be NOT_FOUND error, what is that error exactly? We don't have it on Linux. Is it a matter of passing the COMPONENTS in the right order? https://developer.blender.org/D8855#222249

Brecht Van Lommel (brecht) added inline comments.Oct 7 2020, 3:14 PM

build_files/cmake/platform/platform_apple.cmake
237–254	I guess both problems can be solved by matching `platform_unix.cmake` and having a single `find_package` call.

Cleanup: remove extra argument in else(), reorder messages.
Decouple Sndfile from FFmpeg.
Remove linker flag modification for LLVM 3.4
Use single call for find_package(Boost).
Add required flag for JPEG, PNG, TIFF, Freetype.
Add without_system_libs_{begin/end}; move Zlib out of this block.
Cleanup: Remove SDL2_LIBRARY.
Fix finding thread and wave for multi config generators + ASan. try_compile gets the compile flags (-fsanitize=*) but linker flags are not passed. Linking the test executable fails, so the check for pthread.h fails.
unset temporary variables

Harbormaster completed remote builds in B10616: Diff 29720.Oct 7 2020, 9:37 PM

Add explanatory comment.

Harbormaster completed remote builds in B10617: Diff 29721.Oct 7 2020, 9:44 PM

Ankit Meel (ankitm) added inline comments.Oct 7 2020, 9:44 PM

build_files/cmake/platform/platform_apple.cmake
287	It gives warning `-- Could NOT find Blosc (missing: BLOSC_INCLUDE_DIR)`

Ankit Meel (ankitm) mentioned this in rBd7f482f88ecb: CMake/macOS: Avoid passing compiler flags to try_compile..Oct 8 2020, 4:23 PM

Remove the workaround for thread and wave.

Ready for review.

Harbormaster completed remote builds in B10636: Diff 29747.Oct 8 2020, 4:34 PM

Thanks!

build_files/cmake/platform/platform_apple.cmake
287	Ah, it seems that on Linux we put blosc in a separate directory with includes. We could make macOS consistent with that once, but this is fine for now.