Support MSVC architectures

[ssrobins]

Use the value of CMAKE_GENERATOR_PLATFORM to determine the target architectures on Windows with MSVC since CMAKE_SYSTEM_PROCESSOR doesn't work as expected.

[jcar87]

Hi @ssrobins, thanks so much for this contribution!

This is excellent. Just one small comment, since version 3.27 (just released), CMAKE_GENERATOR_PLATFORM can contain more information (comma-separated key-value pairs) - so in those instances the STREQUAL would fail.
If CMAKE_VS_PLATFORM_NAME is already defined at this stage - I would probably inspect that variable instead, as that contains just the platform without the additional information.

Otherwise we would need to split the string to only extract the platform bit

[jcar87]

I've had a chance to review this further - indeed I can see that CMAKE_VS_PLATFORM_NAME is set after the call to project(), so it's probably safer to rely on this variable, in case CMAKE_GENERATOR_PLATFORM ever contains additional information (like the SDK version). Also thanks for providing a link to: https://gitlab.kitware.com/cmake/cmake/-/issues/15170 - looks like we may have to double check what happens when the ninja generator is used on Windows with msvc.

On the other hand, there's a potential bug that can occur in the following block, because CMAKE_SYSTEM_PROCESSOR is always defined at that stage:

    if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|ARM64|arm64" OR CMAKE_GENERATOR_PLATFORM STREQUAL ARM64)
        set(_ARCH armv8)
    elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "armv7-a|armv7l" OR CMAKE_GENERATOR_PLATFORM STREQUAL ARM)
        set(_ARCH armv7)
    elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "i686" OR CMAKE_GENERATOR_PLATFORM STREQUAL Win32)
        set(_ARCH x86)
    elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "AMD64|amd64|x86_64" OR CMAKE_GENERATOR_PLATFORM STREQUAL x64)
        set(_ARCH x86_64)
    endif()

if CMake is invoked on a Windows/ARM64 machine (like the 5G Surface Pro, Windows DevKit, etc) - it will unconditionally return ARM64 even if the platform target is a different architecture. At the moment the tests all pass successfully because it's running on x86_64 and that's the last architecture being evaluated.

Perhaps we can consider ignoring CMAKE_SYSTEM_PROCESSOR altogether when we know we are using the "Visual Studio" generator - or perhaps we can inspect CMAKE_CXX_COMPILER_ARCHITECTURE_ID (from this suggestion) - however that remains documented as an internal variable pending resolution of https://gitlab.kitware.com/cmake/cmake/-/issues/17702, even though it appears that for MSVC this is reliable.

[ssrobins]

@jcar87 I reconfigured it so it only uses one architecture variable per platform. I also switched to CMAKE_CXX_COMPILER_ARCHITECTURE_ID on Windows because it's effective for both Visual Studio and Ninja generators. Let me know what you think.

[ssrobins]

Just checking in on this one, in case it fell off your radar. If you're busy, no big deal.

[jcar87]

thanks a lot for the refactoring! just a couple more comments

[jcar87]

would it not make sense to check for MSVC instead? Based on the discussions in CMake, my understanding was that CMAKE_CXX_COMPILER_ARCHITECTURE_ID is well defined for msvc, but may not be for others - assuming other compilers may be used in Windows, like gcc

[ssrobins]

Yes, fixed

[jcar87]

why is AMD64 being removed? is it the case that MATCHES will match them regardless of lower-case/upper-case?

[ssrobins]

CMAKE_CXX_COMPILER_ARCHITECTURE_ID lists the arch as x64, but since CMAKE_SYSTEM_PROCESSOR could still be used for non-MSVC compilers on Windows, I put it back.

[ssrobins]

@jcar87 any other feedback on this one or is it ready to merge?

[memsharded]

Sorry that this has been stuck for a while, I'll sync with @jcar87 about this and try to move it forward, thanks for your patience!

[valgur]

Could this be merged, please? The patch works well (both x86 and ARM64 architectures pass CI now after applying it: https://github.com/valgur/novatel_edie/actions/runs/6660339585) and the current state is quite broken. IMHO, it's a straightforward-enough improvement and can be fine-tuned in follow-up PRs, if necessary.

[memsharded]

Ok, sorry about that.
I am merging this, if @jcar87 wants to review and revisit it later, that is always possible, this is still not an official, stable release, it is not a big risk, even if something was broken (not saying that, looking good).
Thanks again for this contribution!