CMake: Fix message() types #879

Simran-B · 2021-02-08T23:18:09Z

Summarize your change.

Removed incorrect commas after message() type
Use proper message type STATUS (there is no INFO)
~~Removed D from ${DOTIO_PYTHON_INSTALL_DIR}~~
Replaced : by = in message text.
Colon is used for type annotation in CMake's syntax, like -DOTIO_DEPENDENCIES_INSTALL:BOOL=OFF

CMakeLists.txt

meshula · 2021-02-09T01:27:19Z

If you wouldn't mind, please rebase this change over the fixes I pushed for the installation problem. The redundant message was rewritten to make it the correct message for that part of the if clause.

Simran-B · 2021-02-09T09:02:43Z

I'm pretty sure that the comments about the paths do not match the actual behavior or that the paths aren't set as intended:
https://github.com/PixarAnimationStudios/OpenTimelineIO/pull/879/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR47-L54

With the following commands on Windows:

cmake .. -G "Visual Studio 16 2019" -DOTIO_CXX_INSTALL=OFF -DCMAKE_INSTALL_PREFIX=appdir
cmake --build . --config Release
cmake --install . --config Release

I end up with below files in the install folder (appdir):

include/opentimelineio/deps/any/any.hpp
include/opentimelineio/deps/nonstd/optional.hpp
lib/_opentime.pyd
lib/_otio.pyd
python/bin/otio*.bat
python/opentimelineio/*.py

With cmake .. -G "Visual Studio 16 2019" -DOTIO_PYTHON_INSTALL=OFF -DCMAKE_INSTALL_PREFIX=appdir:

OpenTime*.cmake
include/opentime/*.h
include/opentimelineio/*.h
include/opentimelineio/deps/any/any.hpp
include/opentimelineio/deps/nonstd/optional.hpp
lib/opentime.lib
lib/opentime.dll
lib/opentimelineio.lib
lib/opentimelineio.dll

With cmake .. -G "Visual Studio 16 2019" -DCMAKE_INSTALL_PREFIX=appdir:

OpenTime*.cmake
include/opentime/*.h
include/opentimelineio/*.h
include/opentimelineio/deps/any/any.hpp
include/opentimelineio/deps/nonstd/optional.hpp
lib/opentime.lib
lib/opentime.dll
lib/opentimelineio.lib
lib/opentimelineio.dll
lib/_opentime.pyd
lib/_otio.pyd
python/bin/otio*.bat
python/opentimelineio/*.py

With cmake .. -G "Visual Studio 16 2019" -DOTIO_PYTHON_INSTALL_DIR="C:/Path/To/OpenTimelineIO/build/appdir":

opentimelineio/opentimelineio.lib
opentimelineio/opentimelineio.dll
opentimelineio/opentime.lib
opentimelineio/opentime.dll
opentimelineio/_opentime.pyd
opentimelineio/_otio.pyd
opentimelineio/*.py
opentimelineio/lib/OpenTime*.cmake
opentimelineio/lib/include/opentime/*.h
opentimelineio/lib/include/opentimelineio/deps/any/any.hpp
opentimelineio/lib/include/opentimelineio/deps/nonstd/optional.hpp
opentimelineio/lib/include/opentimelineio/*.h
bin/otio*.bat

if only CMAKE_INSTALL_PREFIX has been set,
- Python: ${CMAKE_INSTALL_PREFIX}/opentimelineio/python ❌python/opentimelineio
- C++: ${CMAKE_INSTALL_PREFIX}/opentimelineio ❌no parent opentimelineio folder, but include folder has subfolder opentimelineio , lib/dll files directly in lib folder
if only OTIO_PYTHON_INSTALL_DIR has been set,
- Python: ${OTIO_PYTHON_INSTALL_DIR}/opentimelineio ✔ but also lib/dll files?!
- C++: ${OTIO_PYTHON_INSTALL_DIR}/opentimelineio/cxx-libs ❌subfolder lib contains includes only

meshula · 2021-02-09T20:25:44Z

Gosh, we need a spreadsheet, I think. C++ on/off, Python on/off, install_prefix set/not-set, python_install set/not-set, and the expected outcomes for all combinations...

meshula · 2021-02-09T20:27:58Z

This PR is looking good; thanks for all the leg work, and improvements to the in-code documentation.

CMakeLists.txt

meshula

latest push still looking good

CMakeLists.txt

codecov-io · 2021-02-10T23:53:50Z

Codecov Report

Merging #879 (9eb5f50) into master (2a33b58) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #879   +/-   ##
=======================================
  Coverage   84.36%   84.36%           
=======================================
  Files          74       74           
  Lines        3090     3090           
=======================================
  Hits         2607     2607           
  Misses        483      483

Flag	Coverage Δ
unittests	`84.36% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a33b58...9eb5f50. Read the comment docs.

CMakeLists.txt

meshula · 2021-02-11T01:59:20Z

For -DCMAKE_INSTALL_PREFIX=some/path - I think this is what you're proposing? And it's what I expect personally in that case

.
├── OpenTimeConfig-debug.cmake
├── OpenTimeConfig.cmake
├── OpenTimelineIOConfig-debug.cmake
├── OpenTimelineIOConfig.cmake
├── include
├── bin
│   ├── otioview.exe
│   ├── dlls
│   ├── otiocat.bat
│   ├── otioconvert.bat
│   ├── otiopluginfo.bat
│   └── otiostat.bat
├── lib
├── python
│   ├── opentimelineio
│   ├── __init__.py
│   ├── dlls / pyds
│   ├── adapters
│   ├── algorithm
│   ├── console
│   ├── core
│   ├── plugins
│   ├── schema
│   ├── schemadef
│   └── OpenTimelineIOConfig.cmake
└── testutils.py

Simran-B · 2021-02-11T11:42:12Z

@meshula With my proposed changes, the resulting tree looks different to your example:

├───OpenTimeConfig-debug.cmake
├───OpenTimeConfig.cmake
├───OpenTimelineIOConfig-debug.cmake
├───OpenTimelineIOConfig.cmake
├───include
└───python
    ├───bin
    │   └───.bat
    └───opentimelineio
        ├───.dll
        ├───.lib
        ├───.pyd
        └───.py etc.

No lib folder is created.
The bin folder is inside the python folder.
What about the .lib files? Are they supposed to be copied at all? They are usually build artifacts that are needed for linking, although I'm not sure if they could be hosted for others to use them in static linking similar to .a on Linux.
Looking at your file tree, is it correct that the DLLs should be placed in both, the bin and the python folders?
Where does your python\OpenTimelineIOConfig.cmake file come from? I only got four CMake files at the top level.

meshula · 2021-02-12T01:25:26Z

The lib files are for linking with C++, not python, so that's why the the /lib folder is out at the root.

The bin doesn't work without the python so having it where you put it makes sense to me.

So with lib hoisted out, I'm cool with that layout.

meshula · 2021-02-15T23:59:23Z

@Simran-B Have you filed a CLA for OTIO?

Simran-B · 2021-02-16T18:42:06Z

@meshula Yes to CLA, #808 (comment)

I'm still confused regarding the folder structure. Is the lib folder intended for .lib files only? Or .lib and .dll / .so?
Is the cxx-libs folder from one of the other configurations any different?
If the .lib files are supposed to go into their own folder, then another variable should be introduced specifically for that path.
Or am I missing something here? Is there special handling in another CMake file already?

meshula · 2021-02-16T20:56:58Z

The cxx-libs is intended as a C++ SDK. It's not intended as a run time environment.

There's a bin inside site-packages/opentimelineio which is where runtime stuff also lands, including copies of the dlls/dylibs, as sisters to the python bits.

With regards to the structure of the installation, there is also this new issue, related: (#885)

This PR is morphing into something different than the original intent :) Perhaps the best thing to do is to separate installation structure, from message syntax fixes. The message fix is perfectly fine and ready to land, so I suggest we can get that landed as step one, and as step two, tidy up and finalize the build structure and the variables to control it.

meshula · 2021-02-16T23:00:08Z

this PR should resolve the structure:

#886

Simran-B · 2021-02-17T17:26:12Z

This PR is morphing into something different than the original intent

Good point, I reverted the change to the comment about install paths, so that it includes the message() type fix and some whitespace/capitalization improvements only.

ssteinbach

lgtm

CMakeLists.txt

Simran-B commented Feb 8, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Simran-B force-pushed the cmake-typos-2021-02-09 branch 2 times, most recently from 92beeec to ebd8b62 Compare February 9, 2021 08:11

meshula self-assigned this Feb 9, 2021

Simran-B commented Feb 9, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

meshula approved these changes Feb 10, 2021

View reviewed changes

Simran-B force-pushed the cmake-typos-2021-02-09 branch from 8750e40 to a9ea8cd Compare February 10, 2021 23:20

meshula approved these changes Feb 10, 2021

View reviewed changes

Simran-B commented Feb 10, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Simran-B commented Feb 11, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Simran-B added 4 commits February 17, 2021 18:19

CMake: Fix message() types and a set()

481e37b

Use message(STATUS ...), minor cleanup

c3592d0

Minor tweak to message text

f18a941

Undo change to comment about install paths

a34b5e4

Simran-B force-pushed the cmake-typos-2021-02-09 branch from 9eb5f50 to a34b5e4 Compare February 17, 2021 17:23

Simran-B changed the title ~~CMake: Fix message() types and a set()~~ CMake: Fix message() types Feb 17, 2021

ssteinbach approved these changes Feb 17, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt

4bbb97c

ssteinbach approved these changes Feb 17, 2021

View reviewed changes

Merge branch 'master' into cmake-typos-2021-02-09

0008d11

ssteinbach merged commit a7a5db0 into AcademySoftwareFoundation:master Feb 17, 2021

Simran-B deleted the cmake-typos-2021-02-09 branch February 17, 2021 23:20

ssteinbach added this to the Public Beta 14 milestone Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: Fix message() types #879

CMake: Fix message() types #879

Simran-B commented Feb 8, 2021 •

edited

meshula commented Feb 9, 2021

Simran-B commented Feb 9, 2021 •

edited

meshula commented Feb 9, 2021

meshula commented Feb 9, 2021

meshula left a comment

codecov-io commented Feb 10, 2021 •

edited

meshula commented Feb 11, 2021

Simran-B commented Feb 11, 2021

meshula commented Feb 12, 2021

meshula commented Feb 15, 2021

Simran-B commented Feb 16, 2021

meshula commented Feb 16, 2021

meshula commented Feb 16, 2021

Simran-B commented Feb 17, 2021

ssteinbach left a comment

CMake: Fix message() types #879

CMake: Fix message() types #879

Conversation

Simran-B commented Feb 8, 2021 • edited

meshula commented Feb 9, 2021

Simran-B commented Feb 9, 2021 • edited

meshula commented Feb 9, 2021

meshula commented Feb 9, 2021

meshula left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 10, 2021 • edited

Codecov Report

meshula commented Feb 11, 2021

Simran-B commented Feb 11, 2021

meshula commented Feb 12, 2021

meshula commented Feb 15, 2021

Simran-B commented Feb 16, 2021

meshula commented Feb 16, 2021

meshula commented Feb 16, 2021

Simran-B commented Feb 17, 2021

ssteinbach left a comment

Choose a reason for hiding this comment

Simran-B commented Feb 8, 2021 •

edited

Simran-B commented Feb 9, 2021 •

edited

codecov-io commented Feb 10, 2021 •

edited