Address build-time warnings

[jminor]

We aim to meet the OpenSSF Best Practices passing or higher badge level. One of the requirements is to address compiler warnings in the project's source code.

I don't see any warnings in our GitHub Action build logs - is that because there are none, or because they are suppressed/hidden?

Details from OpenSSF Best Practices:

The project MUST address warnings. [warnings_fixed] Hide details These are the warnings identified by the implementation of the warnings criterion. The project should fix warnings or mark them in the source code as false positives. Ideally there would be no warnings, but a project MAY accept some warnings (typically less than 1 warning per 100 lines or less than 10 warnings).

[darbyjohnston]

I fixed a bunch of them with this PR awhile back: https://github.com/AcademySoftwareFoundation/OpenTimelineIO/pull/577

There are a couple of new warnings but they should be easy to fix.

[jminor]

Do you see them in local builds, but not in the CI, or am I just looking in the wrong place (likely)?

[darbyjohnston]

There are a couple in the Windows cpp_build: https://github.com/AcademySoftwareFoundation/OpenTimelineIO/actions/runs/3101844256/jobs/5023591626

An unreferenced variable and some type conversions.

[JeanChristopheMorinPerso]

I'm wondering if someone built an action that parses the build logs and publishes the warnings either as a comment on PR or as build summaries. How do projects track warnings? Do they simply force warnings to be errors?

[jminor]

They probably just treat warnings as errors, that way the developer can check locally before the CI runs.

Thanks for the pointer @darbyjohnston - those look easy to fix. I wonder why they only show up on Windows?

[darbyjohnston]

Different concepts of warnings I guess? That's one of the nice things about building on multiple platforms, each compiler seems to turn up additional warnings. I tried turning on a couple of different GCC flags like "-Wall", "-Wextra", and "-Wunused", but that still didn't show the warnings that MSVC was reporting. You can easily trigger a warning with GCC by adding an unused variable like "int j;":

[ 12%] Building CXX object src/opentime/CMakeFiles/opentime.dir/errorStatus.cpp.o
[ 14%] Building CXX object src/opentime/CMakeFiles/opentime.dir/rationalTime.cpp.o
/home/darby/dev/otio/OpenTimelineIO/src/opentime/rationalTime.cpp: In static member function ‘static opentime::v1_0::RationalTime opentime::v1_0::RationalTime::from_timecode(const string&, double, opentime::v1_0::ErrorStatus*)’:
/home/darby/dev/otio/OpenTimelineIO/src/opentime/rationalTime.cpp:198:9: warning: unused variable ‘j’ [-Wunused-variable]
  198 |     int j;

[ssteinbach]

Setting warnings as errors would be the straightforward thing to do. Do you want to put that in the issue?

[meshula]

clang 10 and up are as finicky as msvc, be prepared :)

@ssteinbach ~ it's not advisable, I think, to set -Werror unless it is an option aimed specifically at CI but otherwise off by default.

Some projects, such as [one of the well known vfxplatform projects, not pointing fingers], do set -Werror, and the outcome is that compiler point revisions that aren't tested for in CI can cause problems. It becomes especially egregious for people maintaining off the beaten path linux distributions, or xcode betas, and they wind up writing local build patches.

[ssteinbach]

Is it just a policy that we try and squash warnings we see at build time in CI before a release? Take a softer stance that is more policy based in that case?

[meshula]

I would fully support a "no-warnings in CI is a prerequisite to merge" policy, which could be along the lines of adding -Werror as an option for local testing and CI to automate that aspect. I'm just advocating for not having -Werror leak out into the wild as a default option.