Fix #10487 - Paths containing characters other than 7-bit ASCII cause failure on windows #10619

jmarrec · 2024-07-23T04:47:03Z

Pull request overview

Fixes Paths containing characters other than 7-bit ASCII cause failure #10487

I have made a test release at https://github.com/jmarrec/EnergyPlus/releases/tag/v24.2.0-windows_non_ascii and @DaveInCaz tried it.

(namespace fs = std::filesystem)

As a reminder, Windows behaves differently than the rest of the world (Shocking, I know). Anyways, on all platforms we target, fs::path::value_type is char, except on windows where it's wchar_t.

With a char that has a 2 bytes unicode such as Б (Unicode 0411), it just plain crashes everytime you call fs::path::string().

The fix here is to avoid ever doing that. I narrow the path manually when either printing or passing it to for eg sqlite3 is needed.

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.

…adJson too though

…+ avoid using path.string() in that file (need to do it for more)

…wide string to string on windows. Supports 's' (string()) or 'g' (generic_string())presentations

jmarrec · 2024-07-23T04:48:01Z

src/EnergyPlus/FileSystem.hh

+    std::string toString(fs::path const &p);
+
+    std::string toGenericString(fs::path const &p);
+
+    fs::path appendSuffixToPath(fs::path const& outputFilePrefixFullPath, const std::string &suffix);


New helpers

jmarrec · 2024-07-23T04:49:15Z

src/EnergyPlus/FileSystem.hh

+// Add a custom formatter for fmt
+template <> struct fmt::formatter<fs::path>
+{
+    // Presentation format: 's' - string, 'g' - generic_string.
+    char presentation = 's';
+
+    // Parses format specifications of the form ['s' | 'g'].
+    constexpr auto parse(format_parse_context &ctx) -> decltype(ctx.begin())
+    {
+        // Parse the presentation format and store it in the formatter:
+        auto it = ctx.begin(), end = ctx.end();
+        if (it != end && (*it == 's' || *it == 'g')) {
+            presentation = *it++;
+        }
+
+        // Check if reached the end of the range:
+        if (it != end && *it != '}') {
+            throw format_error("invalid format");
+        };
+
+        // Return an iterator past the end of the parsed range:
+        return it;
+    }
+
+    template <typename FormatContext> auto format(const fs::path &p, FormatContext &ctx) -> decltype(ctx.out())
+    {
+        return format_to(ctx.out(), "{}", presentation == 'g' ? EnergyPlus::FileSystem::toGenericString(p) : EnergyPlus::FileSystem::toString(p));
+    }
+};


new formatter for fs::path, instead of using the std::string one.

Two presentation types are accepted: s (default), and g.

s uses toString (and is similar to fs::path::string() but with narrowing) and g uses toGenericString (and is sililar to fs::path::generic_string())

This looks great to me. I'm like 23% surprised you had to make your own formatter for this. So 77% not surprised because of wide char nonsense.

jmarrec · 2024-07-23T04:50:11Z

src/EnergyPlus/FileSystem.cc

+#ifdef _WIN32
+        result.make_preferred();
+#else


in makeNativePath, avoid going to a string() if not needed (and will potentially crash)

jmarrec · 2024-07-23T04:50:26Z

src/EnergyPlus/FileSystem.cc

+#ifdef _WIN32
+        std::wstring pathStr = path.native();
+        if (!pathStr.empty()) {
+            while ((pathStr.back() == DataStringGlobals::pathChar) || (pathStr.back() == DataStringGlobals::altpathChar)) {
+                pathStr.erase(pathStr.size() - 1);
+            }
+        }
+#else


Same in getParentDirectoryPath...

jmarrec · 2024-07-23T04:51:47Z

src/EnergyPlus/FileSystem.cc

+        const std::uintmax_t file_size = fs::file_size(filePath);
+        std::ifstream file(filePath, mode);
+        if (!file.is_open()) {
+            throw FatalError(fmt::format("Could not open file: {}", filePath));
        }
