Details

Reviewers

Brecht Van Lommel (brecht)
Sebastián Barschkis (sebbas)
Ray Molenkamp (LazyDodo)

Maniphest Tasks

T83631: Cycles OSL - compiling shaders failing due "\" and "n" symbols combination inside include paths - Needs OSL update

Commits

rC5fe5537a344e: Cmake/deps: Update OSL to 1.11.10.0
rB4fb0c83c1c85: Cmake/deps: Update OSL to 1.11.10.0

Summary

This resolves T83631

sometimes bumping a version is just editing versions.cmake and that's it, this howerver is not one of those cases, OSL made some rather significant changes to their build system (for the better) and turned the previous soft dependency on clang now a hard dependency (they dropped boost wave support) which snowballed a little into other deps.

OSL Expects clang/llvm to live in a single folder, convincing OSL that is not the case seems more work than actually making clang/llvm build together so this is addressed as well.

Included in this diff:
-OSL Update to 1.11.10.0
-refactor the llvm/clang/clang-tools-extra builds into the llvm build using the llvm-project tarball for building that has all of the subprojects in it.
-update ispc/openmp builds since clang no longer its own dependency and they have to depend on the llvm build now.
-Update the windows builder to use the 64 bit host tools since it ran out of ram linking clang
-Since OSL now needs clang to link successfully a findclang.cmake has been provided for linux/mac, I know for sure it links blender on linux at this point, it may or may not need additional tweaks to build/link D9465 and/or mac.

I'm somewhat divided on combining all of this into a single diff, they seem independend things, on the other hand they are also fully dependend on each other, and you cannot roll back one without having to roll back all others as well, so for that reason I have combined all into a single diff

builds and passes all tests on both windows and linux, was not able to test on mac, and I'm not entirely sure I didn't make any mistakes in the refactor of clang-tools-extra (which only builds on mac) so mac maintainers keep an eye on that!

review/test only at this point, landing may be a little tricky, keeping it all in one diff makes sense, however landing the clang stuff first would make things a lot eaiser though.. still torn there..

Note: There is a boost update in T83246: VFX Reference Platform 2021 Compatibility so this should be landing after that to prevent multiple updates of the same lib

Diff Detail

Repository

rB Blender

Branch

tmp_osl (branched from master)

Build Status

Buildable 12450
Build 12450: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Jan 26 2021, 4:24 PM

Harbormaster completed remote builds in B12394: Diff 33159.Jan 26 2021, 4:24 PM

Ray Molenkamp (LazyDodo) requested review of this revision.Jan 26 2021, 4:24 PM

Ray Molenkamp (LazyDodo) added reviewers: Brecht Van Lommel (brecht), Sybren A. Stüvel (sybren), Sebastián Barschkis (sebbas).Jan 26 2021, 4:32 PM

Ray Molenkamp (LazyDodo) added a subscriber: Ankit Meel (ankitm).

Ray Molenkamp (LazyDodo) mentioned this in T85087: Ctest: Crash in cycles_image_colorspace_cpu.Jan 26 2021, 8:27 PM

Fix missing headers on windows.

Harbormaster completed remote builds in B12450: Diff 33262.Jan 28 2021, 6:08 PM

Ankit Meel (ankitm) added inline comments.Jan 30 2021, 7:56 AM

CMakeLists.txt
419	Needs to be added to config files in `build_files/cmake/config/blender_*.cmake`
build_files/build_environment/cmake/llvm.cmake
30	Need to set `BUILD_CLANG_TOOLS` to ON. Or change the check in harvest to `APPLE`
build_files/cmake/Modules/FindClang.cmake
25	lower case clang in `/opt/lib/CLANG`
93	Lower case Clang. It'll create name mismatch warnings otherwise.

Ray Molenkamp (LazyDodo) edited the summary of this revision. (Show Details)Jan 31 2021, 5:39 PM

osx_locals.map and blender.map probably need to be updated to hide clang symbols (unless they are in the llvm namespace?). I can help with this but no time today.

CMakeLists.txt
419	`WITH_CLANG` needs to be enabled below the line `# auto enable llvm for cycles_osl`, otherwise it gives a link error. That mechanism could be changed to something more consistent with how we handle other dependencies, but easiest is to just follow we have we for `WITH_LLVM`.
build_files/cmake/Modules/FindClang.cmake
85	Remove debug print.

This revision now requires changes to proceed.Feb 1 2021, 8:19 PM

Updates with feedback
Merge remote-tracking branch 'origin/master' into tmp_osl

Harbormaster completed remote builds in B12534: Diff 33414.Feb 2 2021, 3:30 AM

Ray Molenkamp (LazyDodo) added a comment.Feb 2 2021, 3:30 AM

This comment was removed by Ray Molenkamp (LazyDodo).

Hide Clang symbols. It uses the clang:: namespace, so do the same as we did for hiding llvm:: symbols.

Harbormaster completed remote builds in B12547: Diff 33436.Feb 2 2021, 3:58 PM

