-
Notifications
You must be signed in to change notification settings - Fork 1
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
Various Windows fixes #537
Conversation
Scan the executable to figure out the dependencies and copy them into the install folder. Fixes #533.
This affects both downloading zip files and calculating checksums for files. Fixes #536.
It returns false if the directory already exists and we seem to be getting spurious failures. Let's not bother checking the return value; we only care about file creation anyway and we are checking that.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
==========================================
+ Coverage 48.72% 49.12% +0.40%
==========================================
Files 164 164
Lines 7795 7796 +1
Branches 1063 1062 -1
==========================================
+ Hits 3798 3830 +32
+ Misses 3988 3957 -31
Partials 9 9 ☔ View full report in Codecov by Sentry. |
Btw: Windows binaries should work fine with It's handy if you want to test that the CI-generated Windows binaries work without having to boot Windows. |
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.
Okay. Not sure I understand the CMake changes, RUNTIME, ARCHIVE, LIBRARY, RUNTIME, ...
. And esp. the windows part.
Also is the create directory exception not needed anymore?
I trust it's right, so LGTM.
@@ -62,11 +62,12 @@ void extract_zip_file(const std::filesystem::path &file_path, | |||
for (const auto &entry : zf.getEntries()) { | |||
out_path = temp_output_directory / entry.getName(); | |||
if (entry.isDirectory()) { | |||
if (!std::filesystem::create_directories(out_path)) { | |||
throw std::runtime_error{ |
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.
Don't need the exception any more?
I don't entirely understand why those bits were needed either 🙃. I copied from here: https://stackoverflow.com/questions/10671916/how-to-copy-dll-files-into-the-same-folder-as-the-executable-using-cmake The bit about installing
I was getting spurious failures here, I think because the extra |
This PR fixes the Windows (MSVC) build of HGPS.
There were a few issues I had in trying to get HGPS to run on Windows:
fstream
s were being opened in text mode rather than binary mode, which won't work with binary files on WindowsFixes #533. Fixes #536.