Compiling v3.0.0 fails on x86
SunBlack opened this issue · 12 comments
Tried to integrate the 3.0.0 release into vcpkg, but it fails due to:
2>C:\dev\libE57Format\src\CheckedFile.cpp(324,42): error C2220: the following warning is treated as an error
2>C:\dev\libE57Format\src\CheckedFile.cpp(324,42): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data
2>C:\dev\libE57Format\src\CheckedFile.cpp(334,45): warning C4244: 'argument': conversion from 'uint64_t' to 'size_t', possible loss of data
Sadly warnings as errors will be always enabled by default as E57_BUILDING_SELF will be set to ON.
What OS version/compiler are you using? (I don't know anything about vcpkg.)
What options are you running CMake with?
Sadly warnings as errors will be always enabled by default as E57_BUILDING_SELF will be set to ON.
As intended! Exactly for this reason.
MSVC 17.4.5 - but this should not matter, since size_t is not necessarily a 64-bit value. Under x86 it seems to be only a 32-bit value and not a 64-bit value, so the warning is correct, since the vectors, arrays, ... can then hold less data.
I understand the warning.
The CI already runs (on x86) using gcc, clang, and MSVC, so I want to know what's different about your setup and why it wasn't captured in CI.
Edit: Ah - I see. The CI for Windows is using x86_64. (Didn't think anyone used 32-bit anymore 😆!) I will add a build for MSVC 32-bit.
Edit: Ah - I see. The CI for Windows is using x86_64. (Didn't think anyone used 32-bit anymore 😆!) I will add a build for MSVC 32-bit.
We also only use x64, but when creating microsoft/vcpkg#29840 I ran vcpkg install libe57format and forgot the triplet and came across it 😅
To locally reproduce it, just run:
cmake ../libE57Format -G "Visual Studio 17 2022" -A Win32 -DE57_BUILD_TEST=OFF
cmake --build .
Another note: Your commit d2f932d doesn't fix the issue, as there are more occurrences. The topic included just one example ;-)
Yeah - I'll get the CI running and look into the others. (I don't have Windows to build on right now.)
Thanks.
(Changes README to say it's 64-bit only 😆 )
Looks like this is going to be... challenging to CI-debug 🤕
Seems to compile now :-) (just build without tests for now, but could test it with them too)
//Edit: Tried it with tests now: I have also XercesC-Issues (probably because I don't have Win32 libs of them on my usually setup)
Just as note, as you have enabled /W4 on MSVC: Not all warnings will be enabled on build with this. To enable all (and have fun), add this (sadly this isn't supported by ccache):
set_target_properties(${target} PROPERTIES
VS_GLOBAL_RunCodeAnalysis true
VS_GLOBAL_EnableMicrosoftCodeAnalysis true
VS_GLOBAL_CodeAnalysisRuleSet AllRules.ruleset
)Also ClangTidy and CppCheck are nice (last one is fast, but has often FP), as I don't see an integration here - they help to find some issues early ;-)
just build without tests for now, but could test it with them too
I have a workflow for this I'll put in after the 32-bit fixes are committed.
I have also XercesC-Issues
Years ago I started a completely new E57 implementation directly from the spec because I couldn't extract xerces-c from this library and because of all the mess in CheckedFile (a constant source of issues). Maybe I should revive that at some point! 😆
you have enabled /W4 on MSVC: Not all warnings will be enabled on build with this
I knew that, but I think it's the closest to -Wall, right?
To enable all (warnings)
I imagine that's like -Weverything... I'm not going to be that crazy especially if I can't fix any of it locally (fixing warnings through CI is... not optimal).
ClangTidy and CppCheck are nice
Agree! 👍 I use them locally in all my projects (through Qt Creator) and have integrated them using CMake/CI in some. I just never integrated them with libE57Format.
Years ago I started a completely new E57 implementation directly from the spec because I couldn't extract xerces-c from this library and because of all the mess in CheckedFile (a constant source of issues). Maybe I should revive that at some point! 😆
This would be great, as we require XercesC just beause of this lib here ;-)
you have enabled /W4 on MSVC: Not all warnings will be enabled on build with this
I knew that, but I think it's the closest to
-Wall, right?
Indeed
To enable all (warnings)
I imagine that's like
-Weverything... I'm not going to be that crazy especially if I can't fix any of it locally (fixing warnings through CI is... not optimal).
-Weverything sounds like /Wand ;-) The ruleset of MSVC is more like ClangTidy.
ClangTidy and CppCheck are nice
Agree! 👍 I use them locally in all my projects (through Qt Creator) and have integrated them using CMake/CI in some. I just never integrated them with libE57Format.
Ah okay, we have integrated them in the CI to make sure no new occurrences will be commited.
This would be great, as we require XercesC just beause of this lib here
Yes - XercesC is a beast of a library. There are several smaller, better options now - especially for something like this - but the way libE57Format is written makes it a chore to replace.
I looked at the state of my new implementation and there would be a fair bit of work to do. I think basic reading (no indices/groups) is about 90% there, writing about 30%. I made some nice command-line tools though! 😄
Most of the infrastructure (and infrastructure-ish) changes in libE57Format 3.0 actually originated with that new implementation - testing, sanitizers, more robust warnings, better version information, ccache, clang-format CI etc..
Given that engagement with libE57Format seems low and there is zero financial support, I'm not sure if I'll pick the new implementation up again. Maybe if I'm bored or inspired someday...