Repository Restructure and CMake refactor #520

Shootfast · 2018-03-04T19:24:56Z

Hi,

This isn't quite 100% ready yet (I need to rebase with some latest patches and unit tests), but I'm putting it here for public review / criticism on what I've done so far.

Rearranged directory structure - libs / binaries / bindings are in their own directories. Headers are in include.
CMake rewritten. Uses target properties and exports where possible (still some work on ext libs to be done)
Windows builds out-of-the-box (using bundled yamlcpp and tinyxml)
Removed Boost dependency

What's not done yet:

unit tests (cpu and GPU)
make external packages correctly export their targets via CMake patches
misc installers I haven't added back in yet.

I'm hoping to tick of the remaining issues over the next week or so!

hodoulp · 2018-03-06T17:47:24Z

CMakeLists.txt

+project(OpenColorIO VERSION 1.2.0)
+
+set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)



Based on this page it seems that set(CMAKE_CXX_EXTENSIONS OFF) must be added to avoid platform specific C++11 extensions.

hodoulp

Not being an expert of cmake I've only review the code changes. I think that it's very nice refactoring of the code and a great simplification of the cmake files. So, that's a win for me 👍

hodoulp · 2018-03-06T17:51:51Z

share/cmake/FindLCMS2.cmake

+
+# attempt to find static library first if this is set
+if(LCMS2_STATIC_LIBRARY)
+	set(LCMS2_STATIC liblcms2.a)


That's a linux specific name.
For Windows, the names of the 'external' libraries are: lcms2.lib, libyamll-cppmd.lib and tinyxml.lib.

dbr · 2018-03-08T04:48:16Z

Looks like a great improvement!

A few minor comments on first glance:

