Skip to content

Add version header. #1907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 14, 2025
Merged

Conversation

peter-targett
Copy link
Contributor

Add a version header file for C++ installs.

Allows 3rd-party apps that use OTIO to reference the version.

Signed-off-by: Peter Targett <[email protected]>
Signed-off-by: Peter Targett <[email protected]>
@darbyjohnston
Copy link
Contributor

Hi, it looks like there are already version.h files in the opentime and opentimelineio directories, we should probably change those to use the CMake variables instead of adding a new file. The version is currently set to v1_0, but I can't imagine anyone is actually checking that:

#define OPENTIMELINEIO_VERSION v1_0

Also sorry, it looks like one of the CI jobs is failing unrelated to your changes, I'll look into that.

@darbyjohnston
Copy link
Contributor

Just as an example, I was looking at how OpenEXR does their version file and they have the version numbers in the header and parse those with CMake:

file(READ "src/lib/OpenEXRCore/openexr_version.h" VERSION_H)
string(REGEX MATCH "VERSION_MAJOR ([0-9]*)" _ ${VERSION_H})
set(OPENEXR_VERSION_MAJOR ${CMAKE_MATCH_1})
string(REGEX MATCH "VERSION_MINOR ([0-9]*)" _ ${VERSION_H})
set(OPENEXR_VERSION_MINOR ${CMAKE_MATCH_1})
string(REGEX MATCH "VERSION_PATCH ([0-9]*)" _ ${VERSION_H})
set(OPENEXR_VERSION_PATCH ${CMAKE_MATCH_1})

I wonder if there are any advantages to this vs. using configure_file to generate the file.

@darbyjohnston darbyjohnston added enhancement A request for something new. and removed time calculations opentime labels Jul 9, 2025
@peter-targett
Copy link
Contributor Author

I wonder if there are any advantages to this vs. using configure_file to generate the file.

I guess it's just another way to end up with the same result, I don't see any obvious benefit?

@darbyjohnston
Copy link
Contributor

It looks like the Python builds are failing on the CI:

/home/runner/work/OpenTimelineIO/OpenTimelineIO/src/opentime/errorStatus.h:6:10: fatal error: opentime/version.h: No such file or directory
      6 | #include "opentime/version.h"
        |          ^~~~~~~~~~~~~~~~~~~~

Fix python builds.

Signed-off-by: Peter Targett <[email protected]>
@meshula
Copy link
Collaborator

meshula commented Jul 19, 2025

I was looking at how OpenEXR does their version file

We did have a more traditional build-system-built version file before. The reason I switched to that system was that I got tired of debugging all the ways people build OpenEXR and manage to make the version header unavailable, the permutations are endless when people start bringing all kinds of different build systems to the table. OpenEXR is built by more than just cmake, and consumed by package managers that all do their own interesting things with the version header file :] I figured if the version.h file was ground truth, then it would always just work and I wouldn't have to deal with GitHub issues where this week vcpkg is broken, next week the Debian distro has a twist, etc.

OTIO doesn't have that build permutation complexity but it does have python packaging to contend with.

I don't have a preference on how to do it, but since @darbyjohnston mentioned the OpenEXR set up, I thought some background is in order.

@darbyjohnston
Copy link
Contributor

I hope we don't have to support any other build systems, but I would like to support packaging like Debian or vcpkg. Do you think the CMake generated version.h file will cause problems for those?

@JeanChristopheMorinPerso
Copy link
Member

It should not cause any issues. Both will run cmake behind the scene.

@darbyjohnston
Copy link
Contributor

Thanks for confirming that, @JeanChristopheMorinPerso

Signed-off-by: Peter Targett <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.86%. Comparing base (c0e97b0) to head (563f621).
⚠️ Report is 62 commits behind head on main.

❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1907      +/-   ##
==========================================
+ Coverage   84.11%   84.86%   +0.74%     
==========================================
  Files         198      181      -17     
  Lines       22241    13054    -9187     
  Branches     4687     1206    -3481     
==========================================
- Hits        18709    11078    -7631     
+ Misses       2610     1790     -820     
+ Partials      922      186     -736     
Flag Coverage Δ
py-unittests 84.86% <ø> (+0.74%) ⬆️

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

Files with missing lines Coverage Δ
...py-opentimelineio/opentimelineio/adapters/otioz.py 84.61% <ø> (-0.39%) ⬇️

... and 136 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@darbyjohnston darbyjohnston merged commit 639f57c into AcademySoftwareFoundation:main Aug 14, 2025
47 checks passed
@darbyjohnston darbyjohnston added this to the Public Beta 18 milestone Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for something new.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants