[mlir][sparse] Fix MLIRSparseTensorEnums target for external dialects (followup to D136506)

[joshchngs]

Summary

As noted on D136506 - the current CMake doesn't work for out-of-tree MLIR projects with the new INTERFACE target added recently. Assuming this passes CI, everything should work now.

[vmurali]

Does https://reviews.llvm.org/D136590 fix it?

[joshchngs]

In D136649#3881034, @vmurali wrote:

Does https://reviews.llvm.org/D136590 fix it?

Yes, thanks for the link – I hadn't seen that commit.

Both fix the issue, the only difference is that using CMAKE_INSTALL_INCLUDEDIR causes the generated $prefix/lib/cmake/mlir/MLIRTargets.cmake to contain an absolute path. In my case it looks like:

set_target_properties(MLIRSparseTensorEnums PROPERTIES
  INTERFACE_SOURCES "/nix/store/ff20yibf1500mbqczywq8lzpajh44s6i-llvm-main/include/mlir/Dialect/SparseTensor/IR/Enums.h"
)

whereas using "include" it remains relative to the cmake directory.

set_target_properties(MLIRSparseTensorEnums PROPERTIES
  INTERFACE_SOURCES "${_IMPORT_PREFIX}/include/mlir/Dialect/SparseTensor/IR/Enums.h"
)

I don't know if either outcome is preferable, since both seem to work, but since the other is already merged it makes sense to cancel this diff. I'm not sure how to do that though?

[wrengr]

I believe D136590 should have fixed this particular issue. Let me know if that doesn't work for you

[wrengr]

In D136649#3881111, @joshchngs wrote:

but since the other is already merged it makes sense to cancel this diff. I'm not sure how to do that though?

You should be able to go to the "Add Action..." menu (like you would if accepting someone else's change) and select the "Abandon" option

[wrengr]

In D136649#3881111, @joshchngs wrote:

Both fix the issue, the only difference is that using CMAKE_INSTALL_INCLUDEDIR causes the generated $prefix/lib/cmake/mlir/MLIRTargets.cmake to contain an absolute path. [...]
whereas using "include" it remains relative to the cmake directory. [...]
I don't know if either outcome is preferable, since both seem to work

I don't know which is preferable either (I'm just a cmake novice). On the one hand, I'd guess that relative paths would be preferable since the whole problem leading to the fix in D136477 had to do with certain things being unintentionally absolute paths. But then, since this is about *installing* the library, that does seem like a situation that wants absolute paths (to wherever it's being installed).

The reason D136477 didn't have the ${CMAKE_INSTALL_INCLUDEDIR} or include part is because that was lacking in the examples at https://stackoverflow.com/a/62465051 which was the only thing I could find re fixing the previous absolute-path problem. Not sure if that post (or rather the https://crascit.com/2016/01/31/enhanced-source-file-handling-with-target_sources/ linked to from there) might shed some more light on what the preferred course of action is.

[joshchngs]

In D136649#3881279, @wrengr wrote:

In D136649#3881111, @joshchngs wrote:

Both fix the issue, the only difference is that using CMAKE_INSTALL_INCLUDEDIR causes the generated $prefix/lib/cmake/mlir/MLIRTargets.cmake to contain an absolute path. [...]
whereas using "include" it remains relative to the cmake directory. [...]
I don't know if either outcome is preferable, since both seem to work

I don't know which is preferable either (I'm just a cmake novice). On the one hand, I'd guess that relative paths would be preferable since the whole problem leading to the fix in D136477 had to do with certain things being unintentionally absolute paths. But then, since this is about *installing* the library, that does seem like a situation that wants absolute paths (to wherever it's being installed).

The reason D136477 didn't have the ${CMAKE_INSTALL_INCLUDEDIR} or include part is because that was lacking in the examples at https://stackoverflow.com/a/62465051 which was the only thing I could find re fixing the previous absolute-path problem. Not sure if that post (or rather the https://crascit.com/2016/01/31/enhanced-source-file-handling-with-target_sources/ linked to from there) might shed some more light on what the preferred course of action is.

I'm not an accomplished CMake wrangler either. The docs for install() suggest that absolute paths are not supported by cpack generators, but that seems tangential to this point. If I understand it right, either version of set_target_properties() is working by accident because the resolved path happens to match the final install path, which won't necessarily be true after distro packaging.

I'll abandon this anyway, since I'm not working on distro packaging.

mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt

 foreach(hdr ${MLIRSparseTensorEnums_hdrs})
   target_sources(MLIRSparseTensorEnums INTERFACE
     $<BUILD_INTERFACE:${MLIR_MAIN_INCLUDE_DIR}/${hdr}>
-    $<INSTALL_INTERFACE:${hdr}>)
+    $<INSTALL_INTERFACE:include/${hdr}>)
 endforeach()
 # The `add_mlir_library_install` is required for other libraries to
 # depend on this one, but the conditional itself and the phony target