Repository navigation
Define _WINDOWS only when the target is Windows - #2291
Closed
YevheniiKotyrlo wants to merge 1 commit into
Closed
YevheniiKotyrlo wants to merge 1 commit into
YevheniiKotyrlo wants to merge 1 commit into
Conversation
CMakeLists.txt tested WIN32 before project(), where it still holds what CMake set for the host, so a build for Android from a Windows host defined _WINDOWS, and the sources that read it compiled their Windows code: ProcessStats.cpp includes windows.h and OSCompat.h io.h, which the NDK does not have. Test it after project(), which resets WIN32 for the target. build.yml's new build-android-from-windows job configures for Android on the Windows runner and builds hermesSupport and hermesInstrumentation, whose OSCompat.h and ProcessStats.cpp include Windows headers under _WINDOWS, so the test cannot move back ahead of project() unnoticed. Fixes meta#2279
YevheniiKotyrlo
marked this pull request as ready for review
November 5, 2026 10:57
|
@lavenzg has imported this pull request. If you are a Meta employee, you can view this in D129958773. |
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
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.
Summary
CMakeLists.txttestsWIN32beforeproject(), where it still holds what CMake set for the host (cmStateSnapshot::SetDefaultDefinitions);project()resets it for the target (CMakeSystemSpecificInitialize.cmake). So a build for Android from a Windows host defines_WINDOWS, and the sources that read it compile their Windows code:OSCompat.hincludesio.handProcessStats.cppincludeswindows.h, which the NDK does not have. React Native's source build of Hermes for Android fails this way on Windows.This tests
WIN32afterproject(), so_WINDOWSis defined exactly when the target is Windows. A newbuild.ymljob,build-android-from-windows, configures for Android on the Windows runner and buildshermesSupportandhermesInstrumentation, whoseOSCompat.handProcessStats.cppinclude Windows headers under_WINDOWS, so the test cannot move ahead ofproject()again unnoticed.Fixes #2279
Test Plan
The new job's step, run on Windows 11 as the runner runs it, with NDK 30.0.16248370, the runner's
ANDROID_NDK_LATEST_HOME, and CMake 4.1.2 where the runner has 3.31.6:static_h's tree at fbdd137 it fails:Hermes's Android build for arm64-v8a on Windows 11, with JDK 17.0.20.1 and the Android SDK's CMake 4.1.2 and NDK 30.0.16248370, the newest installed, in place of the 3.31.6 and 28.2.13676358 that
libs.versions.tomlpins:--project-cache-dirgets these commits' build past #2286.static_hat fbdd137: all 762 compile commands of arm64-v8a'sbuild.ninjacarry-D_WINDOWS, and the build fails:libhermesvm.so, anELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, for Android 24, built by NDK r30 (16248370):With the same change applied to hermes-v260318099.0.2, React Native 0.88.0's source build of Hermes for Android completes on Windows.
Configured for Windows with
build.yml's Visual Studio flags (CMake 4.4.4) and for Linux (CMake 4.2.3 on Ubuntu 26.04), every target gets the same definitions before and after this change,_WINDOWSincluded on Windows; configured for Windows from Linux with mingw-w64, it now gets_WINDOWSas well.The other
build.ymlandrn-build-hermes.ymljobs a pull request runs pass on this branch locally, the macOS ones with Xcode 27.2 where the workflows pin 26.6 and 16.4 and the Linux ones on Ubuntu 26.04;test-linux-test262onubuntu-24.04-arm, with and without the JIT,test-linux-armv7andtest-windows-test262onwindows-11-armare unverified, since I have no arm64 Linux or Windows machine.