Details
- Reviewers
Sergey Sharybin (sergey) Bastien Montagne (mont29) - Maniphest Tasks
- T51024: Change CYCLES_OSL to OSL_ROOT_DIR in install_deps.sh
- Commits
- rBS797b1d505315: Fix T51024: Switch install_deps to set OSL_ROOT_DIR instead of CYCLES_OSL.
rB797b1d505315: Fix T51024: Switch install_deps to set OSL_ROOT_DIR instead of CYCLES_OSL.
Diff Detail
- Repository
- rB Blender
Event Timeline
The way OSL is configured in CMake seems weird in general.
FindOpenShadingLanguage.cmake uses OSL_ROOT_DIR, but install_deps.sh uses CYCLES_OSL and platform_apple.cmake and platform_win32.cmake files seem to contain their own OSL library finder based on CYCLES_OSL.
@Sergey Sharybin (sergey), I guess you know what to do here, so I'll just add you.
The change seems fine to me. Wouldn't go a rabbit hole in this exact patch, but we should indeed clean up some CMake around OSL.
Currently in the middle of some compiler bug investigation and away from the current HEAD, so cant' commit. Adding @Bastien Montagne (mont29) to have final look (he maintains this script nowadays, so fulyl worth having his look as well). Hopefully Bastien can also commit this :)
Not totally sure I agree here… Blender's OSL path is stored in CYCLES_OSL, which is common to all OS's… OSL_ROOT_DIR is only a hint given to FindOpenShadingLanguage.cmake finding module, which is only used by Unix OS's. Even worse, unix sets OSL_ROOT, not OSL_ROOT_DIR, before calling it…
So indeed I think first thing first here is to clean up our CMake code around OSL. Just changing install_deps.sh is kind of meaningless here.
@Bastien Montagne (mont29), but that's exactly the thing. All other flags we pass to CMake are hints for find_package(). This is how it is supposed to work.
In fact, i don't see how CYCLES_OSL could work on Linux. It is initialized to ${LIBDIR}/osl` which is fully meaningless on Linux since LIBDIR is not initialized at all. Then it is only used to initialize OSL_ROOT unless the latter one was already set. But then OSL_ROOT is ignored and nowhere used. Unless i'm missing something ,it is not possible to control OSL installation folder on Linux atm.
Uuuh… Good point, will commit then. But still think this is a mess, and main issue remains in our cmake's OSL handling. It’s really bad when you have different options for same thing under different OS's. :(