build: add flag to treat warnings as errors and enable in CI (#61)

* build: add flag to treat warnings as errors and enable in CI This PR adds an option to our CMake files that allow treating compiler warnings as errors in the targets from this repository and enables that option in CI. This will enforce that we fix compiler warnings before merging PRs, which should help to keep our code clean. Also fix a final warning. Signed-off-by: Ingo Müller <[email protected]>
substrait-io · Jan 29, 2025 · 39864c6 · 39864c6
1 parent 1387501
commit 39864c6
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 1 deletion.
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
@@ -85,6 +85,7 @@ jobs:
           -DMLIR_INCLUDE_INTEGRATION_TESTS=ON \
           -DMLIR_ENABLE_BINDINGS_PYTHON=ON \
           -DMLIR_ENABLE_PYTHON_BENCHMARKS=ON \
+          -DSUBSTRAIT_MLIR_COMPILE_WARNING_AS_ERROR=ON \
           -S${SUBSTRAIT_MLIR_MAIN_SRC_DIR}/third_party/llvm-project/llvm \
           -B${SUBSTRAIT_MLIR_MAIN_BINARY_DIR} -G Ninja
         echo "PYTHONPATH=${PYTHONPATH}:${SUBSTRAIT_MLIR_MAIN_BINARY_DIR}/tools/substrait_mlir/python_packages" | tee -a $GITHUB_ENV

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -5,6 +5,14 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
     "-DLLVM_EXTERNAL_SUBSTRAIT_MLIR_SOURCE_DIR=${CMAKE_CURRENT_SOURCE_DIR}")
 endif()
 
+################################################################################
+# CMake options specific to this sub-project
+################################################################################
+
+option(SUBSTRAIT_MLIR_COMPILE_WARNING_AS_ERROR
+       "value for 'CMAKE_COMPILE_WARNING_AS_ERROR' for Substrait MLIR targets"
+       OFF)
+
 ################################################################################
 # Set up dependencies
 ################################################################################
@@ -69,6 +77,9 @@ add_subdirectory(third_party)
 ################################################################################
 add_custom_target(substrait-mlir-all)
 
+# Set default value for COMPILE_WARNING_AS_ERROR for our targets.
+set(CMAKE_COMPILE_WARNING_AS_ERROR ${SUBSTRAIT_MLIR_COMPILE_WARNING_AS_ERROR})
+
 add_subdirectory(lib)
 add_subdirectory(include)
 add_subdirectory(python)

diff --git a/README.md b/README.md
@@ -285,6 +285,7 @@ cmake \
   -DMLIR_INCLUDE_INTEGRATION_TESTS=ON \
   -DMLIR_ENABLE_BINDINGS_PYTHON=ON \
   -DMLIR_ENABLE_PYTHON_BENCHMARKS=ON \
+  -DSUBSTRAIT_MLIR_COMPILE_WARNING_AS_ERROR=ON \
   -S${SUBSTRAIT_MLIR_SOURCE_DIR}/third_party/llvm-project/llvm \
   -B${SUBSTRAIT_MLIR_BUILD_DIR} \
   -G Ninja

diff --git a/lib/Target/SubstraitPB/Export.cpp b/lib/Target/SubstraitPB/Export.cpp
@@ -689,7 +689,7 @@ SubstraitExporter::exportOperation(LiteralOp op) {
   }
   // `TimeType`.
   else if (auto timeType = dyn_cast<TimeType>(literalType)) {
-    literal->set_time(value.cast<TimeAttr>().getValue());
+    literal->set_time(mlir::cast<TimeAttr>(value).getValue());
   } else
     op->emitOpError("has unsupported value");