[PATCH] D78762: build: use `find_package(Python3)` if available

Nico Weber via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 29 13:28:14 PDT 2020


thakis added a comment.

I believe this breaks check-builtins on macOS. See https://bugs.chromium.org/p/chromium/issues/detail?id=1076480 for details.

The `split()` on the subprocess output throws since it wants a b'.' on py3, but that code has an unqualified except block. That means (I think) we always believe we run on macOS 10.0.0 under py3. I don't understand how we then fail to convert that back to an int (see linked bug).

Anyhoo, please take a look, and if it takes a while to fix we might have to go through yet another reland cycled :/

In D78762#2006872 <https://reviews.llvm.org/D78762#2006872>, @compnerd wrote:

> I was trying to do the minimal change to cover llvm.  I definitely care about LLD and will do that in a subsequent change.
>
> As to the benefit of this, it is primarily that the detection here explicitly ensures that python3 is used rather than python2.  The fallback of aliasing is the easiest way to achieve this without having a lot of complicated logic at each site:
>
>   if(NOT Python3_Interpreter_FOUND)
>     add_custom_command(...)
>   else()
>     add_custom_command(...)
>   endif()


I would've expected we'd just set some new variable, eg LLVM_PYTHON_INTERPRETER to either the py2 or py3 interpreter and use that everywhere. Seems a bit less confusing, and shouldn't make anything else more complicated. But it's all going away in a few months anyways, so I don't have a strong opinion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78762/new/

https://reviews.llvm.org/D78762





More information about the llvm-commits mailing list