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 issues with Win32 / x86 compilation #847

Merged
merged 9 commits into from
Jun 15, 2021
Merged

Fix issues with Win32 / x86 compilation #847

merged 9 commits into from
Jun 15, 2021

Conversation

maxgolov
Copy link
Contributor

@maxgolov maxgolov commented Jun 11, 2021

Fixes #842

NOTE: I have only fixed and tested the essential common core parts. There could be other fixes necessary for Win32 in the optional parts that are not planned for inclusion in v1.0 GA. These should be covered separately by setting up a build loop for Win32, which we do not have in CI. We only test x64 at the moment.

Changes

  • code fix for proper type cast.
  • detect arch and invoke the installation of missing deps for the proper architecture (by default it was installing deps for x64-windows , but if CMake is targeting Win32 - we need to install for x86-windows).
  • gRPC example uses some funky target that is not even applicable; looks like a bug, and I'm not sure why it slipped thru the cracks before. Found it while testing x86 build.

@maxgolov maxgolov requested a review from a team June 11, 2021 00:12
CMakeLists.txt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #847 (65526a5) into main (fa2caa8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   95.49%   95.49%   +0.01%     
==========================================
  Files         156      156              
  Lines        6617     6618       +1     
==========================================
+ Hits         6318     6319       +1     
  Misses        299      299              
Impacted Files Coverage Δ
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <100.00%> (ø)

CMakeLists.txt Outdated
if(CMAKE_SIZEOF_VOID_P EQUAL 8)
# 64 bits
set(ARCH x64)
elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check target is Intel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree we should.. My point was mostly that if someone builds for non-Intel (arm, risc, ppc), they'd have to specify the ARCH variable manually. Since this variable is currently only being used for Windows dependencies and/or with vcpkg, possible supported architectures are [x86, x64, arm, arm64]. I can try to improve this to handle Intel and ARM, but with a disclaimer above that any other non-Intel and non-ARM architectures should go thru if(DEFINED ENV{ARCH}) path above. Let me think it over.

Copy link
Contributor Author

@maxgolov maxgolov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomsonTan - I carefully reviewed the options. The challenge is that on Windows - CMake does not necessarily properly populate the target architecture. This is described in CMake documentation:

image

Best we can come up with:

  • assume that users know what arch they are compiling for. And if this is ARM - on Windows they must manually specify the "exotic" target arch using environment variable, such as set ARCH=arm or set ARCH=arm64 , which will then be respected by the rest of logic that installs dependencies via vcpkg.

  • if users didn't specify the arch, then most likely they are building for Intel / AMD64 , nothing exotic.. And generic arch lookup would look like this (bulky!). This is much smarter than what I had done before, but doesn't really add much additional benefit..

if(DEFINED ENV{ARCH})
  # Architecture may be specified via ARCH environment variable
  set(ARCH $ENV{ARCH})
else()
  # Autodetection logic that populates ARCH environment variable
  if(CMAKE_SYSTEM_PROCESSOR MATCHES "amd64.*|x86_64.*|AMD64.*")
    # Windows may report AMD64 even if target is 32-bit
    if(CMAKE_SIZEOF_VOID_P EQUAL 8)
      # 64 bits
      set(ARCH x64)
    elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
      # 32 bits
      set(ARCH x86)
    endif()
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "i686.*|i386.*|x86.*")
    set(ARCH x86)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(aarch64.*|AARCH64.*|arm64.*|ARM64.*)")
    set(ARCH arm64)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(arm.*|ARM.*)")
    set(ARCH arm)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(powerpc|ppc)64le")
    set(ARCH ppc64le)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(powerpc|ppc)64")
    set(ARCH ppc64)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(mips.*|MIPS.*)")
    set(ARCH mips)
  elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(riscv.*|RISCV.*)")
    set(ARCH riscv)
  else()
    message(FATAL_ERROR "opentelemetry-cpp: unrecognized target processor configuration!")
  endif()
endif()
message(STATUS "Building for architecture: ARCH=${ARCH}")

The only benefit is if we ever want to auto-install dependencies via vcpkg on Linux or Mac, for example, then auto-detecting the target triplet comes in handy. It's a bit of an overkill. This PR for now is for Win32 only. And I don't see a simpler solution. I looked at how Samsung did in in their repo. For Windows they just assume that users manually specify the target arch by parameter, e.g. pretty much as in the first if-condition I had before.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@maxgolov
Copy link
Contributor Author

maxgolov commented Jun 11, 2021

@ThomsonTan - I added a bit smarter logic, that should reliably cover Intel/AMD-Windows and ALL-Linux/-Mac.

Re. ARM cross-compile caveat ... My assumption for arm and arm64 cross-compiling on Windows:

  • if developers compile using tools/build.cmd , then it respects the ARCH set via environment variable, which subsequently gets passed down to CMake. The script also accepts the ARCH as 1st parameter. So no special logic needed in CMakeLists.txt, it'd bypass auto-detection.

  • if developers use Visual Studio 2019 IDE with CMake to cross-compile on Windows for ARM/ARM64, then they can add ARCH=ARM64 variable to CMakeSettings.json using environments section. I also have an expectation that even if they don't do that, the new logic that I added MAY work with newer CMake - assuming that newer CMake is smarter with setting up the proper CMAKE_SYSTEM_PROCESSOR - this is their bug, where they are discussing options of fixing it properly.

There is another even more smart way, to compile a test program - just to determine what arch we are compiling for - based on compiled code features, the program can output the architecture type. But that'd be on overkill. For what it's worth - passing an env var ARCH explicitly always solves any possible ambiguity about what target ARCH we are compiling for.

endif()
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES
"^(aarch64.*|AARCH64.*|arm64.*|ARM64.*)")
set(ARCH arm64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are adding this, should we also be checking for 32 bit OS running on 64bit ARM, and set ARCH accordingly?

Copy link
Contributor Author

@maxgolov maxgolov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The issue with unreliable arch detection is unique to Windows x86/x64 (MSVC) only. These are Linux-reported architectures. And Linux and Mac are always Okay - truthfully report the actual target system processor, even when cross-compiling with gcc or clang.

I am not aware of developers building our SDK on Windows-ARM64 host machine, while targeting x86/x64 intel target. I am not sure even if such MSVC toolset (ARM host, Intel target) exists? If those developers using ARM64 host as their developer box hypothetically do exist somewhere, then they may need to explicitly specify the ARCH environment variable. That'd solve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows ARM64 have x86/x64 MSVC run under emulation. There is no native ARM64 MSVC as for now.

example_grpc_proto
protobuf::libprotobuf
gRPC::grpc++
gRPC::grpc++_reflection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is grpc++_reflection removed?

Copy link
Contributor Author

@maxgolov maxgolov Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is no such package / target / library, at least not in vcpkg build of gRPC. It fails to compile because of this target. Can you point me where this target is declared?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be mistaken on this one. But it all compiles without this target. I see where it is mentioned in gRPC build. But I had an issue with that one on Windows.. we do build all examples across all CI loops for CMake?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a library target as below. I think we build all examples in our CMake CI. We probably don't use use the reflection function in gRPC.

https://github.com/grpc/grpc/blob/master/CMakeLists.txt#L3146

Copy link
Member

@lalitb lalitb Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all examples are built so hopefully should be fine :) Though I faintly remember getting link error without this library, but all good if it builds fine in CI.

@lalitb lalitb merged commit 7fca2c3 into main Jun 15, 2021
@lalitb lalitb deleted the maxgolov/win32_fix branch June 15, 2021 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

circular_buffer.h fails to compile on Win32 because of a conversion from uint64_t to size_t
3 participants