Use consistent Python in MacOS GitHub runner#5331
Conversation
|
Looks good. Mac resolved. |
| add_subdirectory(contrib) | ||
| if(ENABLE_PYTHON_SCRIPTING_RESULT) | ||
| add_subdirectory(pycontrib) | ||
| write_file("CMake_Python3_EXECUTABLE" "${Python3_EXECUTABLE}") |
There was a problem hiding this comment.
Oh, sorry, the comment is great but when I say "can you say more about" in the context of these PR reviews, I just mean to me, in these messages. I'll say something about a "comment" when I think that's needed.
What I'm wondering about here is the context and the scope. Since we're changing the top-level CMakeLists in a way not protected by "APPLE" we need to make sure this doesn't have any unfortunate implications in other contexts.
So:
- The CMake documentation seems to think
write_fileis deprecated since version 3, so we should probably change this to afile(WRITEsemantic - Given that we only use this file in the
macgithub workflow (as far as I can tell) we should probably have this in aif(APPLE)check in the CMakeLists.txt, right?
Other than that, I see no big harm in adding a vaguely mysterious build artifact. I can't see how it would get installed or otherwise confuse things.
There was a problem hiding this comment.
Ah, I see... Well, the more comments the better anyway
Regarding your specific reservations:
- I tried
file(WRITE, and it just didn't work. It produced no file nowhere, and I didn't bother to research it in depth. - As a general rule I prefer to expose such hacks, so that they serve as a sort of canary. If this file causes issues on other platform, it likely means it's not good on Mac either, but it's bad influence is somehow obscured. So I prefer to create it unconditionally, and if there are problems, maybe think of a different and cleaner solution. I understand that it's mostly my personal taste, and can confine it to
APPLEif you still prefer that.
|
OK, I made my picky little changes and it seems to work fine, so I'm merging this. |
🤷♂️ Magic... Thanks! |
MacOS 11 GitHub runner has multiple instances of Python 3.12 installed, with different subversions. We shall use the instance found by CMake for all tests, including smoke.