+        std::string result(file_size, '\0');
+        file.read(result.data(), file_size);


Change readFile and readJSON to use ifstream (that handles fs::path) + avoid using path.string()

jmarrec · 2024-07-23T04:52:09Z

src/EnergyPlus/FileSystem.cc

+    std::string toString(fs::path const &p)
+    {
+        if constexpr (std::is_same_v<typename fs::path::value_type, wchar_t>) {
+            return CLI::narrow(p.wstring());
+        } else {
+            return p.string();
+        }
+    }
+
+    std::string toGenericString(fs::path const &p)
+    {
+        if constexpr (std::is_same_v<typename fs::path::value_type, wchar_t>) {
+            return CLI::narrow(p.generic_wstring());
+        } else {
+            return p.generic_string();
+        }
+    }
+
+    fs::path appendSuffixToPath(fs::path const &outputFilePrefixFullPath, const std::string &suffix)
+    {
+        if constexpr (std::is_same_v<typename fs::path::value_type, wchar_t>) {
+            return {outputFilePrefixFullPath.wstring() + CLI::widen(suffix)};
+        } else {
+            return {outputFilePrefixFullPath.string() + suffix};
+        }
+    }


New helpers definitions

jmarrec · 2024-07-23T04:53:02Z

src/EnergyPlus/SQLiteProcedures.cc

+        if constexpr (debug) {
+            // std::cout << "errorStream=" << errorStream << ", dbName=" << dbName << std::endl;
+            // std::cout << "dbName.string()=" << dbName.string() << std::endl;
+            // std::cout << "dbName.generic_string()=" << dbName.generic_string() << std::endl;
+            std::wcout << "dbName.generic_wstring()=" << dbName.generic_wstring() << std::endl;
+            std::cout << "narrow(dbName.generic_wstring())=" << FileSystem::toGenericString(dbName) << std::endl;
+        }


I need to clean that up...

Done in 48eb2eb

jmarrec · 2024-07-23T04:57:40Z

src/EnergyPlus/SQLiteProcedures.cc

