Skip to content
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

Implement file mapping for UWP apps #1775

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

sergio-nsk
Copy link
Contributor

@sergio-nsk sergio-nsk commented Feb 6, 2024

Fixes #1773.

@yhirose I have not checked it - perhaps, the new code also can replace Windows Desktop code, but it needs a real UTF-8 to UTF-16 conversion.

@yhirose
Copy link
Owner

yhirose commented Feb 6, 2024

@sergio-nsk What does "I have not checked it" mean? At least, I would like you to confirm that this change works on your UWP apps, and also confirm that it doesn't break anything on Windows.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Feb 6, 2024

I mean, I have not checked if the new code can work on Windows Desktop and the inner #if #else can be eliminated. #else #endif works in UWP.
It should not break the current code for Windows Desktop, because it does not change it.

@yhirose
Copy link
Owner

yhirose commented Feb 7, 2024

@sergio-nsk thanks for the reply.

According to your comment in #1773, the only thing we have to do is to change CreateFileA to something else.

If CreateFile2 works on WinRT/UWP, I think the following small change is enough for both the regular Windows and WinRT/UWP. I confirmed that all the unit tests (test/test.cc) related to the code change passed successfully.

diff --git a/httplib.h b/httplib.h
index dfdd260..8ef4d01 100644
--- a/httplib.h
+++ b/httplib.h
@@ -2686,8 +2686,13 @@ inline bool mmap::open(const char *path) {
   close();

 #if defined(_WIN32)
-  hFile_ = ::CreateFileA(path, GENERIC_READ, FILE_SHARE_READ, NULL,
-                         OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+  std::wstring wpath;
+  for (size_t i = 0; i < strlen(path); i++) {
+     wpath += path[i];
+  }
+
+  hFile_ = ::CreateFile2(wpath.c_str(), GENERIC_READ, FILE_SHARE_READ,
+                         OPEN_EXISTING, NULL);

   if (hFile_ == INVALID_HANDLE_VALUE) { return false; }

Could you test the above code on your UWP environment? Thanks a lot!

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Feb 7, 2024

Honestly, I talked only about the first compiler error. Other functions GetFileSize(), CreateFileMapping(), MapViewOfFile() are also unavailable in UWP apps. UnmapViewOfFile() is only available. That's why I suggest that you test the complete new code block.

- #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
+ #if 0 && WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)

I can't test it at my end fairly, because I am getting same failures before and after my changes.

[  FAILED  ] 17 tests, listed below:
[  FAILED  ] ChunkedEncodingTest.FromHTTPWatch_Online
[  FAILED  ] ChunkedEncodingTest.WithContentReceiver_Online
[  FAILED  ] ChunkedEncodingTest.WithResponseHandlerAndContentReceiver_Online
[  FAILED  ] ServerTest.HeadMethod200Static
[  FAILED  ] ServerTest.GetMethodDir
[  FAILED  ] ServerTest.GetMethodDirTest
[  FAILED  ] ServerTest.GetMethodDirTestWithDoubleDots
[  FAILED  ] ServerTest.GetMethodDirMountTest
[  FAILED  ] ServerTest.GetMethodDirMountTestWithDoubleDots
[  FAILED  ] ServerTest.UserDefinedMIMETypeMapping
[  FAILED  ] ServerTest.StaticFileRange
[  FAILED  ] ServerTest.StaticFileRanges
[  FAILED  ] ServerTest.StaticFileRangeHead
[  FAILED  ] ServerTest.StaticFileRangeBigFile
[  FAILED  ] ServerTest.StaticFileRangeBigFile2
[  FAILED  ] ServerTest.StaticFileBigFile
[  FAILED  ] MountTest.Unmount

@yhirose
Copy link
Owner

yhirose commented Feb 7, 2024

Thank you for the details. I tested the following code on my machine and it worked successfully. Could you just change the current code with this, so that we can use the same code for both the regular Windows and WinRT/UWP?

  std::wstring wpath;
  for (size_t i = 0; i < strlen(path); i++) {
     wpath += path[i];
  }

  hFile_ = ::CreateFile2(wpath.c_str(), GENERIC_READ, FILE_SHARE_READ,
                         OPEN_EXISTING, NULL);

  if (hFile_ == INVALID_HANDLE_VALUE) { return false; }

  LARGE_INTEGER size{};
  if (!::GetFileSizeEx(hFile_, &size)) { return false; }
  size_ = static_cast<size_t>(size.QuadPart);

  hMapping_ = ::CreateFileMappingFromApp(hFile_, NULL, PAGE_READONLY, size_,
                                         NULL);

  if (hMapping_ == NULL) {
      close();
      return false;
  }

  addr_ = ::MapViewOfFileFromApp(hMapping_, FILE_MAP_READ, 0, 0);

@sergio-nsk
Copy link
Contributor Author

Sorry, I just noticed that it will not compile on Windows 7. Are you sure you want to break that although you don't support it?

NOTE: Windows 8 or lower, Visual Studio 2013 or lower, and Cygwin and MSYS2 including MinGW are neither supported nor tested.

@yhirose
Copy link
Owner

yhirose commented Feb 8, 2024

I think that's fine, since the support for Windows 7 has already ended on January 14, 2020.

@sergio-nsk sergio-nsk force-pushed the patch-1 branch 2 times, most recently from 942ecce to 53cae82 Compare February 8, 2024 02:45
@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Feb 8, 2024

Done, with support of VS 2015.

CMakeLists.txt Outdated
@@ -227,6 +227,8 @@ target_link_libraries(${PROJECT_NAME} ${_INTERFACE_OR_PUBLIC}

# Set the definitions to enable optional features
target_compile_definitions(${PROJECT_NAME} ${_INTERFACE_OR_PUBLIC}
# Require Windows 8
$<$<PLATFORM_ID:Windows>:_WIN32_WINNT=0x0602>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sum01 @jimmy-park could you review the change in CMakeLists.txt, since I am not an expert of CMake? Thanks a lot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this affect other CMake projects that use httplib?
I think the macro should remain in httplib.h.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimmy-park thank you for the comment.
@sergio-nsk could you remove this change?

Copy link
Contributor Author

@sergio-nsk sergio-nsk Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this affect other CMake projects that use httplib? I think the macro should remain in httplib.h.

It will affect user projects if they don't define own _WIN32_WINNT values.

@yhirose yhirose merged commit ad40bd6 into yhirose:master Feb 9, 2024
4 checks passed
@yhirose
Copy link
Owner

yhirose commented Feb 9, 2024

@sergio-nsk thanks for your contribution!

Repository owner deleted a comment Jun 25, 2024
mol123 added a commit to mol123/cpp-httplib that referenced this pull request Jun 27, 2024
mol123 added a commit to mol123/cpp-httplib that referenced this pull request Jun 27, 2024
yhirose pushed a commit that referenced this pull request Jul 2, 2024
* Fix build when targeting Windows 7 as platform.

This change makes more of the code introduced in
#1775
conditional on feature macros.

`CreateFile2`, `CreateFileMappingFromApp` and `MapViewOfFileFromApp` are
available only starting from Windows 8.

 * https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfile2
 * https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-createfilemappingfromapp
 * https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffilefromapp

* Update feature macros used and use `GetFileSizeEx` conditionally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking change on WinRT/UWP
3 participants