I prefer the original src/core/ to src/OpenColorIO (otherwise it kind of sounds like all the other code isn't part of OCIO)
Confusing having src/pystring/ - seems like it should be under util (same as argparse etc), or I guess ideally under ext/ since it's a thirdparty project too.
I'd expect the tests to be under src/tests/ rather than a separate top-level folder
Lots of the files have been moved in a way which they lose their history, which is unfortunate (git blame etc can be very useful when tracking down bugs)

Might be out-of-scope for this ticket, but since it relates to tidying up the repo:

There's a separate changelog in docs which is empty and untouched for 6 years, could be removed
The rv integration code in thirdparty/rv/ is very outdated and should be deleted. Probably same with Mari one
ocioconvert and maybe ociodisplay feel a bit like example code rather than practical utilities (compared to ociocheck/ociobakelut/etc). Not sure if these should be handled differently, somehow. Particularly given the somewhat circular dependency where OIIO depends on OCIO (and these parts of OCIO then depend on OIIO)

scoopxyz · 2018-03-08T10:57:27Z

I prefer the original src/core/ to src/OpenColorIO (otherwise it kind of sounds like all the other code isn't part of OCIO)

I think its just to highlight that they aren't a part of the core library, which is true. You have executables that link against it, and language bindings that are built on top of it. I'd say they use OpenColorIO but aren't in and of themselves "OpenColorIO" the library, but are a part of "OpenColorIO" the project.

There's a separate changelog in docs which is empty and untouched for 6 years, could be removed
The rv integration code in thirdparty/rv/ is very outdated and should be deleted. Probably same with Mari one
ocioconvert and maybe ociodisplay feel a bit like example code rather than practical utilities (compared to ociocheck/ociobakelut/etc). Not sure if these should be handled differently, somehow. Particularly given the somewhat circular dependency where OIIO depends on OCIO (and these parts of OCIO then depend on OIIO)

I agree on the Changelog, just needs to be deleted. RV and Mari could also disappear since they're natively supported now. I'd say that ocioconvert should go, OIIO has supported OCIO for some time now, so it's redundant and frankly not useful.

ociodisplay is a little different. I think it's useful to provide a good reference implementation of OCIO, but I don't think ociodisplay satisfies that completely. It could be moved to its own repository so as to remove the OIIO dependency from OCIO completely.

scoopxyz · 2018-03-08T11:09:20Z

ah, ociolutimage also depends on OIIO

dekekincaid · 2018-03-08T16:19:55Z

Why would you remove ocioconvert? For automated tests it shouldn’t have an external dependency just to test itself. If you have a problem how do you quickly know if the issue is oiio or ocio?

…

On Thu, Mar 8, 2018 at 02:57 Sean Cooper ***@***.***> wrote: I prefer the original src/core/ to src/OpenColorIO (otherwise it kind of sounds like all the other code isn't part of OCIO) I think its just to highlight that they aren't a part of the core library, which is true. You have executables that link against it, and language bindings that are built on top of it. I'd say they *use* OpenColorIO but aren't in and of themselves "OpenColorIO" the library, but are a part of "OpenColorIO" the project. There's a separate changelog in docs which is empty and untouched for 6 years, could be removed The rv integration code in thirdparty/rv/ is very outdated and should be deleted. Probably same with Mari one ocioconvert and maybe ociodisplay feel a bit like example code rather than practical utilities (compared to ociocheck/ociobakelut/etc). Not sure if these should be handled differently, somehow. Particularly given the somewhat circular dependency where OIIO depends on OCIO (and these parts of OCIO then depend on OIIO) I agree on the Changelog, just needs to be deleted. RV and Mari could also disappear since they're natively supported now. I'd say that *ocioconvert* should go, OIIO has supported OCIO for some time now, so it's redundant and frankly not useful. *ociodisplay* is a little different. I think it's useful to provide a good reference implementation of OCIO, but I don't think *ociodisplay* satisfies that completely. It could be moved to its own repository so as to remove the OIIO dependency from OCIO completely. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHaAUpF5bq_GB_75GgTolPbpMDVRdD1ks5tcQ6XgaJpZM4Sbdef> .

scoopxyz · 2018-03-08T16:30:39Z

ocioconvert already has a dependency on OIIO, and there isn't anything that ocioconvert does that couldn't be done by Processor::apply apart from the file format handling that OIIO provides.

dbr · 2018-03-09T07:51:10Z

I prefer the original src/core/ to src/OpenColorIO (otherwise it kind of sounds like all the other code isn't part of OCIO)

I think its just to highlight that they aren't a part of the core library

Indeed, but seems like having src/core/ highlights this distinction as clearly as anything?

ociodisplay is a little different. I think it's useful to provide a good reference implementation of OCIO

ociodisplay is a definitely valuable piece of example code to have around (same with ocioconvert). I was thinking they could be organised a bit more like, say, the Qt "demos" example code - an clearly separate (optional) component when you build Qt (still be built via CI of course), creates a separate executable, and structured distinctly from the main source - e.g https://github.com/qt/qtbase/tree/5.11/examples/

Wouldn't be a big change, just a slightly clearer distinction between the "core" applications like ociocheck (might just be a case of moving them into examples/ instead of src/)

ah, ociolutimage also depends on OIIO

Hmm oh yeh - that is a bit unfortunate. I wonder if instead of OIIO it could use a few smaller libraries which we could bundle into the thirdparty modules (maybe TinyTIFF and TinyEXR). Much less flexible than OIIO, but that isn't necessary a bad thing (e.g would mean people couldn't accidentally try and extract a LUT from a compressed JPEG)

Probably worth splitting this into a separate ticket

hodoulp · 2018-03-09T16:04:36Z

Adding to the discussion: I would like to mention that at some point, we will have to add the half lut and perhaps the half bit depth for the apply(). Both require the half library part of the OpenEXR. So openEXR could be a good candidate for OCIO tools.
ocioDisplay is definitely a valuable part of the OCIO as example and as test tool.
CPU unit tests are 'white box' ones which means that they access private OCIO classes/methods. It explains why they are 'embedded' in core files (using OIIO macros). On the other hand, GPU unit tests are 'black box' ones (i.e. only using OCIO public API) so they could be in a separate directory. This distinction is not mandatory. So I will not argue on changes but I think that these two approaches are good to have.

hodoulp · 2018-09-14T20:13:03Z

@Shootfast Following the agreement explained in #556, this work is now critical for the next steps of OCIO v2. So, I took few hours to investigate the branch. Only few minor changes are needed to make it compile on Windows (except the unit tests which are missing).

ocio_mark_patch.diff.txt

FindIlmBase.cmake.txt

Not beeing a cmake expert please only use these files as a way to highlight where are the remaining problems.

scoopxyz · 2018-09-24T00:01:15Z

@Shootfast, noticed there we updated commits but I'm not certain if you're done.

For those following at home, here's the proposed structure from the latest commit on this fork:

├── docs
│   ├── configurations
│   │   └── images
│   │       └── spi-vfx
│   ├── developers
│   │   ├── api
│   │   └── bindings
│   ├── ociotheme
│   │   └── static
│   └── userguide
│       └── images
│           └── ps_icc
├── ext
│   ├── oiio
│   │   └── src
│   │       └── include
│   └── sampleicc
│       └── src
│           └── include
├── include
│   └── OpenColorIO
├── share
│   ├── cmake
│   ├── docker
│   ├── nuke
│   │   ├── examples
│   │   └── ocionuke
│   ├── ocio
│   └── sphinx
├── src
│   ├── java
│   │   ├── org
│   │   │   └── OpenColorIO
│   │   └── tests
│   │       └── org
│   │           └── OpenColorIO
│   ├── ociobakelut
│   ├── ociocheck
│   ├── ocioconvert
│   ├── ociodisplay
│   ├── ociolutimage
│   ├── OpenColorIO
│   │   ├── md5
│   │   └── pkgconfig
│   ├── openglbuilder
│   ├── pystring
│   ├── python
│   │   ├── DocStrings
│   │   ├── PyOpenColorIO
│   │   └── tests
│   └── util
├── testdata
├── tests
│   ├── cpu
│   └── gpu
└── thirdparty
    ├── aftereffects
    │   ├── mac
    │   ├── vc
    │   │   └── vc9
    │   │       ├── aftereffects
    │   │       └── ext
    │   ├── win
    │   └── xcode
    │       ├── aftereffects
    │       │   └── OpenColorIO_AE.xcodeproj
    │       ├── ext
    │       │   ├── lcms.xcodeproj
    │       │   ├── tinyxml.xcodeproj
    │       │   └── yaml.xcodeproj
    │       └── OpenColorIO.xcodeproj
    ├── mari
    │   ├── 1.4v1
    │   └── prototype
    ├── nuke
    │   ├── OCIOCDLTransform
    │   ├── OCIOColorSpace
    │   ├── OCIODisplay
    │   ├── OCIOFileTransform
    │   ├── OCIOLogConvert
    │   └── OCIOLookTransform
    ├── photoshop
    │   ├── mac
    │   ├── vc
    │   │   └── vc9
    │   ├── win
    │   └── xcode
    │       └── OpenColorIO_PS.xcodeproj
    └── rv
        ├── Mu
        ├── Packages
        └── Python

92 directories

I do think it is much more clear and straight forward, though I'm still not 100% on the overall structure. In particular ext and thirdparty are ambiguous to me. Isn't it pretty standard to put third party libraries in thirdparty? I also preferred having the apps in a separate directory, even if only for visual clarity.

So anyway, I took a stab at what it would like to me as a additional commit on top of your cmake work (which is much much better) and I didn't touch it much apart from dir changes. Found here:
https://github.com/scoopxyz/OpenColorIO/tree/shootfast-cmake_refactor

Directory layout here:

├── doc
│   ├── configurations
│   │   └── images
│   │       └── spi-vfx
│   ├── developers
│   │   ├── api
│   │   └── bindings
│   ├── ociotheme
│   │   └── static
│   └── userguide
│       └── images
│           └── ps_icc
├── include
│   └── OpenColorIO
├── share
│   ├── cmake
│   ├── docker
│   ├── nuke
│   │   ├── examples
│   │   └── ocionuke
│   ├── ocio
│   └── sphinx
├── src
│   ├── apps
│   │   ├── ociobakelut
│   │   ├── ociocheck
│   │   ├── ocioconvert
│   │   ├── ociodisplay
│   │   ├── ociolutimage
│   │   └── util
│   ├── bindings
│   │   ├── java
│   │   │   ├── org
│   │   │   │   └── OpenColorIO
│   │   │   └── tests
│   │   │       └── org
│   │   │           └── OpenColorIO
│   │   └── python
│   │       ├── DocStrings
│   │       ├── PyOpenColorIO
│   │       └── tests
│   ├── OpenColorIO
│   │   ├── md5
│   │   └── pkgconfig
│   └── util
│       ├── openglbuilder
│       └── pystring
├── test
│   ├── cpu
│   ├── data
│   └── gpu
├── thirdparty
│   ├── oiio
│   │   └── src
│   │       └── include
│   └── sampleicc
│       └── src
│           └── include
└── vendor
    ├── aftereffects
    │   ├── mac
    │   ├── vc
    │   │   └── vc9
    │   │       ├── aftereffects
    │   │       └── ext
    │   ├── win
    │   └── xcode
    │       ├── aftereffects
    │       │   └── OpenColorIO_AE.xcodeproj
    │       ├── ext
    │       │   ├── lcms.xcodeproj
    │       │   ├── tinyxml.xcodeproj
    │       │   └── yaml.xcodeproj
    │       └── OpenColorIO.xcodeproj
    ├── mari
    │   ├── 1.4v1
    │   └── prototype
    ├── nuke
    │   ├── OCIOCDLTransform
    │   ├── OCIOColorSpace
    │   ├── OCIODisplay
    │   ├── OCIOFileTransform
    │   ├── OCIOLogConvert
    │   └── OCIOLookTransform
    ├── photoshop
    │   ├── mac
    │   ├── vc
    │   │   └── vc9
    │   ├── win
    │   └── xcode
    │       └── OpenColorIO_PS.xcodeproj
    └── rv
        ├── Mu
        ├── Packages
        └── Python

95 directories

scoopxyz · 2018-09-24T00:03:32Z

Regardless of the final decision it should try to happen swiftly, so as to unblock the ADSK contributions...

Also your Python bindings weren't building correctly due to a missing definition:
add_definitions(-DPYOCIO_NAME=PyOpenColorIO)

In addition to Python3 being pulled in on MacOS and breaking

Shootfast · 2018-09-24T11:00:33Z

@Shootfast, noticed there we updated commits but I'm not certain if you're done.

Yeah, I'm almost finished. Just chasing the last few build bugs.

In particular ext and thirdparty are ambiguous to me.

ext are our own (OpenColorIO) external dependencies.
thirdparty are the third-party plugins we provide (that link against OpenColorIO)

I still have to shuffle some tests around (python and java binding tests in particular), and I have some linker errors on the windows side that I've not 100% found the cause of yet.

Shootfast · 2018-09-24T11:04:26Z

I like your changes though! I might pull those across / make the same changes to my branch. I haven't yet included putting the ops, fileformats, and transforms into their own subdirectories yet. Do you think that would be a worthwhile change to make here at the same time?

Shootfast · 2018-09-24T11:05:50Z

In addition to Python3 being pulled in on MacOS and breaking

Python3 is not supported yet right? It's the build bot configuration that is wrong, not me right? 😇

scoopxyz · 2018-09-24T12:25:59Z

Ok, yeah, I just think thirdparty has a definite meaning in other repos, so matching convention wouldn't hurt.

The vendor/DCC specific code is definitely odd since some of them really just work as examples, whereas the photoshop and aftereffects are the only real workhorses there that provide something the DCC doesn't, and the rest are outdated... but we can worry about that outside of this PR.

I would go ahead and subdir the transforms/filetypes. Patrick et. al. PRs should still be ready to merge because the they were git-moved, correct?

scoopxyz · 2018-09-24T12:31:31Z

For Python stuff, I think we need to manually force it to Python2.7 if Python3 doesn't work out of the box... at least until its fixed and working.

Seems like you'll specify by:

find_package(PythonLibs 2.7 REQUIRED)
find_package(PythonInterp 2.7 REQUIRED)

In both the python binding and docs CMakeLists.txt

scoopxyz · 2018-09-24T12:35:29Z

Submitted a PR to your branch if you wanted to pull my changes in.

scoopxyz · 2018-09-24T12:58:23Z

That did indeed fix the MacOS issue, but the Windows builds are horribly maimed...

hodoulp · 2018-09-24T13:56:22Z

Hoping it could help, here are my findings for the Windows platform:

jom ociodisplay, apps need an access to 'ilmbase' (Refer to my previous comment)
jom test_gpu_exec needs an access to 'glew'
jom test_cpu_exec symbol visibility issue, (will investigate later this morning)

Shootfast · 2018-09-24T14:13:44Z

jom ociodisplay, apps need an access to 'ilmbase' (Refer to my previous comment)

Nothing in OCIO currently depends on ilmbase, and there don't seem to be any includes for any of its components. This might be a red-herring

jom test_gpu_exec needs an access to 'glew'

Thanks for this one, I'll add it to the CMake for it (though I've disabled the GPU tests on the CI hosts)

jom test_cpu_exec symbol visibility issue, (will investigate this later morning)

Yeah any help here would be great. I'm fairly sure it's me building sub-components with the wrong visibility settings when they pull in the public api. Might be the unit tests too as they pull the same files (but don't link against the OpenColorIO dll)

hodoulp · 2018-09-24T14:14:03Z

BTW, I like @scoopxyz proposal for the new file structure.

hodoulp · 2018-09-24T14:35:01Z

jom ociodisplay, apps need an access to 'ilmbase' (Refer to my previous comment)

Nothing in OCIO currently depends on ilmbase, and there don't seem to be any includes for any of its components. This might be a red-herring

--> OIIO adds an explicit dependency on IlmBase includes
D:\OpenSource\3rdParty\compiled-dist_rls\OpenImageIO-1.9.0\include\OpenImageIO/simd.h(56): fatal error C1083: Cannot open include file: 'OpenEXR/ImathVec.h': No such file or directory

michdolan

Thanks for all your work on this @Shootfast !

scoopxyz

Carrying over the error that necessitates jumping to CMake 3.10.x:

-- The C compiler identification is MSVC 19.0.24213.1
-- The CXX compiler identification is MSVC 19.0.24213.1
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found OIIO: D:/OpenSource/3rdParty/compiled-dist_rls/OpenImageIO-1.9.0/lib/OpenImageIO.lib
-- Found ILMBASE: D:/OpenSource/3rdParty/compiled-dist_rls/IlmBase-2.2.0/include
CMake Error at src/apps/ociobakelut/CMakeLists.txt:7 (target_link_libraries):
  Target "lcms2" of type UNKNOWN_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.


-- Found GLEW: D:/OpenSource/3rdParty/glew-1.9.0/include
-- Found GLUT: D:/OpenSource/3rdParty/freeglut-3.0.0-2/lib/freeglut.lib
-- Found OpenGL: opengl32
-- Found PythonLibs: C:/Python27/libs/python27.lib (found suitable version "2.7.13", minimum required is "2.7")
-- Found PythonInterp: D:/Tools/cygwin64/bin/python2.7.exe (found suitable version "2.7.14", minimum required is "2.7")
CMake Error at src/OpenColorIO/CMakeLists.txt:77 (target_link_libraries):
  Target "tinyxml" of type UNKNOWN_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.


CMake Error at src/OpenColorIO/CMakeLists.txt:77 (target_link_libraries):
  Target "yamlcpp" of type UNKNOWN_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.


CMake Error at tests/cpu/CMakeLists.txt:13 (target_link_libraries):
  Target "tinyxml" of type UNKNOWN_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.
Call Stack (most recent call first):
  tests/cpu/CMakeLists.txt:118 (add_ocio_test)


CMake Error at tests/cpu/CMakeLists.txt:13 (target_link_libraries):
  Target "yamlcpp" of type UNKNOWN_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.
Call Stack (most recent call first):
  tests/cpu/CMakeLists.txt:118 (add_ocio_test)


-- Configuring incomplete, errors occurred!
See also "D:/OpenSource/ocio_mark/build_rls/CMakeFiles/CMakeOutput.log".

scoopxyz · 2018-09-27T09:05:36Z

Also, a summary of the changes included:

Jump to C++11
Dropping Boost dependency
CMake Minimum 3.10
Restructured/Modernized CMake
Restructured repo directories
Bumping master version to 2.0.0
Build Shared or Static libs not both
Explicit linking to Python2.7 libraries (for now)

@Shootfast correct me if wrong/missing things

Shootfast · 2018-09-27T10:37:03Z

The only side note is that the java bindings are not working at the moment, but that is already the case on the master branch, and I will try to fix them up soon. I do also want to make it so that the apps aren't built by default (as this leads to the circular dependency issue), but again I can make those changes after this patch is merged

hodoulp

Windows 7 with Vc14 in Release mode (and using jom), ctest -V fails:

3: Test command: C:\Python27\python.exe "D:/OpenSource/ocio_mark/tests/python/OpenColorIOTestSuite.py" "D:/OpenSource/ocio_mark/build_rls" "Release"
3: Test timeout computed to be: 10000000
3: Traceback (most recent call last):
3:   File "D:/OpenSource/ocio_mark/tests/python/OpenColorIOTestSuite.py", line 18, in <module>
3:     import PyOpenColorIO as OCIO
3: ImportError: No module named PyOpenColorIO
3/3 Test #3: test_python ......................***Failed    0.06 sec

hodoulp

Windows 7 using Vc14 in Debug mode, the Python library link fails:

version:0.0 /machine:x64 /debug /INCREMENTAL C:\Python27\libs\python27.lib ..\..\OpenColorIO\OpenColorIO.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:
CMakeFiles\PyOpenColorIO.dir/intermediate.manifest CMakeFiles\PyOpenColorIO.dir/manifest.res" failed (exit code 1104) with the following output:
LINK : fatal error LNK1104: cannot open file 'python27_d.lib'
[100%] Built target test_gpu_exec
jom: D:\OpenSource\ocio_mark\build_dbg\src\bindings\python\CMakeFiles\PyOpenColorIO.dir\build.make [src\bindings\python\PyOpenColorIO.pyd] Error 2
jom: D:\OpenSource\ocio_mark\build_dbg\CMakeFiles\Makefile2 [src\bindings\python\CMakeFiles\PyOpenColorIO.dir\all] Error 2```

because it looks for **python27_d.lib*** where default Python installation on Windows is release only.

The solution would be to have two Python installations which not a way to go on Windows platform.

hodoulp · 2018-09-27T14:00:12Z

[Minor] The command jom help does not display any target to only run Python tests.

hodoulp · 2018-09-27T14:02:24Z

[Minor] It would be nice to have a target like jom test_verbose to run test in verbose mode (i.e. like ctest -V)

hodoulp · 2018-09-27T14:13:52Z

Using the target Visual Studio 14 2015 Win64 with Vc14 in release mode, the command GPU & Python unit tests fail:

1/3 Test #1: test_cpu .........................   Passed    0.56 sec
test 2
    Start 2: test_gpu

2: Test command: D:\OpenSource\ocio_mark\build_rls\tests\gpu\Release\test_gpu_exec.exe
2: Test timeout computed to be: 10000000
2/3 Test #2: test_gpu .........................***Exception: Exit code 0xc0000135
  0.02 sec
test 3
    Start 3: test_python

3: Test command: D:\Tools\cygwin64\bin\python2.7.exe "D:/OpenSource/ocio_mark/tests/python/OpenColorIOTestSuite.py" "D:/OpenSource/ocio_mark/build_rls" "Release"
3: Test timeout computed to be: 10000000
3: Traceback (most recent call last):
3:   File "D:/OpenSource/ocio_mark/tests/python/OpenColorIOTestSuite.py", line 18, in <module>
3:     import PyOpenColorIO as OCIO
3: ImportError: No module named PyOpenColorIO
3/3 Test #3: test_python ......................***Failed    1.77 sec

33% tests passed, 2 tests failed out of 3

Total Test time (real) =   2.40 sec

The following tests FAILED:
          2 - test_gpu (Exit code 0xc0000135
)
          3 - test_python (Failed)
Errors while running CTest

Update
After some investigations, I found that VS does not create the output results at the specified location, but rather at the location plus "/Release". The GPU unit tests are using the OpenColorIO dynamic library (black box testing) and failed to find it.

A solution is to update the execution environment.

Closed

hodoulp · 2018-09-27T14:17:12Z

[Minor] The compilation variable OCIO_WARNING_AS_ERROR is not used by the build

CMake Warning:
  Manually-specified variables were not used by the project:

    OCIO_WARNING_AS_ERROR

ext/CMakeLists.txt

hodoulp · 2018-09-27T14:53:39Z

ext/CMakeLists.txt

+	endif()
+
+	if(NOT BUILD_SHARED_LIBS)
+		#TODO: Find a way to merge in the static libs when built with internal yamlcpp


typo 'yaml' --> 'tinyXML'

Fixed, and added the same warning to lcms2!

ext/patch.cpp

hodoulp · 2018-09-27T15:09:44Z

src/OpenColorIO/CMakeLists.txt

+)
+
+add_library(OpenColorIO ${SOURCES})
+set_property(TARGET OpenColorIO PROPERTY CXX_STANDARD 11)


The C++ version selection is already done in the CMakeLists.txt line 5. Why is that needed here too?
If oneday the C++ version is changed, only one location should be changed.

Correct, This was leftover from an earlier test. I've removed the additional check.

src/OpenColorIO/CMakeLists.txt

src/OpenColorIO/Op.cpp

hodoulp · 2018-09-27T15:21:44Z

src/apps/CMakeLists.txt

+	add_subdirectory(ociobakelut)
+	add_subdirectory(ociocheck)
+
+	if(TARGET OpenImageIO)


Sometime some external libraries are checked (i.e. OpenImageIO) and the build is 'adjusted'. Sometimes some external libraries (GLUT, GLEW) are required, and the build fails if they are not present. On the Windows platform, the two cases are strictly equivalent as none of these libraries are installed by default.

I would recommend following the same rule (i.e. the OpenImageIO one) for all external libraries.

This is a tricky one. The presence of OIIO is the common requirement for those additional apps, hence me using it to drive their inclusion. In the past we considered OIIO to be a "non blocking" requirement, as you can still build some tools without it. I would be tempted to remove the special check for OIIO and make the entire OCIO_BUILD_APPS require OIIO, as this might also help the people with the circular dependency problem.
This would treat OIIO, GLUT and GLEW as hard dependencies for all apps, and we could just rely on the traditional find_package(OIIO) calls.

On the windows platform, the makefile generation will fail right away as none of these libraries are present by default. I would prefer to let Windows devs be able to generate the makefiles and compile successfully even if some apps are missing (and GPU unit tests are missing too).
Another point is that the libraries are part of two different categories, one to read/write a file (OIIO) and some to have GPU. For example, GPU unit tests only need GPU related libraries while ociodisplay needs both categories.
Note: The find_package with some ' potential path' will never work on Windows.

My proposal is to only have soft dependencies with clear messages explaining the 'adjustment'.

With the long-term maintenance perpective in mind, we should help devs (*nix or Windows platforms) successfully compile the library itself whatever is the platform.

src/bindings/java/CMakeLists.txt

tests/cpu/CMakeLists.txt

tests/gpu/CMakeLists.txt

hodoulp · 2018-09-27T16:55:32Z

Compiling (i.e. Vc14 in Release mode) with -DBUILD_SHARED_LIBS=OFF fails because of symbol issues.
...

PyConstants.cpp.obj : error LNK2001: unresolved external symbol "__declspec(dllimport) enum OpenColorIO::v2_0::BitDepth __cdecl OpenColorIO::v2_0::BitDepthFromString(char const *)" (__imp_?BitDepthFromString@v2_0@OpenColorIO@@YA?AW4BitDepth@12@PEBD@Z
)
PyColorSpace.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: static class std::shared_ptr<class OpenColorIO::v2_0::ColorSpace> __cdecl OpenColorIO::v2_0::ColorSpace::Create(void)" (__imp_?Create@ColorSpace@v2_0@Open
ColorIO@@SA?AV?$shared_ptr@VColorSpace@v2_0@OpenColorIO@@@std@@XZ) referenced in function "int __cdecl OpenColorIO::v2_0::`anonymous namespace'::PyOCIO_ColorSpace_init(struct OpenColorIO::v2_0::PyOCIOObject<class std::shared_ptr<class OpenColorIO::v2
_0::ColorSpace const >,class std::shared_ptr<class OpenColorIO::v2_0::ColorSpace> > *,struct _object *,struct _object *)" (?PyOCIO_ColorSpace_init@?A0x454721e1@v2_0@OpenColorIO@@YAHPEAU?$PyOCIOObject@V?$shared_ptr@$$CBVColorSpace@v2_0@OpenColorIO@@@s
td@@V?$shared_ptr@VColorSpace@v2_0@OpenColorIO@@@2@@23@PEAU_object@@1@Z)
jom: D:\OpenSource\ocio_mark\build_rls\src\bindings\python\CMakeFiles\PyOpenColorIO.dir\build.make [src\bindings\python\PyOpenColorIO.pyd] Error 2

Note: That case should be added to the CI builds.

Note: If it's problematic to fix, it could be done in a follow-up pull request i.e. not blocking

Shootfast · 2018-09-27T17:57:55Z

[Minor] It would be nice to have a target like jom test_verbose to run test in verbose mode (i.e. like ctest -V)

I am not a fan of additional targets that are created solely for non-build related tasks. If you want verbose tests, run ctest -V!

I've made the verbose output be the default for the CI jobs.

Shootfast · 2018-09-27T18:02:59Z

After some investigations, I found that VS does not create the output results at the specified location, but rather at the location plus "/Release". The GPU unit tests are using the OpenColorIO dynamic library (black box testing) and failed to find it.

A solution is to update the execution environment.

The current Python test runner is a bit of a hack. I might try and get cmake to pass the built location of OpenColorIO and PyOpenColorIO explicitly, rather than relying on my guesswork.

hodoulp · 2018-09-28T14:22:14Z

After a discussion with @Shootfast and the fact that the work is blocking for any development on the repo, we agreed to merge the work as-is. The modernization of the depot (refactor/simplify of cmake files, and file source code retructuration) is now successfully completed.

Here is a summary of missing parts:

Compile in 'static library' mode has some symbol issues (dynamic library is working fine) Static library link fails on Windows #590
Discussion around hard vs. soft requirement for external libraries (i.e. OIIO, Glew...) is still pending
Python unit test failure on Windows Python unit tests fail to run on Windows #591
Python debug issue on Windows when linking PyOpenColorIO Python library fails to compile on Windows debug mode #592
Java binding & unit tests not done

@Shootfast please correct me if needed.

) * Jump to C++11 * Removed Boost dependency * Bumped CMake Minimum 3.10 * Modernized CMake builders * Restructured repo directories * Set project version to 2.0.0

