includes in `VC/Tools/MSVC/14.36.32530/include` are not lowercased
Closed this issue · 17 comments
E.g. VC/Tools/MSVC/14.36.32530/include/comdef.h includes Ole2.h and OleCtl.h.
Does 03fed75 fix this issue?
But does it cause further issues? These headers are mostly lowercase, so it should be mostly fine.
A quick check at these headers find the following ones that don't have entirely lowercase names:
./Manifest
./Manifest/dpiaware.manifest
./Manifest/PerMonitorHighDPIAware.manifest
./CodeAnalysis
./CodeAnalysis/HResultCheck
./CodeAnalysis/HResultCheck/warnings.h
./CodeAnalysis/ConcurrencyCheck
./CodeAnalysis/ConcurrencyCheck/warnings.h
./CodeAnalysis/Warnings.h
./CodeAnalysis/VariantClear
./CodeAnalysis/VariantClear/warnings.h
./CodeAnalysis/sourceannotations.h
./CodeAnalysis/CppCoreCheck
./CodeAnalysis/CppCoreCheck/warnings.h
./CodeAnalysis/EnumIndex
./CodeAnalysis/EnumIndex/warnings.h
./fuzzer/FuzzedDataProvider.h
At least the CodeAnalysis/sourceannotations.h header does get included by sal.h, so I guess we'd need to lowercase these files too?
Does 03fed75 fix this issue?
Yes. It does!
we'd need to lowercase these files too?
I do think so. We cannot control how users include these headers, but as least we should make headers installed self-consistent. So that issues like ninja dependencies and ccache cache miss are not due to these installed headers, and users have the chance to fix these issues in their code.
Does 03fed75 fix this issue?
Yes. It does!
Actually, since the WinSDK part of the headers are lowercased with replacement symlinks, I wondered why this was needed at all. But I see that these versions of the names, Ole2.h and OleCtl.h, don't exist in that form at all in the WinSDK. So we do need to do this indeed...
we'd need to lowercase these files too?
I do think so. We cannot control how users include these headers, but as least we should make headers installed self-consistent. So that issues like
ninjadependencies andccachecache miss are not due to these installed headers, and users have the chance to fix these issues in their code.
Yeah, I guess so.
I pushed an amended version at 377a196.
LGTM! NO. In VC/Tools/MSVC/14.36.32532/include/limits:
#include _STL_INTRIN_HEADER
becomes
#include _stl_intrin_header
In
VC/Tools/MSVC/14.36.32532/include/limits:#include _STL_INTRIN_HEADERbecomes
#include _stl_intrin_header
Oh, tricky. Amended the branch as 51d3380.
LGTM now, built OpenCV twice, no issue observed.
Maybe we can add a testcase, simply include all commonly used headers in one source file and compile it?
Maybe we can add a testcase, simply include all commonly used headers in one source file and compile it?
I guess that sounds reasonable.
A file that should trigger many of the issues we've observed here should be this:
#include <windows.h>
#include <comdef.h>
#define _USE_DECLSPECS_FOR_SAL=0
#define _USE_ATTRIBUTES_FOR_SAL=1
#include <sal.h>
I guess we could list dozens of more individual files too, but I don't know how much more specific test coverage that gives, unless you have other cases in mind?
LGTM now, built OpenCV twice, no issue observed.
Pushed these fixes to master, then.
we could list dozens of more individual files
That is what in my mind. We don't pursue test coverage. We can add problematic headers (reported by users) to the test incrementally. We also use the test to verify the correctness of those install scripts, as we might need to fix more complicated issues like:
#define _STL_INTRIN_HEADER <UpperCaseName.h>
#include _STL_INTRIN_HEADERThe ninja-rerun test should be enough. We may also break the headers into some groups, e.g. STL, Win32, MFC/ATL, as sometimes different compile options/definitions lead to different headers being included.
we could list dozens of more individual files
That is what in my mind.
Ok, great. FWIW, a test like this would be good to execute with clang-cl too, to test the headers in case sensitive mode. (We probably don't need to check the deps and build it with ninja, just compiling it manually once should be enough.)
Do you want to add this test into #74?
We still need to check deps with ninja, especially for STL headers. STL uses latest MSVC compiler features which are probably not available in Clang. My thought is to build https://github.com/microsoft/STL/blob/main/stl/modules/std.ixx directly, so that we do not need to track c++ standard headers manually.
Build Win32 and UCRT headers with clang-cl would be good though. I don't have a list of headers in mind now, so I don't want to add this to #74. The test-clang-lld need refactor too and it fails with Clang16 on my PC, I think -fuse-ld=lld is needed but no idea why CI passes.
msvc-wine/.github/workflows/build.yml
Line 112 in 4381888
We still need to check deps with ninja, especially for STL headers.
Yes, that's definitely necessary to test - but for testing the case sensitiveness, a plain compile with clang-cl is good extra coverage too.
STL uses latest MSVC compiler features which are probably not available in Clang. My thought is to build https://github.com/microsoft/STL/blob/main/stl/modules/std.ixx directly, so that we do not need to track c++ standard headers manually.
Ok - that might be reasonable, I don't quite know at the moment - I have zero experience with C++ modules so far.
Build Win32 and UCRT headers with clang-cl would be good though. I don't have a list of headers in mind now, so I don't want to add this to #74. The
test-clang-lldneed refactor too and it fails with Clang16 on my PC, I think-fuse-ld=lldis needed but no idea why CI passes.
Ok, I can take a stab at that after you otherwise feel #74 is ready to be merged (it's still marked as draft).
You're right that it's weird if that fails for you but passes in CI.
When CMake itself uses clang-cl, it will generally default to doing the separate linking step with lld-link instead of link, so that should generally work. But it's possible that some step tries to do a plain compile+link with one command, and then clang-cl will default to trying to find link.exe, which might fail. Even if that would find it though, that CI environment lacks Wine so it can't be doing that. So you're right that it's somewhat mysterious.
If you want to dig further into that, a CI run that prints the CMakeFiles/CMake{Output.log,Error.log,ConfigureLog.yaml} afterwards, and comparing that to yours, probably could shed some light on that difference.
You're right that it's weird if that fails for you but passes in CI.
It's my fault, forgot passing -DCMAKE_SYSTEM_NAME=Windows to cmake.
As per my test, clang (as well as clang-cl) is also supported?
You're right that it's weird if that fails for you but passes in CI.
It's my fault, forgot passing
-DCMAKE_SYSTEM_NAME=Windowsto cmake.
Ah, that explains it. The errors you get when forgetting to set that are rather cryptic…
As per my test,
clang(as well asclang-cl) is also supported?
Yes, clang with a suitable -target <triple> or --target=<triple> is generally equivalent to clang-cl, except that it takes gcc-style arguments. (And clang-cl without a target set implicitly targets msvc for the default architecture.) Since it invokes a link-style linker, the -Wl,-.. flags passed to it must be suitable for link/lld-link though.
That combination is a bit unusual though; not all projects that otherwise might work with both clang/gcc in general and msvc are prepared for that combo.
OK, thanks for the explanations. And I find this thread https://discourse.llvm.org/t/how-to-specify-winsysroot-argument-without-using-clang-cl/61643, the /winsysroot option isn't supported by clang.
Closes as #75 landed.