Skip to content

Fix build test of Zipkin exporter on Windows #1177

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

Merged
lalitb merged 3 commits into from
Jan 20, 2022
Merged

Fix build test of Zipkin exporter on Windows #1177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e3e406
Fix build test of Zipkin exporter
ThomsonTan Jan 18, 2022
da139cd
Fix format
ThomsonTan Jan 18, 2022
a1a7e08
Specify an old good version for docfx as the latest version has issue to
ThomsonTan Jan 19, 2022
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ jobs:
steps:
- uses: actions/checkout@v2
- name: install docfx
run: choco install docfx -y
run: choco install docfx -y --version=2.58.5
- name: run ./ci/docfx.cmd
shell: cmd
run: ./ci/docfx.cmd
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,5 @@
/bazel-*
/plugin
/build

tags
1 change: 1 addition & 0 deletions api/include/opentelemetry/common/spin_lock_mutex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# ifndef NOMINMAX
# define NOMINMAX
# endif
# define _WINSOCKAPI_ // stops including winsock.h
# include <windows.h>
#elif defined(__i386__) || defined(__x86_64__)
# if defined(__clang__)
Expand Down
3 changes: 2 additions & 1 deletion exporters/jaeger/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ install(
PATTERN "recordable.h" EXCLUDE)

if(BUILD_TESTING)
add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1)

add_executable(jaeger_recordable_test test/jaeger_recordable_test.cc)
target_link_libraries(
jaeger_recordable_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
Expand All @@ -69,7 +71,6 @@ if(BUILD_TESTING)
TEST_LIST jaeger_recordable_test)

add_executable(jaeger_exporter_test test/jaeger_exporter_test.cc)
add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1)
if(MSVC)
if(GMOCK_LIB)
unset(GMOCK_LIB CACHE)
Expand Down
2 changes: 2 additions & 0 deletions exporters/zipkin/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ install(
PATTERN "recordable.h" EXCLUDE)

if(BUILD_TESTING)
add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1)
Copy link
Copy Markdown
Member

@lalitb lalitb Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this required ? Just trying to understand what fix we are going for Zipkin.

Copy link
Copy Markdown
Contributor Author

@ThomsonTan ThomsonTan Jan 20, 2022

Choose a reason for hiding this comment

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

There is some unresolved global lock variable in gtest/gmock reported if built without this flag.


add_executable(zipkin_recordable_test test/zipkin_recordable_test.cc)

target_link_libraries(
Expand Down
1 change: 0 additions & 1 deletion exporters/zipkin/test/zipkin_exporter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

#ifndef HAVE_CPP_STDLIB

# define _WINSOCKAPI_ // stops including winsock.h
Copy link
Copy Markdown
Member

@esigo esigo Jan 19, 2022

Choose a reason for hiding this comment

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

Not related to the PR:
This is not part of CMake test for Windows CI.

Copy link
Copy Markdown
Contributor Author

@ThomsonTan ThomsonTan Jan 20, 2022

Choose a reason for hiding this comment

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

I moved this define to spin lock header which includes windows.h as well. So this should be unnecessary. Let me know you prefer keep this define here.

Copy link
Copy Markdown
Member

@esigo esigo Jan 20, 2022

Choose a reason for hiding this comment

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

thanks, spin lock header is actually the right place to have the define.

# include "opentelemetry/exporters/zipkin/zipkin_exporter.h"
# include <string>
# include "opentelemetry/ext/http/client/curl/http_client_curl.h"
Expand Down
3 changes: 2 additions & 1 deletion ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "opentelemetry/ext/http/client/http_client.h"
#include "opentelemetry/version.h"

#include <curl/curl.h>
#include <future>
#include <map>
#include <regex>
Expand All @@ -16,9 +15,11 @@
#include <vector>
#ifdef _WIN32
# include <io.h>
# include <winsock2.h>
#else
# include <unistd.h>
#endif
#include <curl/curl.h>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace ext
Expand Down