opencolorio: use system lcms2 on aarch64 #45682
Conversation
| cmakeFlags = lib.optional stdenv.isDarwin "-DOCIO_USE_BOOST_PTR=ON"; | ||
| cmakeFlags = optional stdenv.isDarwin "-DOCIO_USE_BOOST_PTR=ON" | ||
| ++ optional (!stdenv.hostPlatform.isi686 && !stdenv.hostPlatform.isx86_64) "-DOCIO_USE_SSE=OFF" | ||
| ++ optional (stdenv.hostPlatform.isAarch32 || stdenv.hostPlatform.isAarch64) "-DUSE_EXTERNAL_LCMS=ON"; |
There was a problem hiding this comment.
Probably we can switch to a non-vendored lcms on all platforms. This makes testing easier.
There was a problem hiding this comment.
Shall I change this pull request / make another one to do that then?
While we're at it, should we go non-vendor-crazy and try using system yaml and system tinyxml as well?
There was a problem hiding this comment.
No, this pull request is fine. Yaml/tinyxml can be also de-vendored.
|
@GrahamcOfBorg build opencolorio |
|
Success on aarch64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: opencolorio Partial log (click to expand)
|
|
No problem de-vendoring tinyxml as well. The current version of libyamlcpp is incompatible (changes in iterator API), so we can either stick with the vendored yaml, build an older system yaml, or patch ocio to work with the more recent yaml (I haven't seen any such patches in the wild yet). Another minor issue I noticed is that the last commit (8ea7ad6#diff-2e251a0e479090d497b6670fee265f97) bumps the package version but doesn't actually change the sources (version is hardcoded in the fetch url). If I do change the fetch url to 1.1.0, it starts requiring git if using system lcms (haven't looked into why yet, something to do with patches) and no longer compiles. My inclination is to, at least initially, take the lazy route:
|
For now I would add
Why not just add git for now? |
There are a couple of other issues in 1.1.0 which cause the build to fail:
The first can be fixed by removing the offending |
|
Hang on! The |
|
Try |
Thra11
force-pushed
the
opencolorio-system-lcms
branch
from
73dc853
to
c6fbe9a
Compare
|
CXXFLAGS is cleaner, thanks. |
Thra11
force-pushed
the
opencolorio-system-lcms
branch
from
c6fbe9a
to
cf491c3
Compare
Thra11
force-pushed
the
opencolorio-system-lcms
branch
from
cf491c3
to
52d32c4
Compare
| }; | ||
|
|
||
| outputs = [ "bin" "out" "dev" ]; | ||
|
|
||
| buildInputs = [ cmake unzip ] ++ lib.optional stdenv.isDarwin boost; | ||
| nativeBuildInputs = [ cmake pkgconfig git ]; |
There was a problem hiding this comment.
Could you try to get rid of git? Or at least add a todo comment.
There was a problem hiding this comment.
If I try to remove git, I get
CMake Error at /nix/store/48jl65nlh1msp8yi4apbj7czp1dr978k-cmake-3.11.2/share/cmake-3.11/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
Could NOT find Git (missing: GIT_EXECUTABLE)
Call Stack (most recent call first):
/nix/store/48jl65nlh1msp8yi4apbj7czp1dr978k-cmake-3.11.2/share/cmake-3.11/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
/nix/store/48jl65nlh1msp8yi4apbj7czp1dr978k-cmake-3.11.2/share/cmake-3.11/Modules/FindGit.cmake:83 (find_package_handle_standard_args)
CMakeLists.txt:339 (find_package)
It looks like git is being used as a glorified patch command:
find_package(Git REQUIRED) ## in order to apply patch (for crossplateform
compatibility)
ExternalProject_Add(tinyxml
URL ${TINYXML_ZIPFILE}
SOURCE_DIR ${TINYXML_SOURCE_DIR}/tinyxml
PATCH_COMMAND ${GIT_EXECUTABLE} apply --ignore-whitespace ${TINYXML_
PATCHFILE}
I've added a todo.
Thra11
force-pushed
the
opencolorio-system-lcms
branch
from
52d32c4
to
940e35f
Compare
|
@GrahamcOfBorg build opencolorio |
|
|
||
| buildInputs = [ lcms2 tinyxml ] ++ optional stdenv.isDarwin boost; | ||
|
|
||
| CXXFLAGS = "-Wno-error=unused-function -Wno-error=deprecated-declarations"; |
There was a problem hiding this comment.
Can be -Werror disable all together? This is a maintenance nightmare when upgrading gcc.
There was a problem hiding this comment.
And could you also report the -Werror issue to uptream and add a comment with a link here.
There was a problem hiding this comment.
They can set it explicit in CFLAGS when running their CI build: https://github.com/imageworks/OpenColorIO/blob/master/.travis.yml
Apart from that they should default to -Wall.
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks like there's already a PR: AcademySoftwareFoundation/OpenColorIO#551 and some discussion: AcademySoftwareFoundation/OpenColorIO#518 on the subject of Werror. Not clear if/when they'll get round to doing something about it though.
For completeness, I opened an issue as well: AcademySoftwareFoundation/OpenColorIO#559
There was a problem hiding this comment.
I've replaced the CXX_FLAGS expression which disabled errors on unused-function and deprecated-declarations with a postPatch that strips both occurences of -Werror.
|
Success on aarch64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: opencolorio Partial log (click to expand)
|
The version of LCMS bundled with opencolorio is too old to build on aarch64, simply because its config files date from before aarch64 was announced. However, it can use the system lcms2 if it is found. Also de-vendor tinyxml. In addition, the version had been bumped to 1.1.0, but 1.0.9 was still hard-coded in the fetch uri. Some changes were necessary for 1.1.0 to build. As the sources are fetched from github, use fetchFromGitHub instead of fetchurl.
Thra11
force-pushed
the
opencolorio-system-lcms
branch
from
940e35f
to
43110d8
Compare
|
@GrahamcOfBorg build opencolorio |
|
Success on x86_64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: opencolorio Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: opencolorio Partial log (click to expand)
|
The version of LCMS bundled with opencolorio is too old to build on
aarch64, simply because its config files date from before aarch64
was announced. However, it can use the system lcms2 if it is found.
Motivation for this change
OpenColorIO fails to build on AArch64.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)cc @cillianderoiste
This will probably trigger rebuilds on i686 and x86_64 too, as I moved cmake and unzip from buildInputs to nativeBuildInputs.
We could probably switch to building against system LCMS on all platforms if we wanted to simplify things by removing all the
optionalbits. However, I haven't tested that.