[Mlir-commits] [mlir] [MLIR][Python] Extend bindings for external projects without duplication (PR #173241)

[Maksim Levental]

makslevental wrote:

Thanks for the PR! I've only taken a quick look but a few comments (I'll try it out in the morning).

We already have both of these CMake config vars which can be used to place the package in an arbitrary location in a source install tree:

https://github.com/llvm/llvm-project/blob/2ebe16c58591a4e8dab5e45d9e5de59cc933f2c7/mlir/examples/standalone/CMakeLists.txt#L65-L71

So you didn't need to create the new CMake variable but you don't need it anyway because with `MLIR_BINDINGS_PYTHON_INSTALL_PREFIX` you can install the bindings package to an arbitrary location (and thus the relative paths will work).

But I don't understand the CAPI change; removing `DISABLE_INSTALL` just means that install targets will be emitted for that thing and you can link against as a CMake target in downstream/external projects? That's true but the biggest issue with that part is you haven't handled exactly the thing you said you haven't handled - the fact that the two C API libs will have duplicate symbols for stuff like the dialect registry because (by default) the C APIs *statically* link all of the `MLIRCAPI*` libs. Now that's not true if you do `BUILD_SHARED` (or whatever it's called) so your idea might work then but that's generally not how people build LLVM (and definitely not the bindings). And also if you're already building shared libs then the C API libs should basically be empty (it will just be a bunch RT links to stuff in `CMAKE_INSTALL_PREFIX`. Also by adding that install target you would break everyone who rebuilds that lib and [calls it the same thing](https://github.com/llvm/eudsl/blob/main/projects/mlir-python-bindings/CMakeLists.txt#L85). 

Finally I'll say that using the bindings from the "distro" is not how most people use them them (well at least "in production") - production use cases build pip-installable wheels. So ideally your change needs to work for that use case (or at least not affect that use case).

It would help me understand your patch if you drew my a picture of the relationships between the upstream and downstream/external that you'd like to accomplish.

https://github.com/llvm/llvm-project/pull/173241