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

Enhance the build experience of envoy. #415

Closed
fengli79 opened this issue Feb 3, 2017 · 33 comments
Closed

Enhance the build experience of envoy. #415

fengli79 opened this issue Feb 3, 2017 · 33 comments
Assignees
Milestone

Comments

@fengli79
Copy link
Contributor

fengli79 commented Feb 3, 2017

Today build envoy from github is not trivial. And it's even harder to build its tests.
Shall we?

  1. Add envoy dependencies as its submodules?
  2. Bazel support?
  3. Single command to build?
@lizan
Copy link
Member

lizan commented Feb 3, 2017

If you can use docker to build, it is not hard and just one command to build and test:
https://lyft.github.io/envoy/docs/install/building.html

We have bazel BUILD files in https://github.com/istio/proxy , @mattklein123 are you open to merge it into envoy repo?

@lizan lizan mentioned this issue Feb 3, 2017
@rshriram
Copy link
Member

rshriram commented Feb 3, 2017

Bazel support does not enhance build experience in any manner (not from our experience so far!). As @lizan pointed out, there is already a simple one line docker build command, and the docker image comes with all the dependencies installed in it.

@lizan
Copy link
Member

lizan commented Feb 3, 2017

We have been using bazel in istio/proxy for a while, and I do think bazel enhanced the build experience, especially for attaching debuggers, working with tools doesn't work well with docker. It provides dependency resolution without changing the system environment.

@Reflejo
Copy link
Contributor

Reflejo commented Feb 5, 2017

IMHO what needs to be done here is to fix the CMakeLists.txt to take care of dependencies. That way compiling from the repo would be as easy as setting your CXX/CC and

$ cmake .. && make

or even

$ cmake -GNinja .. && ninja

I have a working branch that I'm planning to push when I have some time to polish

@fengli79
Copy link
Contributor Author

fengli79 commented Feb 9, 2017

@Reflejo , agreed, the deps is the most painful part.
Can we add those deps as submodules of envoy if they are available on github?

@mattklein123
Copy link
Member

Per offsite meeting, here is I think where we landed here:

  • We don't mind multiple build systems, but we need a single source of truth.
  • It's possible we can look at gRPC build system to see how they do both bazel and cmake.
  • We are open to switching to Bazel IFF it doesn't make any of the existing flows worse and does not make CI any longer than it already is (given how slow Travis is and the fact that right now we don't have any other options).

@htuch I believe is going to take the lead on a proposal/investigation here. Going to assign to him.

@rshriram
Copy link
Member

rshriram commented Feb 26, 2017 via email

@fengli79
Copy link
Contributor Author

fengli79 commented Feb 26, 2017 via email

@htuch
Copy link
Member

htuch commented Feb 27, 2017

I'll start looking into this during the week. I agree that submodules could be an idea to consider, but I don't think we want to be in the business of building every single transitive dependency, even if this works today, this will put future constraints on what dependencies we can add feasibly. Envoy might have a wider set of dependencies than the gRPC core libraries which use this approach.

Whatever the outcome, the Docker build will be maintained - this is necessary for CI and some developers (including me) use this as their primary workflow. It's also orthogonal to the choice of the use of (i) submodules (ii) build system (iii) IDE, we should always be able to encapsulate the build flow in a Docker image.

@htuch
Copy link
Member

htuch commented Feb 28, 2017

jamessynge@ mentions that it would also be useful to not build all tests as the only option.

@htuch
Copy link
Member

htuch commented Mar 7, 2017

Latest discussion on this and a proposal is being tracked at https://docs.google.com/a/google.com/document/d/1XG1F38tZXTaWlrDXZFNoH-0xVm0d4HlrZJ4JXbMWqHo/edit?usp=sharing

@daedric
Copy link

daedric commented Mar 8, 2017

Also my +1 for a better dependencies management. Using find_package() should be the first step and submodules even better so you can still have a full static build.

It would ease up a lot setup'ing dev environment.

@mattklein123 mattklein123 modified the milestone: 1.3.0 Mar 9, 2017
@htuch
Copy link
Member

htuch commented Mar 15, 2017

I started to look at adding bazel to the Docker image as the first step in Bazelifying Envoy.

Following the instructions at https://bazel.build/versions/master/docs/install.html#ubuntu led me to installing Oracle JDK 8, as Ubuntu Trusty doesn't have OpenJDK 8 support as a starter. This comes with a big scary license http://www.oracle.com/technetwork/java/javase/terms/license/index.html, IANAL and unclear if this will fly for Docker redistribution by Lyft.

It'll probably be more sane to move to a newer Ubuntu version rather than explore unofficial OpenJDK ports, does this sound sane @mattklein123?

@lizan
Copy link
Member

lizan commented Mar 15, 2017

@htuch Are you trying on Ubuntu 15.10 or 14.04? AFAIK 16.04 with OpenJDK has no problem with bazel.

@htuch
Copy link
Member

htuch commented Mar 15, 2017

This is 14.04, the version of Ubuntu used for the Docker image. If folks are on-board, I will upgrade it to something more recent like 15.10 or 16.04 where there is OpenJDK 8 support.

@mattklein123
Copy link
Member

16.04 should be fine. If possible can we still build with GCC 4.9? I think this is available on 16.04 pretty easily, though from my own machine, there might be some build issues (I have no idea why this would be the case but I recall potentially having an issue when trying), but let's give it a shot.

htuch added a commit to htuch/envoy that referenced this issue Mar 15, 2017
mattklein123 pushed a commit that referenced this issue Mar 15, 2017
htuch added a commit to htuch/envoy that referenced this issue Mar 16, 2017
This patch introduces the minimal Bazel infrastructure required to build and execute utility_test,
as a step towards conversion of the cmake build system to Bazel (envoyproxy#415). The main contributions are:

* Machinery for declaring Envoy C++ library and test targets (envoy_cc_library, envoy_cc_test).

* External dependency management that works both inside the Docker CI environment (using prebuilts)
  and with developer-local dependency fetch/build (based on envoyproxy#416).

* Handling of the implicit dependencies that are today sourced via prebuilts.

* Simple example of building and running a unit test with only its dependencies built. With the
  cmake system, we would need to build all of Envoy and all tests to run utility_test. E.g.

  blaze test //test/common/common:utility_test

This is not intended to be used by anyone other than those working with the Bazel conversion at this
point. The plan is to add "ci/do_ci.sh bazel.debug" as a Travis target to ensure we don't bit rot.
mattklein123 pushed a commit that referenced this issue Mar 17, 2017
* Minimal Bazel infrastructure (#415).

This patch introduces the minimal Bazel infrastructure required to build and execute utility_test,
as a step towards conversion of the cmake build system to Bazel (#415). The main contributions are:

* Machinery for declaring Envoy C++ library and test targets (envoy_cc_library, envoy_cc_test).

* External dependency management that works both inside the Docker CI environment (using prebuilts)
  and with developer-local dependency fetch/build (based on #416).

* Handling of the implicit dependencies that are today sourced via prebuilts.

* Simple example of building and running a unit test with only its dependencies built. With the
  cmake system, we would need to build all of Envoy and all tests to run utility_test. E.g.

  blaze test //test/common/common:utility_test

This is not intended to be used by anyone other than those working with the Bazel conversion at this
point. The plan is to add "ci/do_ci.sh bazel.debug" as a Travis target to ensure we don't bit rot.
@htuch
Copy link
Member

htuch commented Mar 20, 2017

We should add support to generate a textual representation of the build dependency graph once done. This will provide a sanity check that we could in future support other build systems and also provide support to those who want to carry out proprietary integrations. @mattwoodyard.

jmillikin-stripe added a commit to jmillikin-stripe/bazel that referenced this issue Nov 6, 2017
This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes bazelbuild#2840

See envoyproxy/envoy#415 for additional
background and context.
jmillikin-stripe added a commit to jmillikin-stripe/bazel that referenced this issue Nov 12, 2017
This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes bazelbuild#2840

See envoyproxy/envoy#415 for additional
background and context.
jmillikin-stripe added a commit to jmillikin-stripe/bazel that referenced this issue Dec 4, 2017
This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes bazelbuild#2840

See envoyproxy/envoy#415 for additional
background and context.
jmillikin-stripe added a commit to jmillikin-stripe/bazel that referenced this issue Dec 4, 2017
This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes bazelbuild#2840

See envoyproxy/envoy#415 for additional
background and context.
bazel-io pushed a commit to bazelbuild/bazel that referenced this issue Dec 12, 2017
This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)

I'm only doing this in the Linux toolchain because MacOS doesn't do static linking of system libs anyway, and I don't know enough about Windows or FreeBSD to test on those platforms.

Closes #4031.

PiperOrigin-RevId: 178762357
bazel-io pushed a commit to bazelbuild/bazel that referenced this issue Jan 19, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 19, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 23, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 24, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 26, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 26, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 30, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Jan 30, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
iirina pushed a commit to bazelbuild/bazel that referenced this issue Feb 12, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: #4474)

Fixes #4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes #2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
werkt pushed a commit to werkt/bazel that referenced this issue Mar 2, 2018
*** Reason for rollback ***

Breaks C++ on gcc 4.8.4 (specifically, TensorFlow: bazelbuild#4474)

Fixes bazelbuild#4474

*** Original change description ***

When linking mostly-static Linux binaries, link libstdc++.a explicitly.

This allows libstdc++ to be statically linked, which is normally only
possible when invoking GCC as `g++` with the `-static-libstdc++` flag.

Fixes bazelbuild#2840

See envoyproxy/envoy#415 for additional
background and context.

cc @htuch (for Envoy) and @calpeyser @hlopko (who I talked to earlier about this)...

***

RELNOTES: None.
PiperOrigin-RevId: 182519445
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
Automatic merge from submit-queue.

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

This PR will be merged automatically once checks are successful.
```release-note
none
```
wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this issue Jan 23, 2021
* zh-translation: /configuration/listeners/stats.rst

* zh-translation: /configuration/listeners/stats.rst, some minor fix according to feadback

* Fixed typo

* Fixed some more formats

* Translation adjustment
jpsim pushed a commit that referenced this issue Nov 28, 2022
Description: clean up streams in the http dispatcher when the underlying stream fires an onReset callback. Previously we were leaking this state.
Risk Level: med - stream memory management changes. However, we have a guarantee from the underlying async client implementation that terminal callbacks (onReset, onComplete) will only fire once.
Testing: unit tests.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
Description: clean up streams in the http dispatcher when the underlying stream fires an onReset callback. Previously we were leaking this state.
Risk Level: med - stream memory management changes. However, we have a guarantee from the underlying async client implementation that terminal callbacks (onReset, onComplete) will only fire once.
Testing: unit tests.

Signed-off-by: Jose Nino <jnino@lyft.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
Projects
None yet
Development

No branches or pull requests

7 participants