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

Jaeger exporter fails to build with Thrift 0.14.1 as bswap_* is actually a macro #875

Closed
lidavidm opened this issue Jun 24, 2021 · 5 comments · Fixed by #876
Closed

Jaeger exporter fails to build with Thrift 0.14.1 as bswap_* is actually a macro #875

lidavidm opened this issue Jun 24, 2021 · 5 comments · Fixed by #876
Labels
bug Something isn't working

Comments

@lidavidm
Copy link
Contributor

Describe your environment

Ubuntu 18.04 x86_64, OpenTelemetry main @ ae6d810
Thrift 0.14.1 from conda-forge

$ uname -a
Linux partita 5.4.0-74-generic #83~18.04.1-Ubuntu SMP Tue May 11 16:01:00 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
$ cmake --version
cmake version 3.19.6
$ g++ --version
g++ (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0

Steps to reproduce

Build with -DWITH_JAEGER=ON:

> mkdir build2 && cd build2
> cmake .. -DBUILD_TESTING=OFF -GNinja -DWITH_JAEGER=ON 
-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is GNU 9.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/lidavidm/miniconda3/envs/arrow4/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/lidavidm/miniconda3/envs/arrow4/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building for architecture ARCH=x64
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE  
Using external nlohmann::json
Building with nostd types...
-- Found CURL: /home/lidavidm/miniconda3/envs/arrow4/lib/libcurl.so (found version "7.75.0")  
-- Found thrift: /home/lidavidm/miniconda3/envs/arrow4
-- Configuring done
-- Generating done
-- Build files have been written to: /home/lidavidm/Code/upstream/opentelemetry-cpp/build2
> ninja 
[23/62] Building CXX object exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/jaeger_exporter.cc.o
FAILED: exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/jaeger_exporter.cc.o 
/home/lidavidm/miniconda3/envs/arrow4/bin/c++  -I../api/include -I../sdk/include -I../sdk -I../ext/include -I../exporters/jaeger/thrift-gen -I../exporters/jaeger/include -isystem /home/lidavidm/miniconda3/envs/arrow4/include/thrift/.. -isystem /home/lidavidm/miniconda3/envs/arrow4/include -std=gnu++11 -MD -MT exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/jaeger_exporter.cc.o -MF exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/jaeger_exporter.cc.o.d -o exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/jaeger_exporter.cc.o -c ../exporters/jaeger/src/jaeger_exporter.cc
In file included from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/endian.h:61,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/sys/types.h:220,
                 from /home/lidavidm/miniconda3/envs/arrow4/include/thrift/Thrift.h:30,
                 from ../exporters/jaeger/thrift-gen/agent_types.h:12,
                 from ../exporters/jaeger/src/jaeger_exporter.cc:4:
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before '__extension__'
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected ')' before '__extension__'
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before ')' token
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
[27/62] Building CXX object exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/thrift_sender.cc.o
FAILED: exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/thrift_sender.cc.o 
/home/lidavidm/miniconda3/envs/arrow4/bin/c++  -I../api/include -I../sdk/include -I../sdk -I../ext/include -I../exporters/jaeger/thrift-gen -I../exporters/jaeger/include -isystem /home/lidavidm/miniconda3/envs/arrow4/include/thrift/.. -isystem /home/lidavidm/miniconda3/envs/arrow4/include -std=gnu++11 -MD -MT exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/thrift_sender.cc.o -MF exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/thrift_sender.cc.o.d -o exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/thrift_sender.cc.o -c ../exporters/jaeger/src/thrift_sender.cc
In file included from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/endian.h:61,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/ctype.h:41,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/include/c++/9.3.0/cctype:42,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/include/c++/9.3.0/bits/localefwd.h:42,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/include/c++/9.3.0/string:43,
                 from /home/lidavidm/miniconda3/envs/arrow4/include/thrift/TProcessor.h:23,
                 from /home/lidavidm/miniconda3/envs/arrow4/include/thrift/TDispatchProcessor.h:22,
                 from ../exporters/jaeger/thrift-gen/Agent.h:10,
                 from ../exporters/jaeger/src/thrift_sender.h:6,
                 from ../exporters/jaeger/src/thrift_sender.cc:4:
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before '__extension__'
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected ')' before '__extension__'
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before ')' token
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
[29/62] Building CXX object exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/recordable.cc.o
FAILED: exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/recordable.cc.o 
/home/lidavidm/miniconda3/envs/arrow4/bin/c++  -I../api/include -I../sdk/include -I../sdk -I../ext/include -I../exporters/jaeger/thrift-gen -I../exporters/jaeger/include -isystem /home/lidavidm/miniconda3/envs/arrow4/include/thrift/.. -isystem /home/lidavidm/miniconda3/envs/arrow4/include -std=gnu++11 -MD -MT exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/recordable.cc.o -MF exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/recordable.cc.o.d -o exporters/jaeger/CMakeFiles/jaeger_trace_exporter.dir/src/recordable.cc.o -c ../exporters/jaeger/src/recordable.cc
In file included from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/endian.h:61,
                 from /home/lidavidm/miniconda3/envs/arrow4/x86_64-conda-linux-gnu/sysroot/usr/include/sys/types.h:220,
                 from /home/lidavidm/miniconda3/envs/arrow4/include/thrift/Thrift.h:30,
                 from ../exporters/jaeger/thrift-gen/jaeger_types.h:12,
                 from ../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:6,
                 from ../exporters/jaeger/src/recordable.cc:4:
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before '__extension__'
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected ')' before '__extension__'
../exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h:32:17: error: expected unqualified-id before ')' token
   32 | inline uint64_t bswap_64(uint64_t host_int)
      |                 ^~~~~~~~
[40/62] Building CXX object examples/simple/CMakeFiles/example_simple.dir/main.cc.o
ninja: build stopped: subcommand failed.

What is the expected behavior?

OpenTelemetry should build.

What is the actual behavior?

OpenTelemetry fails to build since bswap_64 is actually a macro that gets expanded.

Additional context

From what I can see, opentelemetry/exporters/jaeger/recordable.h includes jaeger_types.h. That includes thrift/TBase.h, which in turn includes thrift/protocol/TProtocol.h. At least on my system, this then includes byteswap.h which defines bswap_64 as a macro (note I am using a Conda environment). This then breaks the definition in OpenTelemetry's headers. Probably there should be an #ifdef guard (though it's rather unfortunate that Thrift leaks its definitions like this).
@lidavidm lidavidm added the bug Something isn't working label Jun 24, 2021
@lalitb
Copy link
Member

lalitb commented Jun 24, 2021

Interesting. It works fine in our CI with Thrift 0.14.1. Not sure, but maybe it's because of different version of libc6-dev which brings byteswap.h. This is the libc6-dev version installed in my setup where build is successful:

$ dpkg -s libc6-dev:amd64 | grep Version
Version: 2.31-0ubuntu9.2

Your suggestion looks good - to create the bswap_64 function only if the macro is not defined. Or maybe rename the function to something unique.

lidavidm added a commit to lidavidm/opentelemetry-cpp that referenced this issue Jun 24, 2021
@lidavidm
Don't define bswap_64 if already defined (open-telemetry#875)
44ce549
@ThomsonTan
Copy link
Contributor

I'd prefer we rename it with some OpenTelemetry prefix, in case the system provided one doesn't provide the required semantics for us. I could take care of this.

@lidavidm
Copy link
Contributor Author

Ah, thanks, I was going to put up a PR with an #ifndef guard, but I realize that won't work since then you can't use it as opentelemetry::exporter::jaeger::bswap_64 which at least the tests use.

@lidavidm
Copy link
Contributor Author

And sorry, it's because my libc comes from conda sysroot_linux-64-2.12-h77966d4_13, though my OS libc6-dev does a similar thing:

> dpkg -s libc6-dev:amd64 | grep Version
Version: 2.27-3ubuntu1.4
> rg bswap_64 /usr/include/
/usr/include/x86_64-linux-gnu/bits/byteswap.h
109:__bswap_64 (__uint64_t __bsx)
114:#  define __bswap_64(x) \
123:#  define __bswap_64(x) \
149:__bswap_64 (__uint64_t __bsx)

/usr/include/byteswap.h
37:#define bswap_64(x) __bswap_64 (x)

@ThomsonTan
Copy link
Contributor

@lidavidm could you rename the function name to either opentelemetry_bswap_64 or otel_bswap_64 to avoid naming conflict with macro name from system header?

I thought about it when adding it and put it under namespace which could avoid any potential link conflict, but missed the macro expansion.

lidavidm added a commit to lidavidm/opentelemetry-cpp that referenced this issue Jun 24, 2021
@lidavidm
Rename bswap_64 to otel_bswap_64 to avoid clash with macro (open-tele…
2f49203 
…metry#875)
@lidavidm lidavidm mentioned this issue Jun 24, 2021
Merged
1 task
lalitb pushed a commit that referenced this issue Jun 24, 2021
@lidavidm
Rename bswap_64 to otel_bswap_64 to avoid clash with macro (#875) (#876)
0a3320e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rename bswap_64 to otel_bswap_64 to avoid clash with macro (#875) lidavidm/opentelemetry-cpp
3 participants
@lidavidm @ThomsonTan @lalitb