Updated GCC/Python Support #10340

Myoldmopar · 2023-12-21T20:45:42Z

On a new machine running Ubuntu 23.10, I encountered several issues. Part of them due to the update to GCC 13.2, part due to updating to Python 3.11/3.12, and part due to updated Clang Format. I'll provide a walkthrough of the changes, and hopefully they can be massaged into a nice set of changes that work for both our release versions as well as my dev machine.

….2/Python 3.11, definitely not ready as-is

Myoldmopar

OK, walkthrough done, I already noticed a couple things I need to just take out from debugging steps. I would love some thoughts on this from @jmarrec at least, and anyone else interested. My goal is to avoid affecting anyone else's build at all, while also keeping the config/build/stuff as simple as possible.

Myoldmopar · 2023-12-21T20:46:01Z

cmake/CompilerFlags.cmake

+  target_compile_options(project_warnings INTERFACE -Wpedantic)
+  # Turn on warnings about constructs/situations that may be non-portable or outside of the standard
+  target_compile_options(project_warnings INTERFACE -Wall) # Turn on warnings
+  target_compile_options(project_warnings INTERFACE -Wextra) # Turn on warnings


No functional change, just cleaning up these lines

Myoldmopar · 2023-12-21T20:48:03Z

cmake/CompilerFlags.cmake

+    # https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=6b927b1297e66e26e62e722bf15c921dcbbd25b9
+    target_compile_options(project_warnings INTERFACE -Wno-dangling-reference)
+    target_compile_options(project_warnings INTERFACE -Wno-array-bounds)
+    target_compile_options(project_warnings INTERFACE -Wno-stringop-overflow)


Alright, so with GCC 13.2, I am getting lots of what appear to be false-positives. I'm not the only one seeing the warnings unexpectedly based on internet searches. Before this merges, I would like this to be investigated further, and:

understand why the errors are being emitted, are they actually false positives?

be more surgical with the warning mutes, if they will stay at any level

Define many/lots please? Obviously we're not fans of plain silencing a whole section of warnings, so just trying to gauge if feasible to pragma ignore some of them.

And yes, totally agreed that we should investigate if they are false positive.

(At minimum, I'd say this should be a conditional ignore)

Myoldmopar · 2023-12-21T20:48:13Z

src/ConvertInputFormat/main.cpp

@@ -88,7 +88,7 @@ static constexpr std::array<std::string_view, static_cast<int>(OutputTypes::Num)

 static constexpr auto outputTypeExperimentalStart = OutputTypes::CBOR;

-template <typename... Args> void displayMessage(std::string_view str_format, Args &&... args)
+template <typename... Args> void displayMessage(std::string_view str_format, Args &&...args)


Format only

Myoldmopar · 2023-12-21T20:48:22Z

src/EnergyPlus/EPVector.hh

@@ -263,12 +263,12 @@ private:
    bool m_allocated{false};
 };

-template <typename T>[[nodiscard]] bool allocated(EPVector<T> const &v) noexcept
+template <typename T> [[nodiscard]] bool allocated(EPVector<T> const &v) noexcept


Format only in this file.

Myoldmopar · 2023-12-21T20:48:32Z

src/EnergyPlus/FileSystem.hh

-    inline constexpr bool enable_json_v = is_all_json_type(fileType) && is_any<T, nlohmann::json>::value &&
-                                          !is_any<T, std::string_view, std::string, char *>::value;
+    inline constexpr bool enable_json_v =
+        is_all_json_type(fileType) && is_any<T, nlohmann::json>::value && !is_any<T, std::string_view, std::string, char *>::value;


Format only.

Myoldmopar · 2023-12-21T20:57:30Z

src/EnergyPlus/PluginManager.cc

@@ -690,7 +752,7 @@ void PluginInstance::reportPythonError([[maybe_unused]] EnergyPlusData &state)
            return;
        }

-        unsigned long numVals = PyList_Size(pyth_val);
+        Py_ssize_t numVals = PyList_Size(pyth_val);


There were several new warnings being emitted due to slight signed/unsigned mismatches and things like that. I thought about ignoring, but it ended up only being a handful of changes and now everything is happy.

Myoldmopar · 2023-12-21T20:57:58Z

src/EnergyPlus/PluginManager.cc

@@ -753,6 +815,7 @@ void PluginInstance::setup([[maybe_unused]] EnergyPlusData &state)
        EnergyPlus::ShowFatalError(state, "Python import error causes program termination");
    }
    PyObject *pModuleDict = PyModule_GetDict(this->pModule);
+    // printDict(pModuleDict);


