fix(build): gate _CRT_SECURE_NO_WARNINGS on WIN32, not MSVC - #111
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kota_project_options guarded _CRT_SECURE_NO_WARNINGS with
$<$<BOOL:${MSVC}>:...>, which is FALSE for clang++ targeting Windows
natively (e.g. conda-forge clangxx used by downstream CI). That
compiler still links ucrt/msvcrt via -Xclang --dependent-lib=msvcrt,
so MSVC CRT headers still emit _CRT_INSECURE_DEPRECATE on fopen,
strcpy, etc. Previously this only surfaced as a warning, but
#109 (rename + introduce -Wall -Wextra -Werror in project_options)
promoted it to an error, breaking kotatsu's own
recording_transport.cpp compile on clang-native Windows as soon as
downstream projects fetched kotatsu main.
Gate the define on WIN32 instead, so cl/clang-cl/clang-native all
get it. MinGW also picks it up but its CRT doesn't emit the macro,
so the define is a harmless no-op there.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bd2b8c8 to
09bde67
Compare
1 participant
Summary
On Windows, any Clang-based toolchain that links against MSVC CRT (ucrt/msvcrt) — clang-cl, clang-native via
-Xclang --dependent-lib=msvcrt, MSVC itself — triggers the_CRT_INSECURE_DEPRECATE(fopen_s)attribute on functions likefopen. Kotatsu's ownsrc/ipc/recording_transport.cppusesstd::fopen, so that attribute produces a-Wdeprecated-declarationswarning during kotatsu's internal build unless_CRT_SECURE_NO_WARNINGSis defined.The existing gate
$<$<BOOL:${MSVC}>:_CRT_SECURE_NO_WARNINGS>only applies to MSVC proper and clang-cl. Under clang-native on Windows (e.g.conda-forgeclangxx, which downstream CI matrices use),${MSVC}is FALSE, the define is skipped, and the warning fires on kotatsu's own TU.Before #109, this was tolerable — it was only a warning. #109 introduced
-Wall -Wextra -Werrorintokota::project_options, which promoted the warning to an error. Result: every downstream consumer (e.g. clice#428) started failing its Windows-clang CI on kotatsu's internal compile the moment it picked up main.Fix
Gate the define on
WIN32instead ofMSVC. MinGW gets the define too, but its CRT doesn't emit_CRT_INSECURE_DEPRECATE, so it's a harmless no-op there.Test plan
_deps/*/src/ipc/recording_transport.cpp.obj:warning: 'fopen' is deprecated … [-Wdeprecated-declarations](non-fatal)error: 'fopen' is deprecated … [-Werror,-Wdeprecated-declarations](fatal)clang++.exe -Xclang --dependent-lib=msvcrtcommand line; the only relevant delta was-Werrorbeing introduced.Why not fix
recording_transport.cppto usefopen_s/std::ofstreaminstead?Viable, but scope creep — the define is how every MSVC-CRT consumer silences deprecation spam on CRT functions kotatsu uses today and may use tomorrow. One-line gate fix is tighter.
🤖 Generated with Claude Code