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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a7d3757
Fix issues with Win32 / x86 compilation
maxgolov Jun 11, 2021
f0758a9
Merge branch 'main' into maxgolov/win32_fix
maxgolov Jun 11, 2021
cff5008
Address code review comments
maxgolov Jun 11, 2021
136c211
Add smarter detection logic + formatting
maxgolov Jun 11, 2021
f7137bb
Merge branch 'maxgolov/win32_fix' of https://github.com/open-telemetr…
maxgolov Jun 11, 2021
22b7ff9
Merge branch 'main' into maxgolov/win32_fix
maxgolov Jun 11, 2021
fbb27b3
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
maxgolov Jun 11, 2021
5406491
Merge branch 'maxgolov/win32_fix' of https://github.com/open-telemetr…
maxgolov Jun 11, 2021
65526a5
Merge branch 'main' into maxgolov/win32_fix
maxgolov Jun 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,21 @@ project(opentelemetry-cpp)
# Mark variables as used so cmake doesn't complain about them
mark_as_advanced(CMAKE_TOOLCHAIN_FILE)

if(DEFINED ENV{ARCH})
# Architecture specified manually via env var, e.g. ARM64
set(ARCH $ENV{ARCH})
else()
# Architecture detection logic for Intel chipsets only. For other
# architectures - please specify manually.
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 •
edited
Loading

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.

# 32 bits
set(ARCH x86)
endif()
endif()

# Autodetect vcpkg toolchain
if(DEFINED ENV{VCPKG_ROOT} AND NOT DEFINED CMAKE_TOOLCHAIN_FILE)
set(CMAKE_TOOLCHAIN_FILE
Expand Down Expand Up @@ -112,7 +127,9 @@ if(WIN32)
add_definitions(-DNOMINMAX)
if(BUILD_TESTING)
if(MSVC)
# GTest bug: https://github.com/google/googletest/issues/860
# Uncomment for strict Level4 warnings if building for test:
# set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4") GTest bug:
maxgolov marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/google/googletest/issues/860
add_compile_options(/wd4275)
endif()
endif()
Expand Down Expand Up @@ -144,8 +161,12 @@ find_package(Threads)

function(install_windows_deps)
# Bootstrap vcpkg from CMake and auto-install deps in case if we are missing
# deps on Windows
# deps on Windows. Respect the target architecture variable.
set(VCPKG_TARGET_ARCHITECTURE
${ARCH}
PARENT_SCOPE)
message("Installing build tools and dependencies...")
set(ENV{ARCH} ${ARCH})
execute_process(
COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/tools/setup-buildtools.cmd)
set(CMAKE_TOOLCHAIN_FILE
Expand Down
9 changes: 2 additions & 7 deletions examples/grpc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,7 @@ endif()
foreach(_target client server)
add_executable(${_target} "${_target}.cpp")
target_link_libraries(
${_target}
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 •
edited
Loading

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 •
edited
Loading

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.

opentelemetry_trace
opentelemetry_exporter_ostream_span)
${_target} example_grpc_proto protobuf::libprotobuf gRPC::grpc++
opentelemetry_trace opentelemetry_exporter_ostream_span)
patch_protobuf_targets(${_target})
endforeach()
9 changes: 5 additions & 4 deletions sdk/include/opentelemetry/sdk/common/circular_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,12 @@ class CircularBuffer
auto data = data_.get();
if (tail_index < head_index)
{
return CircularBufferRange<AtomicUniquePtr<T>>{
nostd::span<AtomicUniquePtr<T>>{data + tail_index, head_index - tail_index}};
return CircularBufferRange<AtomicUniquePtr<T>>{nostd::span<AtomicUniquePtr<T>>{
data + tail_index, static_cast<std::size_t>(head_index - tail_index)}};
}
return {nostd::span<AtomicUniquePtr<T>>{data + tail_index, capacity_ - tail_index},
nostd::span<AtomicUniquePtr<T>>{data, head_index}};
return {nostd::span<AtomicUniquePtr<T>>{data + tail_index,
static_cast<std::size_t>(capacity_ - tail_index)},
nostd::span<AtomicUniquePtr<T>>{data, static_cast<std::size_t>(head_index)}};
}
};
} // namespace common
Expand Down