Remove this debug line.

Myoldmopar · 2023-12-21T20:59:20Z

third_party/ssc/shared/lib_battery_lifetime.h

@@ -81,7 +81,7 @@ class lifetime_t {

    lifetime_t(const lifetime_t &rhs);

-    virtual lifetime_t &operator=(const lifetime_t &rhs);
+    lifetime_t &operator=(const lifetime_t &rhs);


I was getting a compiler warning about how the virtual operator was being hidden by derived classes. By taking off the virtual, we should be getting the same behavior, but without the warning. I could also mute the warning when we include this third party folder, I really don't care. FYI @nmerket

Myoldmopar · 2023-12-21T20:59:34Z

third_party/ssc/shared/lib_battery_lifetime.h

@@ -68,7 +68,7 @@ struct lifetime_state {

    lifetime_state(const std::shared_ptr<cycle_state>& cyc, const std::shared_ptr<calendar_state>& cal);

-    lifetime_state(const std::shared_ptr<lifetime_nmc_state>& nmc);
+    explicit lifetime_state(const std::shared_ptr<lifetime_nmc_state>& nmc);


Tiny warning about single argument constructors. Would be happy to undo.

Myoldmopar · 2023-12-21T21:00:25Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

@@ -2487,109 +2487,109 @@ TEST_F(EnergyPlusFixture, VRF_FluidTCtrl_VRFOU_Compressor)
    }

    // Run and Check: VRFOU_CompSpd
-    {//   Test the method VRFOU_CompSpd, which calculates the compressor speed at given
-     //   operational conditions to meet the evaporator or condenser capacity provided.
+    { //   Test the method VRFOU_CompSpd, which calculates the compressor speed at given


Format changes only in this file.

jmarrec · 2023-12-22T07:52:07Z

cmake/CompilerFlags.cmake

+    # https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=6b927b1297e66e26e62e722bf15c921dcbbd25b9
+    target_compile_options(project_warnings INTERFACE -Wno-dangling-reference)
+    target_compile_options(project_warnings INTERFACE -Wno-array-bounds)
+    target_compile_options(project_warnings INTERFACE -Wno-stringop-overflow)


Define many/lots please? Obviously we're not fans of plain silencing a whole section of warnings, so just trying to gauge if feasible to pragma ignore some of them.

And yes, totally agreed that we should investigate if they are false positive.

(At minimum, I'd say this should be a conditional ignore)

jmarrec · 2023-12-22T07:53:50Z

src/EnergyPlus/PhaseChangeModeling/HysteresisModel.cc

@@ -165,7 +165,7 @@ namespace HysteresisPhaseChange {
                    phaseChangeState = PhaseChangeStates::FREEZING;
                    this->enthNew =
                        this->getEnthalpy(updatedTempTDT, this->peakTempFreezing, this->deltaTempFreezingLow, this->deltaTempFreezingHigh);
-                } else if (this->enthNew < this->enthalpyF && this->enthNew > this->enthalpyM) {
+                } else if (this->enthNew > this->enthalpyM && this->enthNew < this->enthalpyF) {


There should just be parenthesis around each condition, that'd improve readability too. And I think clang format will stop messing up.

Suggested change

} else if (this->enthNew > this->enthalpyM && this->enthNew < this->enthalpyF) {

} else if ((this->enthNew < this->enthalpyF) && (this->enthNew > this->enthalpyM)) {

jmarrec · 2023-12-22T07:55:28Z

src/EnergyPlus/PluginManager.cc

 #if LINK_WITH_PYTHON
+#if PY_MINOR_VERSION <= 10 // fix this


have you tried compiling with python 3.8 (current) and the PyConfig stuff yet? Maybe it just works?

jmarrec · 2023-12-22T08:00:17Z

src/EnergyPlus/PluginManager.cc

-        Py_SetPythonHome(a);
-        PyMem_RawFree(a);
-    }
+#ifdef _WIN32


So, in our general build cases (MSVC, macOS clang, Ubuntu gcc), the macro and the type_traits are indeed functionally equivalent. But I wasn't trying to just be "clever" and use type traits just because I could, I was actually trying to make it truly cross-platform.
What happens if someone tries to build with MinGW on windows for eg? I don't know. But it'll work with type_traits.
And the if constexpr does not make a runtime check, so no penalty.

TL;DR: I think this is a case where ignoring the warning is better.

jmarrec · 2023-12-22T08:35:26Z

src/EnergyPlus/PluginManager.cc

+    PyStatus status;
+
+    // first pre-config Python so that it can speak UTF-8
+    PyPreConfig preConfig;
+    PyPreConfig_InitPythonConfig(&preConfig);
+    preConfig.utf8_mode = 1;
+    status = Py_PreInitialize(&preConfig);
+    if (PyStatus_Exception(status)) {
+        ShowFatalError(state, "Could not pre-initialize Python to speak UTF-8...weird");
+    }


Nice. Perhaps we should improve the error message here, maybe it'll save us some trouble if someone somehow runs into that error and we can't reproduce...

I think this formatter should do. I'll test locally.

template <> struct fmt::formatter<PyStatus> { // parse is inherited from formatter<string_view>. constexpr auto parse(format_parse_context &ctx) -> format_parse_context::iterator { return ctx.begin(); } auto format(const PyStatus &status, format_context &ctx) const -> format_context::iterator { if (!PyStatus_Exception(status)) { return ctx.out(); } if (PyStatus_IsExit(status)) { return fmt::format_to(ctx.out(), "Exited with code {}", status.exitcode); } if (PyStatus_IsError(status)) { auto it = ctx.out(); it = fmt::format_to(it, "Fatal Python error: "); if (status.func) { it = fmt::format_to(it, "{}: ", status.func); } it = fmt::format_to(it, "{}", status.err_msg); return it; } return ctx.out(); } };

Usage: ShowFatalError(state, fmt::format("Could not pre-initialize Python to speak UTF-8... {}", status));

jmarrec · 2023-12-22T09:09:31Z

src/EnergyPlus/PluginManager.cc

+    auto const &gVarNames = state.dataPluginManager->globalVariableNames;
+    auto const it = std::find(gVarNames.begin(), gVarNames.end(), varNameUC);
+    if (it != gVarNames.end()) {
+        return (int)std::distance(gVarNames.begin(), it);


C'MON. C style cast?

jmarrec · 2023-12-22T09:12:17Z

src/EnergyPlus/PluginManager.cc

+    }
+    fs::path const pathToPythonPackages = programDir / "python_standard_lib";
+
+#ifdef LINK_WITH_PYTHON


Wrong position, and wrong macro (shoudl be if, not ifdef)

jmarrec · 2023-12-22T09:25:15Z

src/EnergyPlus/PluginManager.cc

 #if LINK_WITH_PYTHON
+#if PY_MINOR_VERSION <= 10 // fix this


I'm actually a strong proponent of making PyConfig changes into its own PR so we can just test wiht python 3.8. I'm doing that locally as we speak.

* 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

jmarrec · 2023-12-22T09:38:29Z

@Myoldmopar I extracted the PluginManager changes into its own PR at #10342, enabling them for all python versions (including 3.8, our currently supported Python... PyConfig was added in 3.8). It builds and tests perfect on my mac.

Myoldmopar · 2023-12-22T15:41:09Z

So @jmarrec took the work here and split it into a couple PRs, and enhanced them each there. This can be closed, thanks @jmarrec

Updated code, compiler warnings, format, etc. for Ubuntu 23.10/GCC 13…

1f4fb18

….2/Python 3.11, definitely not ready as-is

Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Dec 21, 2023

Myoldmopar commented Dec 21, 2023

View reviewed changes

Myoldmopar added 2 commits December 21, 2023 20:22

Go back to using Clang Format 10, should pass happily now

8e3b692

Clarify compiler args, eliminate some comments

1bf5fb0

jmarrec requested changes Dec 22, 2023

View reviewed changes

jmarrec mentioned this pull request Dec 22, 2023

Use PyConfig to initialize python #10342

Merged

20 tasks

jmarrec mentioned this pull request Dec 22, 2023

GCC13 Fixes #10344

Merged

20 tasks

Myoldmopar closed this Dec 22, 2023

Myoldmopar deleted the Ubuntu2310Fixes branch March 12, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated GCC/Python Support #10340

Updated GCC/Python Support #10340

Myoldmopar commented Dec 21, 2023

Myoldmopar left a comment

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

jmarrec Dec 22, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

Myoldmopar Dec 21, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec Dec 22, 2023

jmarrec commented Dec 22, 2023

Myoldmopar commented Dec 22, 2023

	} else if (this->enthNew > this->enthalpyM && this->enthNew < this->enthalpyF) {
	} else if ((this->enthNew < this->enthalpyF) && (this->enthNew > this->enthalpyM)) {

Updated GCC/Python Support #10340

Updated GCC/Python Support #10340

Conversation

Myoldmopar commented Dec 21, 2023

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Dec 22, 2023

Myoldmopar commented Dec 22, 2023