[mlir] Make MLIR Python sources idempotent

[dukebw]

Make MLIR Python sources idempotent by moving the custom commands off the custom target for each source.
Previously, a custom target was created for each MLIR Python source file and a custom command added to symlink it to a destination path. However this causes the build to not be idempotent because custom targets always run.

Instead, this PR separates the symlink/copy to destination paths into custom commands and makes the custom target depend on the output. That prevents re-running them and restores idempotency.

Testing:

• Build with -D MLIR_ENABLE_BINDINGS_PYTHON=ON. Prior to this change, the build is not idempotent (building twice re-runs the symlink/copy command for Python source targets). After this change it is idempotent.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Brendan Duke (dukebw)

Changes

Make MLIR Python sources idempotent by moving the custom commands off the custom target for each source.
Previously, a custom target was created for each MLIR Python source file and a custom command added to symlink it to a destination path. However this causes the build to not be idempotent because custom targets always run.

Instead, this PR separates the symlink/copy to destination paths into custom commands and makes the custom target depend on the output. That prevents re-running them and restores idempotency.

Testing:

• Build with -D MLIR_ENABLE_BINDINGS_PYTHON=ON. Prior to this change, the build is not idempotent (building twice re-runs the symlink/copy command for Python source targets). After this change it is idempotent.

---

Full diff: https://github.com/llvm/llvm-project/pull/97167.diff

1 Files Affected:

• (modified) mlir/cmake/modules/AddMLIRPython.cmake (+11-6)

diff --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index f030798a94ae9..7b91f43e2d57f 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -565,8 +565,6 @@ function(add_mlir_python_sources_target name)
     message(FATAL_ERROR "Unhandled arguments to add_mlir_python_sources_target(${name}, ... : ${ARG_UNPARSED_ARGUMENTS}")
   endif()
 
-  add_custom_target(${name})
-
   # On Windows create_symlink requires special permissions. Use copy_if_different instead.
   if(CMAKE_HOST_WIN32)
     set(_link_or_copy copy_if_different)
@@ -575,8 +573,6 @@ function(add_mlir_python_sources_target name)
   endif()
 
   foreach(_sources_target ${ARG_SOURCES_TARGETS})
-    add_dependencies(${name} ${_sources_target})
-
     get_target_property(_src_paths ${_sources_target} SOURCES)
     if(NOT _src_paths)
       get_target_property(_src_paths ${_sources_target} INTERFACE_SOURCES)
@@ -590,6 +586,8 @@ function(add_mlir_python_sources_target name)
       get_target_property(_root_dir ${_sources_target} INTERFACE_INCLUDE_DIRECTORIES)
     endif()
 
+    # Initialize an empty list of all Python source destination paths.
+    set(all_dest_paths "")
     foreach(_src_path ${_src_paths})
       file(RELATIVE_PATH _source_relative_path "${_root_dir}" "${_src_path}")
       set(_dest_path "${ARG_OUTPUT_DIRECTORY}/${_source_relative_path}")
@@ -598,13 +596,17 @@ function(add_mlir_python_sources_target name)
       file(MAKE_DIRECTORY "${_dest_dir}")
 
       add_custom_command(
-        TARGET ${name} PRE_BUILD
+        OUTPUT "${_dest_path}"
+        PRE_BUILD
         COMMENT "Copying python source ${_src_path} -> ${_dest_path}"
         DEPENDS "${_src_path}"
-        BYPRODUCTS "${_dest_path}"
         COMMAND "${CMAKE_COMMAND}" -E ${_link_or_copy}
             "${_src_path}" "${_dest_path}"
       )
+
+      # Track the symlink or copy command output.
+      list(APPEND all_dest_paths "${_dest_path}")
+
       if(ARG_INSTALL_DIR)
         # We have to install each file individually because we need to preserve
         # the relative directory structure in the install destination.
@@ -621,6 +623,9 @@ function(add_mlir_python_sources_target name)
       endif()
     endforeach()
   endforeach()
+
+  # Create a new custom target that depends on all symlinked or copied sources.
+  add_custom_target("${name}" DEPENDS ${all_dest_paths})
 endfunction()
 
 ################################################################################

[akirchhoff-modular]

Looks good to me. I was initially confused why we were creating a list of paths to use as DEPENDS later rather than using add_dependencies as we go, but I now see in the CMake documentation that add_dependencies is only for top-level targets to depend on other top-level targets, so a list is in fact needed and this PR as-is makes sense.

[stellaraccident]

Looks good to me. I was initially confused why we were creating a list of paths to use as DEPENDS later rather than using add_dependencies as we go, but I now see in the CMake documentation that add_dependencies is only for top-level targets to depend on other top-level targets, so a list is in fact needed and this PR as-is makes sense.

Yes, cmake is very fiddly for mixed file/target dependencies.