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

Fix comparing SYSTEM_PROCESSOR with win32. #4374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BillyONeal
Copy link
Member

This is going to be a processor type like x86, x64, arm, or arm64.

This change was originally authored by @LilyWangLL in microsoft/vcpkg#39475

See https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html

This is going to be a processor type like x86, x64, arm, or arm64.

This change was originally authored by @LilyWangLL in microsoft/vcpkg#39475
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.49%. Comparing base (8a741dc) to head (14ab90a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4374      +/-   ##
==========================================
+ Coverage   85.46%   85.49%   +0.02%     
==========================================
  Files          56       56              
  Lines       15426    15426              
==========================================
+ Hits        13184    13188       +4     
+ Misses       2242     2238       -4     

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

@nibanks
Copy link
Member

nibanks commented Jun 24, 2024

@BillyONeal this broke several of our builds in automation

@BillyONeal
Copy link
Member Author

It looks like the values in the repo are broken for non-VS generators, while this change is broken for VS generators. I suppose it can just check for either.

I observe that there's a checked in .pgd file with unusual linker settings getting dragged in here too

@BillyONeal
Copy link
Member Author

BillyONeal commented Jun 25, 2024

Ah, I see. This is a bit o_O:

msquic/CMakeLists.txt

Lines 438 to 442 in 8a741dc

if (CMAKE_GENERATOR_PLATFORM STREQUAL "")
string(TOLOWER ${CMAKE_SYSTEM_PROCESSOR} SYSTEM_PROCESSOR)
else()
string(TOLOWER ${CMAKE_GENERATOR_PLATFORM} SYSTEM_PROCESSOR)
endif()

@nibanks Would you like to see a change to consistently compare with CMAKE_SYSTEM_PROCESSOR values or always check for either?

I think the right thing to do is to always supply CMAKE_SYSTEM_PROCESSOR values given the name SYSTEM_PROCESSOR like this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3f8f4d58f..343b3cf47 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -435,10 +435,17 @@ if (NOT MSVC AND NOT APPLE AND NOT ANDROID)
     endif()
 endif()
 
-if (CMAKE_GENERATOR_PLATFORM STREQUAL "")
-string(TOLOWER ${CMAKE_SYSTEM_PROCESSOR} SYSTEM_PROCESSOR)
+if ("${CMAKE_GENERATOR_PLATFORM}" STREQUAL "")
+    string(TOLOWER "${CMAKE_SYSTEM_PROCESSOR}" SYSTEM_PROCESSOR)
 else()
-string(TOLOWER ${CMAKE_GENERATOR_PLATFORM} SYSTEM_PROCESSOR)
+    string(TOLOWER "${CMAKE_GENERATOR_PLATFORM}" SYSTEM_PROCESSOR)
+    # Translate from Visual Studio generator platform values to
+    # CMAKE_SYSTEM_PROCESSOR values (derived from %PROCESSOR_ARCHITECTURE%)
+    if ("${SYSTEM_PROCESSOR}" STREQUAL "win32")
+        set(SYSTEM_PROCESSOR "x86")
+    elseif ("${SYSTEM_PROCESSOR}" STREQUAL "x64")
+        set(SYSTEM_PROCESSOR "amd64")
+    endif()
 endif()
 
 if(WIN32)

But the existing behavior e.g.

elseif (${SYSTEM_PROCESSOR} STREQUAL "x64" OR ${SYSTEM_PROCESSOR} STREQUAL "amd64")

checks for both x64 (which would be the CMAKE_GENERATOR_PLATFORM value) and amd64 (which is the CMAKE_SYSTEM_PROCESSOR value).

(And if that seems funny and inconsistent, I'm sorry:

Microsoft Windows [Version 10.0.22631.3737]
(c) Microsoft Corporation. All rights reserved.

D:\>echo %PROCESSOR_ARCHITECTURE%
AMD64

D:\>C:\Windows\Syswow64\cmd.exe
Microsoft Windows [Version 10.0.22631.3737]
(c) Microsoft Corporation. All rights reserved.

D:\>echo %PROCESSOR_ARCHITECTURE%
x86

D:\>

)

@BillyONeal
Copy link
Member Author

If it helps make the choice, I observe that

$/src/bin/winuser/pgo_x64 is used by Visual Studio but not Ninja generators, but $/src/bin/winuser/pgo_x86 is used by Ninja but not Visual Studio. (And the latter is 3 years old)

BillyONeal added a commit to microsoft/vcpkg that referenced this pull request Jun 25, 2024
Also fix uwp builds so that all ci.baseline.txt entries can be removed,
submitted upstream as microsoft/msquic#4373
Also fix x86-windows builds which incorrectly compared SYSTEM_PROCESSOR
with 'win32' rather than 'x86'. Submitted upstream as
microsoft/msquic#4374 . This patch originally
authored by @LillyWangLL

Originally started from #39475

Co-authored by: Lily Wang <v-lilywang@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants