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.

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

Failed to build with external abseil-cpp #1065

Closed
hcoona opened this issue Nov 15, 2021 · 7 comments · Fixed by #1172
Closed

Failed to build with external abseil-cpp #1065

hcoona opened this issue Nov 15, 2021 · 7 comments · Fixed by #1172
Assignees
Labels
bug Something isn't working

Comments

@hcoona
Copy link
Contributor

hcoona commented Nov 15, 2021

It's simple to understand so I didn't follow the template.

The opentelemetry-cpp vendor parts of the absl source code within the API but without change the namespace. If I build a cpp target included both opentelementry-cpp & abseil-cpp, it would occurred an error (build with bazel & llvm-13):

external/com_google_absl/absl/meta/type_traits.h:162:8: error: redefinition of 'is_detected_convertible_impl<typename std::enable_if<std::is_convertible<Op<Args...>, To>::value>::type, To, Op, Args...>'
struct is_detected_convertible_impl<
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bazel-out/k8-fastbuild/bin/external/io_opentelemetry_cpp/api/_virtual_includes/api/opentelemetry/nostd/./absl/types/../types/internal/../../meta/type_traits.h:154:8: note: previous definition is here
struct is_detected_convertible_impl<
       ^

The critical rule is to use an alternative namespace than the original namespace absl when vendoring any source code.

@hcoona hcoona added the bug Something isn't working label Nov 15, 2021
@hcoona
Copy link
Contributor Author

hcoona commented Nov 15, 2021

Still failing after add --copt -DHAVE_ABSEIL=1 to .bazelrc

In file included from external/io_opentelemetry_cpp/exporters/ostream/test/ostream_span_test.cc:17:
In file included from external/com_google_googletest/googletest/include/gtest/gtest.h:62:
In file included from external/com_google_googletest/googletest/include/gtest/internal/gtest-internal.h:40:
In file included from external/com_google_googletest/googletest/include/gtest/internal/gtest-port.h:2299:
In file included from external/com_google_absl/absl/types/any.h:57:
In file included from external/com_google_absl/absl/utility/utility.h:49:
In file included from external/com_google_absl/absl/base/internal/inline_variable.h:20:
external/com_google_absl/absl/base/internal/identity.h:26:8: error: redefinition of 'identity'
struct identity {
       ^
bazel-out/k8-fastbuild/bin/external/io_opentelemetry_cpp/api/_virtual_includes/api/opentelemetry/nostd/absl/types/../types/internal/../../base/internal/identity.h:26:8: note: previous definition is here
struct identity {
       ^

@lalitb
Copy link
Member

lalitb commented Nov 15, 2021

Does change the order of inclusion of header file helps as documented here: https://github.com/open-telemetry/opentelemetry-cpp/blob/main/examples/otlp/README.md#additional-notes-regarding-abseil-library

If not, please let us know the version of abseil-cpp used externally?

@hcoona
Copy link
Contributor Author

hcoona commented Nov 16, 2021

I didn't try changing the order. It's ridiculous to make it work relying about including order, and our coding guideline disallow manually ordering the includes.

The abseil-cpp commit id is 3c8b5d7 from master branch.

@lalitb
Copy link
Member

lalitb commented Nov 16, 2021

Agree, I still believe -DHAVE_ABSEIL=ON should work in this case. If not, this needs to be fixed.

@lalitb lalitb self-assigned this Nov 22, 2021
@lalitb
Copy link
Member

lalitb commented Dec 1, 2021

Tested with below cmake command, and the build and tests were successful:

$ cmake -DWITH_ABSEIL=ON -DCMAKE_PREFIX_PATH=/home/labhas/custom_abseil/ .. && make -j14

Looks like the problem is specific to bazel. Need to test more.

@owent
Copy link
Member

owent commented Dec 28, 2021

We meet the same problem and I solve this problem by add modify HAVE_ABSEIL to api/BUILD and the move the paths of internal abseil-cpp.
This issue can assign to me and I can try to merge the changes here and add some tests some days later. @lalitb

@lalitb lalitb assigned owent and unassigned lalitb Dec 28, 2021
@lalitb
Copy link
Member

lalitb commented Dec 28, 2021

Thanks, @owent. Assigned to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants