Fix issues with `Win32` / `x86` compilation #847

maxgolov · 2021-06-11T00:12:45Z

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.

CMakeLists.txt

codecov · 2021-06-11T00:21:59Z

Codecov Report

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

@@            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%> (ø)`

ThomsonTan · 2021-06-11T00:39:29Z

CMakeLists.txt

+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    # 64 bits
+    set(ARCH x64)
+  elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)


Also check target is Intel?

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.

@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:

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.

lalitb

LGTM.

…y/opentelemetry-cpp into maxgolov/win32_fix

maxgolov · 2021-06-11T17:51:52Z

@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.

lalitb · 2021-06-11T18:12:02Z

CMakeLists.txt

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


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

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.

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

…y-cpp into maxgolov/win32_fix

…y/opentelemetry-cpp into maxgolov/win32_fix

ThomsonTan · 2021-06-14T23:14:06Z

examples/grpc/CMakeLists.txt

-    example_grpc_proto
-    protobuf::libprotobuf
-    gRPC::grpc++
-    gRPC::grpc++_reflection


Why is grpc++_reflection removed?

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?

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?

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

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.

Fix issues with Win32 / x86 compilation

a7d3757

maxgolov requested a review from a team June 11, 2021 00:12

maxgolov commented Jun 11, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ThomsonTan reviewed Jun 11, 2021

View reviewed changes

lalitb approved these changes Jun 11, 2021

View reviewed changes

maxgolov added 4 commits June 11, 2021 10:22

Merge branch 'main' into maxgolov/win32_fix

f0758a9

Address code review comments

cff5008

Add smarter detection logic + formatting

136c211

Merge branch 'maxgolov/win32_fix' of https://github.com/open-telemetr…

f7137bb

…y/opentelemetry-cpp into maxgolov/win32_fix

lalitb reviewed Jun 11, 2021

View reviewed changes

maxgolov mentioned this pull request Jun 11, 2021

circular_buffer.h fails to compile on Win32 because of a conversion from uint64_t to size_t #842

Closed

maxgolov added 3 commits June 11, 2021 11:43

Merge branch 'main' into maxgolov/win32_fix

22b7ff9

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

fbb27b3

…y-cpp into maxgolov/win32_fix

Merge branch 'maxgolov/win32_fix' of https://github.com/open-telemetr…

5406491

…y/opentelemetry-cpp into maxgolov/win32_fix

maxgolov mentioned this pull request Jun 14, 2021

Windows compile with CMake fails - Can't find googletest or nlohmann_json #843

Closed

Merge branch 'main' into maxgolov/win32_fix

65526a5

ThomsonTan reviewed Jun 14, 2021

View reviewed changes

ThomsonTan approved these changes Jun 14, 2021

View reviewed changes

lalitb merged commit 7fca2c3 into main Jun 15, 2021

lalitb deleted the maxgolov/win32_fix branch June 15, 2021 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with `Win32` / `x86` compilation #847

Fix issues with `Win32` / `x86` compilation #847

maxgolov commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading

ThomsonTan Jun 11, 2021

maxgolov Jun 11, 2021

maxgolov Jun 11, 2021 •

edited

Loading

lalitb left a comment

maxgolov commented Jun 11, 2021 •

edited

Loading

lalitb Jun 11, 2021

maxgolov Jun 11, 2021 •

edited

Loading

ThomsonTan Jun 14, 2021

ThomsonTan Jun 14, 2021

maxgolov Jun 14, 2021 •

edited

Loading

maxgolov Jun 14, 2021

ThomsonTan Jun 14, 2021

lalitb Jun 15, 2021 •

edited

Loading

Fix issues with Win32 / x86 compilation #847

Fix issues with Win32 / x86 compilation #847

Conversation

maxgolov commented Jun 11, 2021 • edited Loading

Changes

codecov bot commented Jun 11, 2021 • edited Loading

Codecov Report

ThomsonTan Jun 11, 2021

Choose a reason for hiding this comment

maxgolov Jun 11, 2021

Choose a reason for hiding this comment

maxgolov Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

lalitb left a comment

Choose a reason for hiding this comment

maxgolov commented Jun 11, 2021 • edited Loading

lalitb Jun 11, 2021

Choose a reason for hiding this comment

maxgolov Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

ThomsonTan Jun 14, 2021

Choose a reason for hiding this comment

ThomsonTan Jun 14, 2021

Choose a reason for hiding this comment

maxgolov Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

maxgolov Jun 14, 2021

Choose a reason for hiding this comment

ThomsonTan Jun 14, 2021

Choose a reason for hiding this comment

lalitb Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Fix issues with `Win32` / `x86` compilation #847

Fix issues with `Win32` / `x86` compilation #847

maxgolov commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading

maxgolov Jun 11, 2021 •

edited

Loading

maxgolov commented Jun 11, 2021 •

edited

Loading

maxgolov Jun 11, 2021 •

edited

Loading

maxgolov Jun 14, 2021 •

edited

Loading

lalitb Jun 15, 2021 •

edited

Loading