+                    if (fs::is_regular_file(dbName)) {
+                        std::error_code ec;
+                        if (!fs::remove(dbName, ec)) {
+                            // File operation failed. SQLite connection is not in an error state.
+                            *m_errorStream << "SQLite3 message, can't remove old database. code=" << ec.value() << ", error: " << ec.message()
+                                           << std::endl;
+                            ok = false;
+                        }


instead of calling the C stdio.h remove, use the fs::remove one

jmarrec · 2024-07-23T04:58:03Z

src/EnergyPlus/SQLiteProcedures.cc

@@ -2665,7 +2696,7 @@ SQLiteProcedures::SQLiteProcedures(std::shared_ptr<std::ostream> const &errorStr

        if (ok) {
            // Now open the output db for the duration of the simulation
-            rc = sqlite3_open_v2(dbName.string().c_str(), &m_connection, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, nullptr);
+            rc = sqlite3_open_v2(dbName_utf8.c_str(), &m_connection, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, nullptr);


When opening the sqlite database, same thing: don't call string(), but our narrowing one.

jmarrec · 2024-07-23T04:58:34Z

src/EnergyPlus/ScheduleManager.cc

@@ -490,7 +490,7 @@ namespace ScheduleManager {
                                ShowSevereError(state, error);
                            }
                        }
-                        ShowContinueError(state, fmt::format("Error Occurred in {}", state.files.TempFullFilePath.filePath.string()));
+                        ShowContinueError(state, fmt::format("Error Occurred in {}", state.files.TempFullFilePath.filePath));


use the new fmt::formatterfs::path everywhere, so we don't call fs::path::string()

Myoldmopar

Looks good, I'll pull develop and test things out. :) :g :)

Myoldmopar · 2024-07-25T19:46:54Z

src/EnergyPlus/FileSystem.hh

+// Add a custom formatter for fmt
+template <> struct fmt::formatter<fs::path>
+{
+    // Presentation format: 's' - string, 'g' - generic_string.
+    char presentation = 's';
+
+    // Parses format specifications of the form ['s' | 'g'].
+    constexpr auto parse(format_parse_context &ctx) -> decltype(ctx.begin())
+    {
+        // Parse the presentation format and store it in the formatter:
+        auto it = ctx.begin(), end = ctx.end();
+        if (it != end && (*it == 's' || *it == 'g')) {
+            presentation = *it++;
+        }
+
+        // Check if reached the end of the range:
+        if (it != end && *it != '}') {
+            throw format_error("invalid format");
+        };
+
+        // Return an iterator past the end of the parsed range:
+        return it;
+    }
+
+    template <typename FormatContext> auto format(const fs::path &p, FormatContext &ctx) -> decltype(ctx.out())
+    {
+        return format_to(ctx.out(), "{}", presentation == 'g' ? EnergyPlus::FileSystem::toGenericString(p) : EnergyPlus::FileSystem::toString(p));
+    }
+};


This looks great to me. I'm like 23% surprised you had to make your own formatter for this. So 77% not surprised because of wide char nonsense.

…SCII_paths

Myoldmopar · 2024-07-25T20:09:09Z

Everything is totally happy locally, but I had to apply Clang Format. I'll push that up real quick and let CI confirm that, and then merge.

Myoldmopar · 2024-07-25T20:33:12Z

CI seems all happy with it, and the only thing I did was apply Clang Format, so I'm not waiting on Decent to finish. Merging. Thanks @jmarrec

jmarrec added 7 commits July 23, 2024 06:38

print args when --debug-cli is passed

4b6a726

WIP, seems to work even with 2 byte unicode chars. I need to tweak re…

9fc5f17

…adJson too though

Change readFile and readJSON to use ifstream (that handles fs::path) …

cd84fa3

…+ avoid using path.string() in that file (need to do it for more)

Add a specific fmt::formatter for fs::path that will **narrow** from …

6121f19

…wide string to string on windows. Supports 's' (string()) or 'g' (generic_string())presentations

Use the new fmt::formatter<fs::path> (except ExternalInterface)

1573494

tweak toString to use a type_traits too

16e1ba3

update ExternalInterface

83d7cf3

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jul 23, 2024

jmarrec self-assigned this Jul 23, 2024

Remove debug code in SQLiteProcedures

48eb2eb

jmarrec commented Jul 23, 2024

View reviewed changes

jmarrec requested a review from Myoldmopar July 23, 2024 04:59

Myoldmopar approved these changes Jul 25, 2024

View reviewed changes

Myoldmopar added 2 commits July 25, 2024 14:57

Merge remote-tracking branch 'origin/develop' into 10487_Windows_NonA…

fcf2672

…SCII_paths

Applied clang format

ba5a73f

Myoldmopar merged commit f2cf440 into develop Jul 25, 2024
10 of 15 checks passed

Myoldmopar deleted the 10487_Windows_NonASCII_paths branch July 25, 2024 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10487 - Paths containing characters other than 7-bit ASCII cause failure on windows #10619

Fix #10487 - Paths containing characters other than 7-bit ASCII cause failure on windows #10619

jmarrec commented Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

Myoldmopar Jul 25, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

jmarrec Jul 23, 2024

Myoldmopar left a comment

Myoldmopar Jul 25, 2024

Myoldmopar commented Jul 25, 2024

Myoldmopar commented Jul 25, 2024

Fix #10487 - Paths containing characters other than 7-bit ASCII cause failure on windows #10619

Fix #10487 - Paths containing characters other than 7-bit ASCII cause failure on windows #10619

Conversation

jmarrec commented Jul 23, 2024

Pull request overview

Pull Request Author

Reviewer

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

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jul 25, 2024

Myoldmopar commented Jul 25, 2024