Fix #724 #725

owent · 2021-05-05T06:18:51Z

Signed-off-by: owent admin@owent.net

Fixes #724

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2021-05-05T06:32:34Z

Codecov Report

Merging #725 (2f70ae2) into main (5914160) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #725   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         217      217           
  Lines        9923     9923           
=======================================
  Hits         9403     9403           
  Misses        520      520

Impacted Files	Coverage Δ
...include/opentelemetry/sdk/common/circular_buffer.h	`97.87% <0.00%> (-2.13%)`	⬇️
sdk/src/logs/batch_log_processor.cc	`95.00% <0.00%> (+1.25%)`	⬆️

ThomsonTan · 2021-05-05T06:39:59Z

exporters/elasticsearch/CMakeLists.txt

@@ -3,6 +3,9 @@ include_directories(${CMAKE_SOURCE_DIR}/ext/include)

 add_library(opentelemetry_exporter_elasticsearch_logs src/es_log_exporter.cc)

+target_link_libraries(opentelemetry_exporter_elasticsearch_logs


Seems this change is only for MinGW, is it needed for other toolchains?

It builds with msvc already, so I assume it's unique to MinGW?

Seems this change is only for MinGW, is it needed for other toolchains?

Yes. Some test targets use target_link_libraries to link additional dependencies which are also depended by exporters. I think the depended targets should be added automatically by linking the exporters, and users should not concern these targets unless they use these targets directly.

These PR mainly remove the linking targets from test targets and add these targets into the exporters.

It builds with msvc already, so I assume it's unique to MinGW?

I create a script toolset to build opentelemetry-cpp and here is the output when using MinGW64 https://github.com/atframework/cmake-toolset/runs/2492797467 .I'm trying to build all the dependencies myself or use my vcpkg instead of opentelemetry-cpp/tools/vcpkg to build opentelemetry-cpp .

owent · 2021-05-05T12:53:41Z

Don't know why coverage descrease. This PR has not change sdk/include/opentelemetry/sdk/metrics/controller.h and
sdk/src/logs/batch_log_processor.cc .

maxgolov · 2021-05-05T16:54:55Z

sdk/src/common/core.cc

@@ -2,6 +2,19 @@
 #include "opentelemetry/version.h"

 #if defined(HAVE_ABSEIL)
+
+#  if defined(__GNUC__) || defined(__GNUG__)


I am sorry for adding void __cdecl ThrowBadVariantAccess(). What you are doing here seems reasonable.

lalitb · 2021-05-05T19:40:25Z

Don't know why coverage descrease. This PR has not change sdk/include/opentelemetry/sdk/metrics/controller.h and
sdk/src/logs/batch_log_processor.cc .

You can ignore this as it is not related to PR. The coverage issue has occurred multiple times and needs to be looked at separately.

reyang · 2021-05-06T15:26:09Z

Is there a way to add MinGW in the CI?

owent

Is there a way to add MinGW in the CI?

Github hosted runner has mingw toolchain. Maybe we can use C:/msys64/msys2_shell.cmd -mingw64 -defterm -no-start -here -lc "ci/do_ci.sh cmake.test" to build on MinGW(e.g. https://github.com/atframework/cmake-toolset/blob/main/.github/workflows/build.yaml#L98), but I didn't test it.

owent · 2021-05-06T04:42:25Z

exporters/elasticsearch/CMakeLists.txt

@@ -3,6 +3,9 @@ include_directories(${CMAKE_SOURCE_DIR}/ext/include)

 add_library(opentelemetry_exporter_elasticsearch_logs src/es_log_exporter.cc)

+target_link_libraries(opentelemetry_exporter_elasticsearch_logs


Seems this change is only for MinGW, is it needed for other toolchains?

Yes. Some test targets use target_link_libraries to link additional dependencies which are also depended by exporters. I think the depended targets should be added automatically by linking the exporters, and users should not concern these targets unless they use these targets directly.

These PR mainly remove the linking targets from test targets and add these targets into the exporters.

owent · 2021-05-06T04:51:20Z

exporters/elasticsearch/CMakeLists.txt

@@ -3,6 +3,9 @@ include_directories(${CMAKE_SOURCE_DIR}/ext/include)

 add_library(opentelemetry_exporter_elasticsearch_logs src/es_log_exporter.cc)

+target_link_libraries(opentelemetry_exporter_elasticsearch_logs


It builds with msvc already, so I assume it's unique to MinGW?

I create a script toolset to build opentelemetry-cpp and here is the output when using MinGW64 https://github.com/atframework/cmake-toolset/runs/2492797467 .I'm trying to build all the dependencies myself or use my vcpkg instead of opentelemetry-cpp/tools/vcpkg to build opentelemetry-cpp .

lalitb · 2021-05-11T05:07:24Z

@owent - Can you update the branch with base, that will allows us to merge the PR

Signed-off-by: owent <admin@owent.net>

owent · 2021-05-11T06:52:11Z

@owent - Can you update the branch with base, that will allows us to merge the PR

@lalitb done.

owent requested a review from a team May 5, 2021 06:18

ThomsonTan reviewed May 5, 2021

View reviewed changes

owent force-pushed the fix_compiling_on_mingw64 branch from 75f60a4 to 6344d37 Compare May 5, 2021 06:56

maxgolov reviewed May 5, 2021

View reviewed changes

lalitb approved these changes May 6, 2021

View reviewed changes

owent commented May 7, 2021

View reviewed changes

ThomsonTan approved these changes May 10, 2021

View reviewed changes

Fix open-telemetry#724

2f70ae2

Signed-off-by: owent <admin@owent.net>

owent force-pushed the fix_compiling_on_mingw64 branch from 6344d37 to 2f70ae2 Compare May 11, 2021 06:19

lalitb merged commit 5278e8c into open-telemetry:main May 11, 2021

owent deleted the fix_compiling_on_mingw64 branch May 11, 2021 12:24

owent mentioned this pull request Nov 24, 2021

REQUEST: New membership for @owent open-telemetry/community#920

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #724 #725

Fix #724 #725

owent commented May 5, 2021 •

edited by reyang

Loading

codecov bot commented May 5, 2021 •

edited

Loading

ThomsonTan May 5, 2021 •

edited

Loading

maxgolov May 5, 2021 •

edited

Loading

owent May 6, 2021

owent May 6, 2021

owent commented May 5, 2021

maxgolov May 5, 2021 •

edited

Loading

lalitb commented May 5, 2021

reyang commented May 6, 2021

owent left a comment

owent May 6, 2021

owent May 6, 2021

lalitb commented May 11, 2021

owent commented May 11, 2021 •

edited

Loading

		@@ -3,6 +3,9 @@ include_directories(${CMAKE_SOURCE_DIR}/ext/include)

		add_library(opentelemetry_exporter_elasticsearch_logs src/es_log_exporter.cc)

		target_link_libraries(opentelemetry_exporter_elasticsearch_logs

Fix #724 #725

Fix #724 #725

Conversation

owent commented May 5, 2021 • edited by reyang Loading

Changes

codecov bot commented May 5, 2021 • edited Loading

Codecov Report

ThomsonTan May 5, 2021 • edited Loading

Choose a reason for hiding this comment

maxgolov May 5, 2021 • edited Loading

Choose a reason for hiding this comment

owent May 6, 2021

Choose a reason for hiding this comment

owent May 6, 2021

Choose a reason for hiding this comment

owent commented May 5, 2021

maxgolov May 5, 2021 • edited Loading

Choose a reason for hiding this comment

lalitb commented May 5, 2021

reyang commented May 6, 2021

owent left a comment

Choose a reason for hiding this comment

owent May 6, 2021

Choose a reason for hiding this comment

owent May 6, 2021

Choose a reason for hiding this comment

lalitb commented May 11, 2021

owent commented May 11, 2021 • edited Loading

owent commented May 5, 2021 •

edited by reyang

Loading

codecov bot commented May 5, 2021 •

edited

Loading

ThomsonTan May 5, 2021 •

edited

Loading

maxgolov May 5, 2021 •

edited

Loading

maxgolov May 5, 2021 •

edited

Loading

owent commented May 11, 2021 •

edited

Loading