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

Jump to bottom

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 all commits
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
46 changes: 45 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,46 @@ 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 may be specified via ARCH environment variable
set(ARCH $ENV{ARCH})
else()
# Autodetection logic that populates ARCH 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)
set(ARCH x64)
elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
set(ARCH x86)
endif()
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "i686.*|i386.*|x86.*")
# Windows may report x86 even if target is 64-bit
if(CMAKE_SIZEOF_VOID_P EQUAL 8)
set(ARCH x64)
elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
set(ARCH x86)
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 •
edited
Loading

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.

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}")

# Autodetect vcpkg toolchain
if(DEFINED ENV{VCPKG_ROOT} AND NOT DEFINED CMAKE_TOOLCHAIN_FILE)
set(CMAKE_TOOLCHAIN_FILE
Expand Down Expand Up @@ -144,8 +184,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