Skip to content

Comments

Replace CMake generated version files with checked-in versions#1993

Open
darbyjohnston wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
darbyjohnston:source_version
Open

Replace CMake generated version files with checked-in versions#1993
darbyjohnston wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
darbyjohnston:source_version

Conversation

@darbyjohnston
Copy link
Contributor

In PR #1907 we added version headers so applications could identify which OpenTimelineIO they were using. However this also introduced a dependency on CMake to generate the headers, which can cause issues for other projects that use OTIO without CMake (for example the OTIO Swift bindings).

This PR replaces the CMake file generation with checked-in files, and instead has CMake parse the version from the checked-in header files. This is similar to what OpenEXR does for their version handling.

Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, minor request

#define OPENTIME_VERSION_MINOR 19
#define OPENTIME_VERSION_PATCH 0
#define OPENTIME_VERSION v0_19_0
#define OPENTIME_VERSION_NS v0_19
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a variant that leaves no room for human forgetfulness

#define OPENTIME_VERSION_MAJOR 0
#define OPENTIME_VERSION_MINOR 19
#define OPENTIME_VERSION_PATCH 0
#define OPENTIME_VERSION
v##OPENTIME_VERSION_MAJOR####OPENTIME_VERSION_MINOR####OPENTIME_VERSION_PATCH
#define OPENTIME_VERSION_NS
v##OPENTIME_VERSION_MAJOR##_##OPENTIME_VERSION_MINOR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but that doesn't seem to be working for me using VS2022. I get: vOPENTIME_VERSION_MAJOR_OPENTIME_VERSION_MINOR_OPENTIME_VERSION_PATCH.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like too many ## in a row, or missing some _ between?. Here's how that's supposed to work:
https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but I don't think the macros are expanding like expected. Running this simple example through cpp on Linux:

#define VERSION_MAJOR 1
#define VERSION_MINOR 2
#define VERSION_NS v##VERSION_MAJOR##_##VERSION_MINOR

namespace VERSION_NS {}

Results in this output:

namespace vVERSION_MAJOR_VERSION_MINOR {}

I can get it to work by using macro arguments, but I don't think the added complexity is worth it:

#define VERSION_MAJOR 1
#define VERSION_MINOR 2
#define VERSION_NS v##VERSION_MAJOR##_##VERSION_MINOR
#define VERSION_NS2(MAJOR, MINOR) v##MAJOR##_##MINOR
#define VERSION_NS3(MAJOR, MINOR) VERSION_NS2(MAJOR, MINOR)

namespace VERSION_NS {}
namespace VERSION_NS2(VERSION_MAJOR, VERSION_MINOR) {}
namespace VERSION_NS3(VERSION_MAJOR, VERSION_MINOR) {}

Results in this output:

namespace vVERSION_MAJOR_VERSION_MINOR {}
namespace vVERSION_MAJOR_VERSION_MINOR {}
namespace v1_2 {}

#define OPENTIMELINEIO_VERSION_MAJOR 0
#define OPENTIMELINEIO_VERSION_MINOR 19
#define OPENTIMELINEIO_VERSION_PATCH 0
#define OPENTIMELINEIO_VERSION v0_19_0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same DRY note as above

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.15%. Comparing base (f3cb304) to head (d3d5713).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1993   +/-   ##
=======================================
  Coverage   85.15%   85.15%           
=======================================
  Files         181      181           
  Lines       12783    12783           
  Branches     1206     1206           
=======================================
  Hits        10885    10885           
  Misses       1715     1715           
  Partials      183      183           
Flag Coverage Δ
py-unittests 85.15% <ø> (ø)

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


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 f3cb304...d3d5713. Read the comment docs.

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

@meshula
Copy link
Collaborator

meshula commented Feb 24, 2026

Sorry, I don't know how I typed 4 #'s in a row. I'm still in favor of concatenation even with a helpy macro!

@darbyjohnston
Copy link
Contributor Author

Sorry, I don't know how I typed 4 #'s in a row. I'm still in favor of concatenation even with a helpy macro!

That requires updating every source file with the new macro. That seems like a lot of work to not duplicate a number, plus it also adds complexity which I think undermines the benefits of DRY.

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.

4 participants