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

update msvc ASAN support and copy required install dlls #77

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DewJunkie
Copy link
Contributor

MSVC does not allow the use of ASAN on debug builds, so modified sanitizers.cmake to reflect that.
MSVC requires the ASAN dlls to run, so I put an install for them into sanitizers.cmake.

The package updates are unrelated, but I was getting a lot of deprication warnings (promoted to errors) in msvc debug builds. Bumping the version to the latest resolved them.

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d19d892) 24.70% compared to head (792ce81) 24.70%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage   24.70%   24.70%           
=======================================
  Files           6        6           
  Lines         251      251           
  Branches      142      143    +1     
=======================================
  Hits           62       62           
- Misses        181      184    +3     
+ Partials        8        5    -3     
Flag Coverage Δ
Linux 14.89% <ø> (-1.54%) ⬇️
Windows 21.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DewJunkie
Copy link
Contributor Author

I'm pretty certain that the build failures are related to cpp check now failing the constexpr unit tests, because it issues a warning that the tests will always be true.

cmake_template\test\constexpr_tests.cpp(10,41): warning GEB24262F: Condition 'factorial_constexpr(3)==6' is always true [knownConditionTrueFalse]
    STATIC_REQUIRE(factorial_constexpr(3) == 6);

Are you ok with a new option for cppcheck warnings as errors, currently it picks up the project warnings as errors. The other option would be to just not have it set in the constexpr tests.

It is also picking up some errors in spdlog, I'm guessing there must be a way to tell spdlog to not check 3rd party libs, but I guess for me I'd rather still see them.

cmake_template\out\build\windows-clang-release\_deps\spdlog-src\include\spdlog\details\null_mutex.h(19,5): warning GDD13A24C: Member variable 'null_atomic_int::value' is not initialized in the constructor. [uninitMemberVar]
      null_atomic_int() = default;

Not in any rush to merge this change, but it does make the windows builds clone->build->run, which I think is a plus.

@lefticus
Copy link
Member

@DewJunkie The MSVC + ASAN + CMake has been no end of pain for me over the years. I dislike having to copy the DLL's around, but am up for it if it gets things working.

There's a couple of other CI fixes in the pipeline, I'm going to try and get those merged first, then sort this one out with you.

@DewJunkie
Copy link
Contributor Author

@lefticus too true. I can put some time into seeing if I can just mark those files as runtime dependencies, rather than forcing them to get installed. install() is one of my weaker cmake areas.

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.

3 participants