Add TORCHCODEC_DISABLE_COMPILE_WARNING_AS_ERROR environment variable to disable -Werror compilation option

[traversaro]

The -Werror option is typically enabled automatically by library authors, so they ensure that all contributors to the library early fail if new code they write contain a warning, ensuring as soon as possible that no new warnings are added to the library. This is the reason (I guess) why -Werror was added in ttps://github.com//pull/452 . On the other hand, people that package libraries for distributions prefer to disable the -Werror option, as they may want to compile a given library with a new compilers or new ffmpeg versions (that may not have even existed when a given release of a library was tagged), that introduce new warnings, without having a failure. For a detailed discussion of this from the point of view of people packaging libraries, see https://youtu.be/_5weX5mx8hc?si=ZtiWaK7KPTfQ01_g&t=322 .

For example, torchcodec 0.3.0 currently fails with error:

 │ │   [ 38%] Building CXX object src/torchcodec/_core/CMakeFiles/libtorchcodec_decoder7.dir/Encoder.cpp.o
 │ │   cd $SRC_DIR/build/temp.linux-x86_64-cpython-310/src/torchcodec/_core && $BUILD_PREFIX/bin/x86_64-conda-linux-gnu-
 │ │ c++ -DPROTOBUF_USE_DLLS -DUSE_C10D_GLOO -DUSE_C10D_NCCL -DUSE_DISTRIBUTED -DUSE_RPC -DUSE_TENSORPIPE -Dlibtorchcode
 │ │ c_decoder7_EXPORTS -I$SRC_DIR/src/torchcodec/_core/./../../.. -I$PREFIX/include/python3.10 -isystem $PREFIX/lib/pyt
 │ │ hon3.10/site-packages/torch/include -isystem $PREFIX/lib/python3.10/site-packages/torch/include/torch/csrc/api/incl
 │ │ ude -fvisibility-inlines-hidden -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-prot
 │ │ ector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local
 │ │ /src/conda/torchcodec-0.3.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix  -I$PREFIX/targets/x86_64-linux/
 │ │ include -I$BUILD_PREFIX/targets/x86_64-linux/include -Wall -Wextra -pedantic -Werror -D_GLIBCXX_USE_CXX11_ABI=1 -O3
 │ │  -DNDEBUG -std=gnu++17 -fPIC -fdiagnostics-color=always -D_GLIBCXX_USE_CXX11_ABI=1 -MD -MT src/torchcodec/_core/CMa
 │ │ keFiles/libtorchcodec_decoder7.dir/Encoder.cpp.o -MF CMakeFiles/libtorchcodec_decoder7.dir/Encoder.cpp.o.d -o CMake
 │ │ Files/libtorchcodec_decoder7.dir/Encoder.cpp.o -c $SRC_DIR/src/torchcodec/_core/Encoder.cpp
 │ │   $SRC_DIR/src/torchcodec/_core/Encoder.cpp: In function 'void facebook::torchcodec::{anonymous}::validateSampleRat
 │ │ e(const AVCodec&, int)':
 │ │   $SRC_DIR/src/torchcodec/_core/Encoder.cpp:11:15: error: 'AVCodec::supported_samplerates' is deprecated [-Werror=d
 │ │ eprecated-declarations]
 │ │      11 |   if (avCodec.supported_samplerates == nullptr) {
 │ │         |               ^~~~~~~~~~~~~~~~~~~~~
 │ │   In file included from $PREFIX/include/libavcodec/avcodec.h:41,
 │ │                    from $SRC_DIR/src/torchcodec/_core/./../../../src/torchcodec/_core/FFMPEGCommon.h:14,
 │ │                    from $SRC_DIR/src/torchcodec/_core/./../../../src/torchcodec/_core/Encoder.h:3,
 │ │                    from $SRC_DIR/src/torchcodec/_core/Encoder.cpp:3:
 │ │   $PREFIX/include/libavcodec/codec.h:217:16: note: declared here
 │ │     217 |     const int *supported_samplerates;       ///< @deprecated use avcodec_get_supported_config()
 │ │         |                ^~~~~~~~~~~~~~~~~~~~~
 │ │   $SRC_DIR/src/torchcodec/_core/Encoder.cpp:11:15: error: 'AVCodec::supported_samplerates' is deprecated [-Werror=d
 │ │ eprecated-declarations]
 │ │      11 |   if (avCodec.supported_samplerates == nullptr) {
 │ │         |               ^~~~~~~~~~~~~~~~~~~~~
 │ │   $PREFIX/include/libavcodec/codec.h:217:16: note: declared here
 │ │     217 |     const int *supported_samplerates;       ///< @deprecated use avcodec_get_supported_config()
 │ │         |                ^~~~~~~~~~~~~~~~~~~~~
 │ │   $SRC_DIR/src/torchcodec/_core/Encoder.cpp:11:15: error: 'AVCodec::supported_samplerates' is deprecated [-Werror=d
 │ │ eprecated-declarations]
 │ │      11 |   if (avCodec.supported_samplerates == nullptr) {
 │ │         |               ^~~~~~~~~~~~~~~~~~~~~
 │ │   $PREFIX/include/libavcodec/codec.h:217:16: note: declared here
 │ │     217 |     const int *supported_samplerates;       ///< @deprecated use avcodec_get_supported_config()
 │ │         |                ^~~~~~~~~~~~~~~~~~~~~
 │ │   $SRC_DIR/src/torchcodec/_core/Encoder.cpp:15:28: error: 'AVCodec::supported_samplerates' is deprecated [-Werror=d
 │ │ eprecated-declarations]
 │ │      15 |   for (auto i = 0; avCodec.supported_samplerates[i] != 0; ++i) {
 │ │         |                            ^~~~~~~~~~~~~~~~~~~~~

when compiled against ffmpeg >=7.1.0, as the supported_samplerates deprecation was added in 7.1.0 .

This PR leaves the default behavior as it is, but adds a TORCHCODEC_DISABLE_COMPILE_WARNING_AS_ERROR environment variable (and corresponding CMake variable) to permit to disable the -Werror option.

CMake provides the built-in CMake variable CMAKE_COMPILE_WARNING_AS_ERROR since CMake 3.24, but as the CMake minimum required version is 3.18, using a custom CMake option seems the solution with minimal complexity.

Similar to PickNikRobotics/RSL#117 and ros-misc-utilities/ffmpeg_encoder_decoder#2 .

[NicolasHug]

Thanks for the PR @traversaro . That's fine with me. @scotts , any concern? Do you know if there's a more obvious way of achieving this?

[NicolasHug]

CMake provides the built-in CMake variable CMAKE_COMPILE_WARNING_AS_ERROR since CMake 3.24, but as the CMake minimum required version is 3.18, using a custom CMake option seems the solution with minimal complexity.

Oh, I missed this sorry. @traversaro , can you try to update the minimum version to 3.18 and use CMAKE_COMPILE_WARNING_AS_ERROR? If our CI is green then I think we can go with it.

[NicolasHug]

Also I noticed you were working on building torchcodec for conda-forge - thank you for this!!

[scotts]

Yeah, I'm good with this in principle. If we can use CMake features to achieve this instead of environment variables, that would be good too.

[traversaro]

Also I noticed you were working on building torchcodec for conda-forge - thank you for this!!

Thanks for working on the torchcodec on the first place, and thanks for making it a small standalone package, that drastically simplifies the packaging.

@traversaro , can you try to update the minimum version to 3.18 and use CMAKE_COMPILE_WARNING_AS_ERROR? If our CI is green then I think we can go with it.

Sorry for the confusion, the minimum version is now 3.18, and we need to increase it to 3.24 to use CMAKE_COMPILE_WARNING_AS_ERROR. 3.24 was release on August 2022 (see https://www.kitware.com/cmake-3-24-0-is-available-for-download). The main downside of doing that is that the CMake project will not work out of the box with the CMake available via apt on Ubuntu 22.04 (see https://repology.org/project/cmake/versions).

If we can use CMake features to achieve this instead of environment variables, that would be good too.

Unfortunately as far as I know there is no CMake environment variable that controls the CMAKE_COMPILE_WARNING_AS_ERROR variable or the --compile-no-warning-as-error command line option. So anyhow we need some kind of environment variables (or other mechanism, I just used the env variable mechanism as it was already used in the setup.py) are required to permit to se the CMake options when torchcodec is built from pip.

To avoid the need to add a specific environment variable for each option that may be propagated from pip invocation to CMake invocation, most CMake-based PEP517 build backends tipically expose an environment variable to specify arbitrary CMake command line options, see for example:

• SKBUILD_CONFIGURE_OPTIONS or CMAKE_ARGS for scikit-build (https://scikit-build.readthedocs.io/en/latest/usage.html#environment-variable-configuration)
• SKBUILD_CMAKE_ARGS or CMAKE_ARGS for scikit-build-core (https://scikit-build-core.readthedocs.io/en/latest/configuration/index.html#configuring-cmake-arguments-and-defines)

A possible option is to support some similar env variable also in torchcodec.

[NicolasHug]

Thanks for the details @traversaro , I understand now that we'll need a new env variable regardless of the CMAKE_COMPILE_WARNING_AS_ERROR option. So the PR LGTM as-is.

Good point on a potential catch-all CMAKE option env var, we can add it in the future if we end up having the parametrize the cmake build a lot.