Skip to content

[cpuinfo] Fix cpuid-dump is not generated with x86-windows#24726

Closed
Cheney-W wants to merge 2 commits intomicrosoft:masterfrom
Cheney-W:Dev/Cheney/cpuinfo
Closed

[cpuinfo] Fix cpuid-dump is not generated with x86-windows#24726
Cheney-W wants to merge 2 commits intomicrosoft:masterfrom
Cheney-W:Dev/Cheney/cpuinfo

Conversation

@Cheney-W
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

    Fixes [cpuinfo] Build error #24713

    1. Modify portfile.cmake to pass the correct values.
    2. Add a condition to make the option to use the value passed by portfile.cmake under windows.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:internal labels May 16, 2022
@Cheney-W Cheney-W marked this pull request as ready for review May 17, 2022 00:15
@LilyWangLL LilyWangLL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label May 17, 2022
@BillyONeal
Copy link
Member

Can you explain what this patch does? Has upstream been notified?

(Normally I wouldn't have questions like that about what look like relatively simple build system changes, but this port is literally cpuinfo so it likely cares a lot about the specific CPU it wants to target)

@BillyONeal BillyONeal added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels May 17, 2022
@Cheney-W
Copy link
Contributor Author

I added this patch to make following part set in portfile.cmake take effect in cmakelist.txt of source:

set(CPUINFO_TARGET_PROCESSOR_param "")
if(VCPKG_TARGET_IS_WINDOWS)
    # NOTE: arm64-windows is unsupported for now;
    # see https://github.com/pytorch/cpuinfo/pull/82 for updates
    # NOTE: arm-windows is unsupported
    if(VCPKG_TARGET_ARCHITECTURE STREQUAL "x86")
        set(CPUINFO_TARGET_PROCESSOR_param "-DCPUINFO_TARGET_PROCESSOR=i386")
    elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL "x64")
        set(CPUINFO_TARGET_PROCESSOR_param "-DCPUINFO_TARGET_PROCESSOR=AMD64")
    elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL "arm")
        set(CPUINFO_TARGET_PROCESSOR_param "-DCPUINFO_TARGET_PROCESSOR=ARM")
    elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL "arm64")
        set(CPUINFO_TARGET_PROCESSOR_param "-DCPUINFO_TARGET_PROCESSOR=ARM64")
    endif()
endif()

If the target is windows, CPUINFO_TARGET_PROCESSOR_param will be passed as an option to CMakeLists.txt of source by vcpkg_cmake_configure(), this option sets different values for CPUINFO_TARGET_PROCESSOR under different architectures.

But CMakeLists.txt of source have following content:
SET(CPUINFO_TARGET_PROCESSOR "${CMAKE_SYSTEM_PROCESSOR}")
This causes the value of CPUINFO_TARGET_PROCESSOR always be ${CMAKE_SYSTEM_PROCESSOR} even if the target is window.

If cpuinfo install with x86-windows, the value of ${CMAKE_SYSTEM_PROCESSOR} will be x86, this leads to a false result in the following judgment in CMakeLists.txt of source:

  IF(CPUINFO_TARGET_PROCESSOR MATCHES "^(i[3-6]86|AMD64|x86_64)$")
    ADD_EXECUTABLE(cpuid-dump tools/cpuid-dump.c)
    CPUINFO_TARGET_ENABLE_C99(cpuid-dump)
    CPUINFO_TARGET_RUNTIME_LIBRARY(cpuid-dump)
    TARGET_INCLUDE_DIRECTORIES(cpuid-dump BEFORE PRIVATE src)
    TARGET_INCLUDE_DIRECTORIES(cpuid-dump BEFORE PRIVATE include)
    INSTALL(TARGETS cpuid-dump RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
  ENDIF()

This is also the reason why cpuid-dump is not generated under x86-windows.
To solve this problem, I originally planned two scenarios:

  1. Modify the part of the copy tool in the portfile.cmake so that it does not copy cpuid-dump under x86.
  2. Modify the value of CPUINFO_TARGET_PROCESSOR to i386 when triplet is x86-windows, and add judgment to make CPUINFO_TARGET_PROCESSOR use the value passed by vcpkg_cmake_configure instead of ${CMAKE_SYSTEM_PROCESSOR}.

Because there is no documentation that cpuid-dump is not available under x86, I used scheme 2.

@BillyONeal
Copy link
Member

@myd7349 @luncliff Do you have any comments on this patch and/or how we should let the upstream owners know?

@Cheney-W Cheney-W marked this pull request as draft May 25, 2022 10:35
@Cheney-W Cheney-W marked this pull request as ready for review June 15, 2022 02:10
@LilyWangLL LilyWangLL requested a review from BillyONeal June 15, 2022 06:33
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Jun 15, 2022
@BillyONeal
Copy link
Member

@LilyWangLL My point about upstream needing to be involved stands. However, I did my own digging here and am proposing a competing resolution here, which upstream has already OK'd: #25258

@Cheney-W
Copy link
Contributor Author

Dup to #25258

@Cheney-W Cheney-W closed this Jun 16, 2022
BillyONeal added a commit that referenced this pull request Jun 16, 2022
@Cheney-W Cheney-W deleted the Dev/Cheney/cpuinfo branch August 30, 2022 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[cpuinfo] Build error

3 participants