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

Fix build when targeting Windows 7 as platform. #1869

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

mol123
Copy link
Contributor

@mol123 mol123 commented Jun 27, 2024

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.

@Zor-X-L
Copy link

Zor-X-L commented Jul 1, 2024

@yhirose win7 boxes are still widely available, great downstream projects like llama.cpp are using this great library, I hope it can support win7. ggerganov/llama.cpp#8208

@mol123 I think your code can work but for the actual condition according to win11 sdk header file:

  • CreateFile2: (_WIN32_WINNT >= _WIN32_WINNT_WIN8) && WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)
  • CreateFileMappingFromApp: (_WIN32_WINNT >= _WIN32_WINNT_WIN8) && WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM)
  • MapViewOfFileFromApp: (_WIN32_WINNT >= _WIN32_WINNT_WIN8) && WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM)
  • GetFileSizeEx: WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_APP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)
    • So if only WINAPI_PARTITION_DESKTOP is available, in theory we should use GetFileSize, which has condition WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_SYSTEM | WINAPI_PARTITION_GAMES)

@mol123
Copy link
Contributor Author

mol123 commented Jul 1, 2024

  • I've updated the conditions used as suggested by @Zor-X-L
  • GetFileSizeEx is now used conditionally as well.

@yhirose
Copy link
Owner

yhirose commented Jul 2, 2024

@mol123 thanks for the pull request. Could you confirm that Win8 or later use the new APIs and Win7 or lower uses the old APIs? Once you test and confirm it, please let me know.

@mol123
Copy link
Contributor Author

mol123 commented Jul 2, 2024

I've tested the code on both Windows 7 and Windows 10 (>= Windows8) and can confirm that it works.

Details:

I've created the following test program:

#include <iostream>
#include <string>

#include "httplib.h"

int main() {
  httplib::detail::mmap m("c:\\Windows\\System.ini");
  std::string s(m.data(), m.size());
  std::cout << m.size() << std::endl;
  std::cout << s << std::endl;
}

I've compiled it:

  • With -D_WIN32_WINNT=_WIN32_WINNT_WIN8 -DWINVER=_WIN32_WINNT_WIN8 (targeting Win8 as minimum version) -> httplibtest8.exe
  • With -D_WIN32_WINNT=_WIN32_WINNT_WIN7 -DWINVER=_WIN32_WINNT_WIN7 (targeting Win7 as minimum version) -> httplibtest7.exe

On Windows 10, both httplibtest8.exe and httplibtest7.exe work.

On Windows 7, httplibtest7.exe works. httplibtest8.exe fails with the error "The procedure entry point CreateFile2 could not be located in the dynamic link library KERNEL32.dll "

@yhirose yhirose merged commit c8bcaf8 into yhirose:master Jul 2, 2024
4 checks passed
@yhirose
Copy link
Owner

yhirose commented Jul 2, 2024

@mol123 @Zor-X-L thanks for your contribution!

@wolf-hunter404
Copy link

I pull the latest code,compile using VS2017, system is win11.
Still can't run on win7.

@wolf-hunter404
Copy link

@yhirose

@yhirose
Copy link
Owner

yhirose commented Jul 3, 2024

@mol123 @Zor-X-L, could you answer the question from @qwe857359351a, since I didn't work on this? Thanks.

@mol123
Copy link
Contributor Author

mol123 commented Jul 4, 2024

@qwe857359351a: the code has to be compiled targeting Windows 7 as minimal version, specifically setting -D_WIN32_WINNT=_WIN32_WINNT_WIN7 -DWINVER=_WIN32_WINNT_WIN7 during compilation.

See:

@wolf-hunter404
Copy link

_WIN32_WINNT=_WIN32_WINNT_WIN7 -DWINVER=_WIN32_WINNT_WIN7

Thinks, already able to run.
@yhirose

Royna2544 added a commit to Royna2544/c_cpp_telegrambot that referenced this pull request Jul 16, 2024
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.

4 participants