hodoulp reviewed Mar 6, 2018

View reviewed changes

hodoulp mentioned this pull request Sep 14, 2018

Add Academy/ASC Common LUT Format (CLF) read support (#36) #556

Closed

Shootfast added 6 commits September 21, 2018 20:10

Initial repostitory restructure

66e6019

Rewrote CMake builders

bb99740

Updated unit test includes

6f7bb1d

Updated CI builders

37ff5a7

Fixed windows fstream issues

e32fd92

Patched out embedded yaml-cpp shenanigans

cca5709

hodoulp mentioned this pull request Sep 26, 2018

Repo Rules #578

Closed

michdolan previously approved these changes Sep 27, 2018

View reviewed changes

scoopxyz self-requested a review September 27, 2018 08:52

scoopxyz previously approved these changes Sep 27, 2018

View reviewed changes

hodoulp reviewed Sep 27, 2018

View reviewed changes

hodoulp requested changes Sep 27, 2018

View reviewed changes

Additional restructuring following code review

22c23a6

Shootfast dismissed stale reviews from scoopxyz and michdolan via 22c23a6 September 27, 2018 18:09

scoopxyz approved these changes Sep 28, 2018

View reviewed changes

hodoulp approved these changes Sep 28, 2018

View reviewed changes

Shootfast merged commit 2f5fbfc into AcademySoftwareFoundation:master Sep 28, 2018

scoopxyz mentioned this pull request Sep 28, 2018

On linux, there's a cmake option USE_EXTERNAL_LCMS, but it isn't used #558

Closed

hodoulp mentioned this pull request Oct 3, 2018

Fix oiio dependent apps compilation #560

Closed

This was referenced Jan 8, 2019

About dropping Boost and C++ 11. #484

Closed

ninja not working because of missing dependencies #467

Closed

hodoulp mentioned this pull request Apr 2, 2019

Make it possible to build OpenColorIO for Windows on Linux #404

Closed

Repository Restructure and CMake refactor #520

Repository Restructure and CMake refactor #520

Conversation

Shootfast commented Mar 4, 2018

Choose a reason for hiding this comment

hodoulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbr commented Mar 8, 2018

scoopxyz commented Mar 8, 2018

scoopxyz commented Mar 8, 2018

dekekincaid commented Mar 8, 2018 via email

scoopxyz commented Mar 8, 2018 • edited

dbr commented Mar 9, 2018

hodoulp commented Mar 9, 2018

hodoulp commented Sep 14, 2018

scoopxyz commented Sep 24, 2018

scoopxyz commented Sep 24, 2018 • edited

Shootfast commented Sep 24, 2018

Shootfast commented Sep 24, 2018

Shootfast commented Sep 24, 2018

scoopxyz commented Sep 24, 2018 • edited

scoopxyz commented Sep 24, 2018

scoopxyz commented Sep 24, 2018

scoopxyz commented Sep 24, 2018

hodoulp commented Sep 24, 2018 • edited

Shootfast commented Sep 24, 2018

hodoulp commented Sep 24, 2018

hodoulp commented Sep 24, 2018 • edited

michdolan left a comment

Choose a reason for hiding this comment

scoopxyz left a comment

Choose a reason for hiding this comment

scoopxyz commented Sep 27, 2018

Shootfast commented Sep 27, 2018

hodoulp left a comment

Choose a reason for hiding this comment

hodoulp left a comment

Choose a reason for hiding this comment

hodoulp commented Sep 27, 2018 • edited

hodoulp commented Sep 27, 2018

hodoulp commented Sep 27, 2018 • edited

hodoulp commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hodoulp commented Sep 27, 2018

Shootfast commented Sep 27, 2018

Shootfast commented Sep 27, 2018

hodoulp commented Sep 28, 2018 • edited

scoopxyz commented Mar 8, 2018 •

edited

scoopxyz commented Sep 24, 2018 •

edited

scoopxyz commented Sep 24, 2018 •

edited

hodoulp commented Sep 24, 2018 •

edited

hodoulp commented Sep 24, 2018 •

edited

hodoulp commented Sep 27, 2018 •

edited

hodoulp commented Sep 27, 2018 •

edited

hodoulp commented Sep 28, 2018 •

edited