Brecht Van Lommel (brecht) accepted this revision.Feb 2 2021, 3:58 PM

This revision is now accepted and ready to land.Feb 2 2021, 3:58 PM

Fine from my side. Builds correctly on Linux and Cycles OSL tests pass.

Cleanups: fix CMake name mismatch warning, lower case for functions, fix typos, use info_cfg_option.
No functional change.

Harbormaster completed remote builds in B12556: Diff 33453.Feb 2 2021, 7:18 PM

Brecht Van Lommel (brecht) mentioned this in T83246: VFX Reference Platform 2021 Compatibility.Feb 3 2021, 4:17 PM

Please remember to include install_deps (and ideally, to first create a task for such lib updates)... Annoying to always get that kind of info by random other tasks/topics.

I generally make those tasks when i'm actually expecting the other platform devs to start landing the libs which is not the case just yet here.

Ray Molenkamp (LazyDodo) mentioned this in T85365: Libraries Changes for Blender 2.93.Feb 5 2021, 2:16 AM

Apparently OSL stores the *.h files for the shaders in a different directory now (shaders/*.h → share/OSL/shaders/*.h). I needed this addition to the patch for the harvesting step to pick them up.

I didn't need anything extra to build Blender itself against the upgraded OSL.

diff --git a/build_files/build_environment/cmake/harvest.cmake b/build_files/build_environment/cmake/harvest.cmake
index b1686a75eaf..722b8ec5630 100644
--- a/build_files/build_environment/cmake/harvest.cmake
+++ b/build_files/build_environment/cmake/harvest.cmake
@@ -157,7 +157,7 @@ harvest(xr_openxr_sdk/lib xr_openxr_sdk/lib "*.a")
 harvest(osl/bin osl/bin "oslc")
 harvest(osl/include osl/include "*.h")
 harvest(osl/lib osl/lib "*.a")
-harvest(osl/shaders osl/shaders "*.h")
+harvest(osl/share/OSL/shaders osl/share/OSL/shaders "*.h")
 harvest(png/include png/include "*.h")
 harvest(png/lib png/lib "*.a")
 harvest(pugixml/include pugixml/include "*.hpp")

This revision now requires changes to proceed.Feb 16 2021, 12:58 PM

Sybren A. Stüvel (sybren) commandeered this revision.Feb 16 2021, 2:39 PM

Sybren A. Stüvel (sybren) edited reviewers, added: Ray Molenkamp (LazyDodo); removed: Sybren A. Stüvel (sybren).

This revision is now accepted and ready to land.Feb 16 2021, 2:39 PM

CMake/deps: OSL, harvest new shader directory

Harbormaster completed remote builds in B12920: Diff 34042.Feb 16 2021, 2:40 PM

CMake/deps: Updates and cleanups to build OSL on macOS

Harbormaster completed remote builds in B12974: Diff 34130.Feb 18 2021, 3:44 PM

@Sybren A. Stüvel (sybren) @Ray Molenkamp (LazyDodo) Are DOPENEXR_HOME, DILMBASE_HOME and DLLVM_ROOT needed on Windows / Linux?
On macOS they were just unused variables.

I added DOPENEXR_ROOT, DILMBASE_ROOT and DLLVM_DIRECTORY.

Ray Molenkamp (LazyDodo) added inline comments.Feb 18 2021, 3:49 PM

build_files/build_environment/cmake/osl.cmake
71	Duplicate option `-DBUILD_SHARED_LIBS=OFF`

@Sybren A. Stüvel (sybren) A. Stüvel (sybren) @Ray Fonk (ray) molenkamp (LazyDodo) Are DOPENEXR_HOME, DILMBASE_HOME and DLLVM_ROOT needed on Windows / Linux?

They were in the past, unsure if they are still needed currently, given how badly this diff snowballed, i just wanted it to be over and didn't do much cleanup

Sebastián Barschkis (sebbas) added inline comments.Feb 18 2021, 3:52 PM

build_files/build_environment/cmake/osl.cmake
71	Ah, that was the unused `DBUILDSTATIC` ..

CMake/deps: Removed duplicate shared libs variables

Harbormaster completed remote builds in B12975: Diff 34131.Feb 18 2021, 3:54 PM

This patch no longer applies to master, as rBae370e292af2f7092db02301e9deb6dd9d7a1441 also modified LLVM. I'll see if I can merge the changes.

Oh, the upgrade to llvm 11 in that commit will most likely break the OSL build on macOS arm ... @Ray Molenkamp (LazyDodo) this is not over yet :)

Rebase on master, merging in the macOS changes. @Sebastián Barschkis (sebbas) I'm expecting this to break macOS, can you check?

Harbormaster completed remote builds in B12999: Diff 34169.Feb 19 2021, 1:49 PM

The update to llvm was only for mac/arm64 should have left all other platforms intact ?

@Sybren A. Stüvel (sybren) On macOS x86 it builds just fine. For macOS arm I can test next week (I am fairly certain there will be a problem with OSL C++11 and LLVM 11).
What about ILMBASE_HOME, OPENEXR_HOME and LLVM_ROOT? Can they be removed? (also @Ray Molenkamp (LazyDodo))

In D10212#263900, @Ray Molenkamp (LazyDodo) wrote:

The update to llvm was only for mac/arm64 should have left all other platforms intact ?

It did, but this patch changes from "llvm-only" to "llvm-project", which I also blindly applied to the macOS conditional code in versions.cmake. This means I changed macOS-specific stuff without any testing.

as for the cleanup i'll have to once more build it and check, i'll have an answer over the weekend

OSL CMake parameters updated

Harbormaster completed remote builds in B13009: Diff 34186.Feb 19 2021, 5:58 PM

In D10212#263912, @Sebastián Barschkis (sebbas) wrote:

What about ILMBASE_HOME, OPENEXR_HOME and LLVM_ROOT? Can they be removed? (also @Ray Molenkamp (LazyDodo))

There have been some case changes. I get warnings that ILMBASE_HOME and OPENEXR_HOME are unused, but I also get these warnings:

-- Could NOT find Imath (missing: Imath_DIR)
-- Could NOT find IlmBase (missing: IlmBase_DIR)
-- Could NOT find OpenEXR (missing: OpenEXR_DIR)

I can't get it to find Imath though, even when I add -DImath_ROOT=${LIBDIR}/openexr/ it complains it can't find it. Builds fine without it, though.

Looking at the CMake output, it also complains that it can't find Qt5 files. We may want to add -DUSE_Qt5=OFF to prevent accidentally linking to Qt when they happen to be available on the system.

I've updated the patch for all of the above.

I can't get it to find Imath though, even when I add -DImath_ROOT=${LIBDIR}/openexr/ it complains it can't find it. Builds fine without it, though.

You can ignore that, took a closer look, Imath has been moved to its own package in 3.x it tries to find that first, if that fails it tries a few 2.x based strategies where it lives with the rest of openexr

current diff seems to build ok for me.

Builds fine on macOS, let's get this in!

Closed by commit rB4fb0c83c1c85: Cmake/deps: Update OSL to 1.11.10.0 (authored by Ray Molenkamp (LazyDodo)). · Explain WhyFeb 24 2021, 3:13 PM

This revision was automatically updated to reflect the committed changes.

Ray Molenkamp (LazyDodo) added a commit: rB4fb0c83c1c85: Cmake/deps: Update OSL to 1.11.10.0.

Phil Hargett (hargettp) mentioned this in T83631: Cycles OSL - compiling shaders failing due "\" and "n" symbols combination inside include paths - Needs OSL update.Feb 25 2021, 4:06 PM

Sebastián Barschkis (sebbas) mentioned this in rBfb7751f3e6c3: CMake/deps: Fix LLVM and OSL builds.Feb 26 2021, 5:39 PM

Sebastián Barschkis (sebbas) mentioned this in D10552: CMake/deps: Adjust OSL shader directory.Feb 26 2021, 9:14 PM

Sebastián Barschkis (sebbas) mentioned this in rBe00a87163cfd: CMake/deps: Adjust OSL shader directory.Feb 26 2021, 9:36 PM

Stefan Werner (swerner) added a commit: rC5fe5537a344e: Cmake/deps: Update OSL to 1.11.10.0.Jun 2 2021, 9:17 PM

Cmake/deps: Update OSL to 1.11.10.0
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 33262

CMakeLists.txt

build_files/build_environment/CMakeLists.txt

build_files/build_environment/cmake/clang.cmake

build_files/build_environment/cmake/harvest.cmake

build_files/build_environment/cmake/ispc.cmake

build_files/build_environment/cmake/llvm.cmake

build_files/build_environment/cmake/openmp.cmake

build_files/build_environment/cmake/osl.cmake

build_files/build_environment/cmake/versions.cmake

build_files/build_environment/patches/llvm.diff

build_files/build_environment/patches/osl.diff

build_files/build_environment/windows/build_deps.cmd

build_files/cmake/Modules/FindClang.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

intern/cycles/kernel/osl/CMakeLists.txt

Cmake/deps: Update OSL to 1.11.10.0ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 33262

CMakeLists.txt

build_files/build_environment/CMakeLists.txt

build_files/build_environment/cmake/clang.cmake

build_files/build_environment/cmake/harvest.cmake

build_files/build_environment/cmake/ispc.cmake

build_files/build_environment/cmake/llvm.cmake

build_files/build_environment/cmake/openmp.cmake

build_files/build_environment/cmake/osl.cmake

build_files/build_environment/cmake/versions.cmake

build_files/build_environment/patches/llvm.diff

build_files/build_environment/patches/osl.diff

build_files/build_environment/windows/build_deps.cmd

build_files/cmake/Modules/FindClang.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

intern/cycles/kernel/osl/CMakeLists.txt

Cmake/deps: Update OSL to 1.11.10.0
ClosedPublic
Actions

Revision Contents
Changeset List