-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use PyConfig to initialize python #10342
Conversation
* Isolate #10340 into it's own PR * remove blocks on python version (use PyConfig for 3.8 too), * Put back type_traits * Improve error messages via a fmt formatter for PyStatus * Replace C style casts with static_casts
… even if not LINK_WITH_PYTHON)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good stuff. Thanks for bringing it over into its own dedicated PR, that's much cleaner. As long as CI and local testing is happy, this shall merge.
} else { | ||
// PyConfig_SetBytesString takes a `const char * str` and decodes str using Py_DecodeLocale() and set the result into *config_str | ||
// But we want to avoid doing it three times, so we PyDecodeLocale manually | ||
// Py_DecodeLocale can be called because Python has been PreInitialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
#if LINK_WITH_PYTHON | ||
void initPython(EnergyPlusData &state, fs::path const &pathToPythonPackages) | ||
{ | ||
PyStatus status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wish I had been able to use the PyConfig method a long time ago, but at the time I think I was still using 3.7 or earlier. This is so much nicer nowadays.
@@ -379,6 +411,82 @@ void PluginManager::setupOutputVariables([[maybe_unused]] EnergyPlusData &state) | |||
#endif | |||
} | |||
|
|||
#if LINK_WITH_PYTHON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this a standalone function is fine with me, given what it does.
} | ||
}; | ||
} // namespace fmt | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, didn't expect this, but a dedicated formatter for PyStatus is pretty nice.
@@ -174,6 +174,7 @@ endif() | |||
|
|||
# we are making *a Python 3.6 Interpreter* a required dependency, so find it here | |||
# If LINK_WITH_PYTHON, also request the Development (libs) at the same time, to ensure consistent version between interpreter and Development | |||
# and ask for at least 3.8 (for the PyConfig stuff). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be almost inclined to just require 3.8 all the time, considering 3.6 died a long time ago, and 3.8 is about to go to end-of-life next year. But this is clearly laying out the needs for our CPython uses. 👍
Everything is super happy on CI and locally. Thanks @jmarrec, merging this in! |
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.