Fix a compilation error for com_lightstep_tracer_cpp

[TrevorTaoARM]

When compiling the external repo com_lightstep_tracer_cpp on arm64
by GCC, it would show the following error:

com_lightstep_tracer_cpp/src/common/BUILD:173:1: C++ compilation of rule
'@com_lightstep_tracer_cpp//src/common:utility_lib' failed (Exit 1) gcc
failed: error executing command /usr/bin/gcc -U_FORTIFY_SOURCE
-fstack-protector -Wall -Wunused-but-set-parameter ...
external/com_lightstep_tracer_cpp/src/common/utility.cpp:76:20: error:
comparison is always true due to limited range of data type
[-Werror=type-limits]
if ('\x00' <= c && c <= '\x1f') {
~~~~~~~^~~~
cc1plus: all warnings being treated as errors
Target //source/exe:envoy-static failed to build

This warning is due to the explanation for 'char' type on arm64 is explained as
'unsigned char', but for x86_64, it's for 'signed char'.
More details about this warning could be found at:
https://stackoverflow.com/questions/757482/\
comparison-is-always-true-due-to-limited-range-of-data-type-warning-in-c

This patch fixes the above error by adding a forced 'signed-char' flag
to copts when building lightstep_tracer_cpp package which would force to
use the 'singed char' explanation to 'char' type.

Signed-off-by: Trevor Tao trevor.tao@arm.com

[snowp]

Did you look into upstreaming this to the lightstep code vs having to maintain a patch in this repo?

[TrevorTaoARM]

Did you look into upstreaming this to the lightstep code vs having to maintain a patch in this repo?

@snowp Now the Envoy uses a certain release (3efe2372ee3d7c2138d6b26e542d757494a7938d) of the lightstep_tracer_cpp package, and I think it would last for rather a long time to use this release.
For the commit to lightstep_tracer_cpp, it would need some time to be merged and may only available to the master branch. So it makes sense to commit the patch here in Envoy. On the other side, Envoy could just abandon this change when the patch is available in a certain future release, but not now.
Thanks!

[snowp]

I think we're already using a master commit to include a build fix that @lizan introduced a few months ago, it should be easy enough to bump the sha once we get it merged upstream. Could you give upstreaming this a shot and if it takes too long we can consider the patch approach?

[snowp]

cc @rnburn

[TrevorTaoARM]

I think we're already using a master commit to include a build fix that @lizan introduced a few months ago, it should be easy enough to bump the sha once we get it merged upstream. Could you give upstreaming this a shot and if it takes too long we can consider the patch approach?
@snowp I think we are still using a certain release of lighstep_tracer in bazel/repository_locations.bzl:
# 2020-03-24
urls = ["https://github.com/lightstep/lightstep-tracer-cpp/archive/3efe2372ee3d7c2138d6b26e542d757494a7938d.tar.gz"],
which was actually patched by @lizan, but not the master release.
I would like to upstream the patch to lightstep_tracer, but for our legal request and the upstream merging process, it may require a lot of time and only available for the master branch.

[lizan]

Yeah just send a PR to upstream lightstep. In short term you should be able to just pass it as --copt or --per_file_copt to bazel if you want to build with GCC on arm64.

[TrevorTaoARM]

Yeah just send a PR to upstream lightstep. In short term you should be able to just pass it as --copt or --per_file_copt to bazel if you want to build with GCC on arm64.

@lizan Yes, I would like to upstream it to lightstep, but it still need some extra time to do our internal/external process(legal, review ...). On the other side, it would greatly help our work on arm platform if it's available in the Envoy master branch, and it could be rewinded if it's available in upstream in the future.
Thanks!

[mattklein123]

@rnburn any chance of just having you get ^ patch upstream so we can consume it?

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[rnburn]

This was fixed with lightstep/lightstep-tracer-cpp#249

[moderation]

@rnburn should Envoy pull in the SHA with the fix or will there be a 0.14 release?