Skip to content

Hack to allow conditionally compiling out lightstep#982

Closed
tschroed wants to merge 5 commits intoenvoyproxy:masterfrom
tschroed:lightstep_optional
Closed

Hack to allow conditionally compiling out lightstep#982
tschroed wants to merge 5 commits intoenvoyproxy:masterfrom
tschroed:lightstep_optional

Conversation

@tschroed
Copy link
Copy Markdown
Contributor

Compiling in lightstep also brings in the opensource version of protobuf. Unfortunately, this introduces link time conflicts with (nearly) the same library in Google when bringing in Google dependencies.

We're working on that but it's going to take some time. Issue 967 tracks implementation of a generalized registry which provides a more elegant solution.

Comment thread source/server/configuration_impl.cc Outdated
#include "common/tracing/http_tracer_impl.h"
#ifndef DISABLE_LIGHTSTEP_TRACING
#include "common/tracing/lightstep_tracer_impl.h"
+#endif // DISABLE_LIGHTSTEP_TRACING
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

:/ It would seem that I patched over from g3 but didn't actually build. I am cleaning this up and a couple of other things.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Seems legit given the #967 plan FTW.

Comment thread source/server/BUILD Outdated
srcs = ["configuration_impl.cc"],
hdrs = ["configuration_impl.h"],
copts = select({
"//bazel:disable_lightstep_tracing": ["-DDISABLE_LIGHTSTEP_TRACING"],
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 May 17, 2017

Choose a reason for hiding this comment

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

nit: This is where I would put the hack TODO comment. If we do registration correctly, we should not need any defines. We are going to have to have selectors no matter what to compile different code.

Comment thread source/common/tracing/BUILD Outdated
"//bazel:disable_lightstep_tracing": [],
"//conditions:default": ["lightstep_tracer_impl.h"],
}),
external_deps = ["lightstep"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

select here too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"select" is actually a type that's implemented by skylark. I haven't figured out how (or if) to get at its contents from within build extensions. Obviously we'll need to figure it out long term, I just haven't done that here.

Copy link
Copy Markdown
Member

@lizan lizan May 18, 2017

Choose a reason for hiding this comment

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

Is it possible to separate lightstep_tracer_impl.{cc,h} as a separate cc_library and use select in deps?
It will help our use case in istio proxy so we don't need build lightstep at all, cc #840

Trevor Schroeder added 2 commits May 18, 2017 17:06
gets pruned because it's not used, but that's pretty icky. Let's not
bother building at all.
@htuch
Copy link
Copy Markdown
Member

htuch commented May 22, 2017

@tschroed Can we drop this given #994?

@tschroed
Copy link
Copy Markdown
Contributor Author

Let's close this for now. I'll bug @mrice32 about adding the appropriate bazel conditionals to the build system to select lightstep and zipkin.

@tschroed tschroed closed this May 22, 2017
@RomanDzhabarov RomanDzhabarov self-assigned this May 22, 2017
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
Automatic merge from submit-queue.

[DO NOT MERGE] Auto PR to update dependencies of proxy

This PR will be merged automatically once checks are successful.
```release-note
none
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
This has a few small changes

1. Always apply `--incompatible_objc_compile_info_migration` so that
   when you switch between configurations this doesn't force bazel to
   reanalyze all targets
2. Use consistent `--define` `=` syntax

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
This has a few small changes

1. Always apply `--incompatible_objc_compile_info_migration` so that
   when you switch between configurations this doesn't force bazel to
   reanalyze all targets
2. Use consistent `--define` `=` syntax

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants