fix: Windows/MSVC build (5 build.rs bugs)#7
Open
zxcloli666 wants to merge 5 commits into
Open
Conversation
…figure_arg
cmake-rs only recognizes Ninja through its own generator field; on MSVC
targets it otherwise assumes a Visual Studio generator and appends
-Thost=x64 -Ax64, which conflicts with a Ninja generator passed as a raw
configure_arg ("Generator Ninja does not support platform specification").
…r binding.cc .flag() passes GCC/Clang spellings (-std=c++17, -fexceptions, -frtti) straight through; cl.exe silently ignores unrecognized "-"-prefixed args instead of erroring, so binding.cc compiled in a pre-C++17 mode under MSVC and failed on structured bindings. flag_if_supported() probes the actual compiler and applies whichever spelling (GCC/Clang or MSVC) it accepts.
…libs on Windows Two compounding bugs in the same linking step, the first masking the second: (1) the static-archive discovery walk only matched "*.a" files, so on MSVC (which produces "*.lib") it silently linked none of Hermes' own built libraries at all; (2) the system-library fallback branch unconditionally added "stdc++"/"icuuc"/"icui18n"/"icudata", which don't exist under those names on Windows and made the linker fail before ever reaching a symbol-resolution error that would have surfaced bug (1).
…elease
cmake-rs infers CMAKE_BUILD_TYPE from cargo's own profile ("Debug" for a
plain cargo build). Under MSVC that pulls in CMake's default /MDd (debug
CRT) flags for Hermes' object files, while Rust's linked output always
expects the release CRT (/MD) regardless of --release — the mismatch
surfaces as dozens of unresolved __imp__calloc_dbg (and other debug-CRT
symbols) at final link. Force Release unconditionally: a vendored C++ VM
built unoptimized to match our own dev profile buys nothing (we never
edit it) and would just be slower to run.
CMake reporting "Using Windows 10 built-in ICU" only means PlatformUnicodeICU.cpp calls the same unorm2_*/ucol_*/udat_*/u_str* symbols ICU4C exposes, backed by the OS's icu.dll instead of a vendored ICU — it doesn't embed that linkage into hermesvm_a.lib, and Windows' import library is named "icu", not "icuuc"/"icui18n"/"icudata" (those are POSIX ICU4C names). SamplingProfilerSampler.cpp's high-res timer also needs winmm's timeBeginPeriod/timeEndPeriod.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Got this building and running on Windows (MSVC 14.44 + Ninja, Windows 10 SDK) while wiring Hermes into a custom desktop runtime.
libhermes-sys/build.rshad five separate bugs blocking it, all Windows-only in effect — Linux/macOS paths are either untouched or the change is a strict superset there.CMake generator conflict.
-G Ninjawas passed viaconfigure_argas a raw string instead ofConfig::generator("Ninja").cmake-rsonly recognizes Ninja through its owngeneratorfield — on MSVC targets it still assumes a Visual Studio generator and appends-Thost=x64 -Ax64, which conflicts with the real-G Ninjaarg ("Generator Ninja does not support platform specification").binding.cc compiler flags.
cc::Buildused.flag("-std=c++17")/.flag("-fexceptions")/.flag("-frtti")— GCC/Clang spellings thatcl.exesilently ignores (unrecognized--prefixed args just warn, they don't error). It compiles in whatever pre-C++17 mode MSVC defaults to and fails on structured bindings. Switched to.flag_if_supported()with both spellings (POSIX +/std:c++17//EHsc//GR).Static archive discovery + system libs. The discovery walk only matched
*.a; MSVC produces*.lib, so none of Hermes' own built libraries were ever linked. This was masked by a second bug in the same fallback: it unconditionally linkedstdc++/icuuc/icui18n/icudatafor anything that isn't macOS, and those names don't exist on Windows, so the linker died on a missingstdc++.libbefore it ever got far enough to expose the first bug.Debug/release CRT mismatch. Once 3 was fixed, link failed on ~20
unresolved external symbol __imp__calloc_dbg(and friends).cmake-rsinfersCMAKE_BUILD_TYPEfrom Cargo's profile ("Debug" for a plaincargo build), which under MSVC pulls in the debug CRT (/MDd) for Hermes' objects — but Rust's own linked output always expects the release CRT (/MD), regardless of--release. Forced.profile("Release")for the vendored build unconditionally; it's not something downstream crates ever edit, so building it unoptimized just to match the outer dev profile bought nothing.Missing Windows ICU/Winmm linkage. "Using Windows 10 built-in ICU" in the CMake log just means Hermes calls the same
unorm2_*/ucol_*/udat_*/u_strTo*entry points ICU4C exposes, backed by the OS'sicu.dll— it doesn't get baked intohermesvm_a.lib. Windows' import lib for it is namedicu, noticuuc/icui18n/icudata. Also neededwinmmfortimeBeginPeriod/timeEndPeriod(sampling profiler).Verified with a minimal crate depending on this branch:
cargo buildsucceeds and the resulting binary evaluates JS correctly (1 + 2->3).