The "[mlir] Leverage CMake interface libraries for mlir python" breaks build with cmake 3.15

Our buildbot happens to use a relatively old environment – ubuntu 18.04 and particularly cmake 3.15.1. Revision ac521d9ecde70bc8ad11d9e605b43658014fb5af, “[mlir] Leverage CMake interface libraries for mlir python,” causes the build to fail in that environment, in the cmake-configure stage, first with

CMake Warning at /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/cmake/modules/MLIRDetectPythonEnv.cmake:18 (message):
  This version of CMake is not compatible with statically built Python
  installations.  If Python fails to detect below this may apply to you.
  Recommend upgrading to at least CMake 3.18.  Detected current version:
  3.15.1
Call Stack (most recent call first):
  /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/CMakeLists.txt:139 (mlir_configure_python_dev_packages)

and then a whole mess of

CMake Error at /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/cmake/modules/AddMLIRPython.cmake:74 (set_property):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "INCLUDE_DIRECTORIES" is not allowed.
Call Stack (most recent call first):
  /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/python/CMakeLists.txt:7 (declare_mlir_python_sources)

and similar errors.

I can fix the problem by updating to the latest cmake (3.23, I think); I haven’t tried to determine the minimal update. But the cmake_minimum_required in mlir/CMakeLists.txt is 3.13.4.

Should the minimum-required be updated? Should the patch be retrofitted? Is this worth making an issue for? (I looked in the llvm github issues under mlir, mlir:core, and mlir:llvm and didn’t see one like it.)

Thanks.

@sstamenova

My personal opinion is that we diverged a long time ago from the Python bindings for MLIR being fully compatible with ancient CMake and Python versions. As an optional feature that targets much newer software (i.e. the version of Python that ships with Ubuntu 18.04 is 3.6.x, which is EOL and not supported anyway), I don’t think anyone supporting this has a lot of time to be attempting to retrofit for software versions that old.

With that said, I have run into this issue with CMake before and missed it in the review (it is really hard to keep the version compatibility gotchas of this stuff completely in mind). It may be correctable, but I think your call stack may be missing some lines that let me see where this is happening (old CMakes throw this error when both attempting to set such a property and when attempting to read – and the latter can be worked around/is usually incidental).

Sorry this broke you: If it were me and I was just before a long holiday weekend, I would update CMake. However, I think the developer policy would also support a rollback.

I can look into this after the long weekend (downgrade cmake locally, try to reproduce, etc.), but it won’t be for a few days, so I agree that the quickest solution would be to upgrade cmake on the buildbot.

@pcf : Could you share more of the log in the meantime or a link to the bot?

That was really the whole backtrace logged for the error, but there are a few hundred others in the log, and some have two lines and some have three. The first failing run is Buildbot and the bot is Buildbot . (These URLs worked for me. If necessary, go to the staging server and look for mlir-rocm-mi200, tagged “mlir”.)

I had gone ahead and updated my cmake, figuring it was an “us” problem, but it was suggested to me that it was an upstream issue. I’m using python 3.8 because the buildbot software requires it, but Ubuntu 18 because our base image requires it and the goal was to catch us-specific problems. It’s a bit of a mess.

It might just be time to raise the minimum cmake version to 3.18 or whatever works.

The last time bumping cmake was discussed, there were a lot of opinions. I am strongly in the category of “never get too behind on cmake versions and know how to upgrade”, but I don’t believe there is consensus on that point.

We ran into this one again, and in both instances, were able to bump CMake to 3.23. I don’t know if we’ve taken the time to narrow down exactly which version is the minimum now, but we also saw the errors about INCLUDE_DIRECTORIES, most recently on CMake 3.18.2:

CMake Error at .../circt/llvm/mlir/cmake/modules/AddMLIRPython.cmake:74 (set_property):
   INTERFACE_LIBRARY targets may only have whitelisted properties.  The
   property "INCLUDE_DIRECTORIES" is not allowed.
Call Stack (most recent call first):
   .../circt/llvm/mlir/python/CMakeLists.txt:7 (declare_mlir_python_sources)

Should we make this check/message more precise?

I am open to suggestions/help! It seems like we have, at a minimum, a presubmit problem: I am pretty sure we are testing with an arbitrarily different CMake version.

1 Like

I know that 3.19 works, but 3.18.3 doesn’t, so I think it’s safe to say that 3.19 is likely the minimum required (in reality it could be something in between, but I’m not sure that such granularity matters).

I sent this for review a while back and this would backport the feature to 3.15 ( :gear: D129163 [mlir] Support cmake 3.15+ in the python mlir configuration (llvm.org)) but I agree with Stella that it is not a particularly elegant solution and it would be better if we were able to avoid it.

I was wondering whether there’s a reason not to require 3.19 for the cmake version for mlir itself - not all of llvm, but just the mlir project. It already requires at least 3.15 which is higher than the minimum llvm version, so is there a reason not to bump it to 3.19?

Thanks, I haven’t bisected that yet, but I can definitely send a PR to update that.

Also, we saw some issues in CIRCT that seem related to the symptom fixed in ⚙ D129434 Restore Python install behavior from before D128230.. We’ve included that fix, but are still seeing a wonky install directory. I’m looking into that today. Not sure yet if CIRCT is behaving strangely, or if there is another patch needed to MLIR’s Python CMake stuff, but I will send one if necessary.

I’m not sure the desires of the broader MLIR community here, but perhaps we can bump it. For now, it seems sufficient to have the safeguard for MLIR Python, which only checks the CMake version if the Python bindings are enabled.

Here’s a patch to update that check to 3.19: ⚙ D130171 [mlir] Update Python CMake version requirement..

I was wrong, I was looking at an install directory from before the patch that fixed this. No issue here.