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

build: switch to libc++ by default #8859

Merged
merged 19 commits into from
Nov 6, 2019
Merged

build: switch to libc++ by default #8859

merged 19 commits into from
Nov 6, 2019

Conversation

lizan
Copy link
Member

@lizan lizan commented Nov 1, 2019

Signed-off-by: Lizan Zhou [email protected]

Description:
Risk Level: Med
Testing: CI
Docs Changes: N/A
Release Notes: Added
Fixes #4251

@PiotrSikora
Copy link
Contributor

What about this one?

# TODO(cmluciano) fix and re-enable _LIBCPP_VERSION testing for TCMALLOC in Envoy::Stats::TestUtil::hasDeterministicMallocStats
# and update stats_integration_test with appropriate m_per_cluster value

@mattklein123 mattklein123 self-assigned this Nov 4, 2019
@lizan
debug
7660ca4 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
debug
7d43192 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
debug
43be1d7 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
tests only
12a54de 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
Copy link
Member Author

lizan commented Nov 4, 2019

cc @yevgenypats seems fuzzit docker image doesn't have libc++ installed, while libc++ is the default C++ standard library used in oss-fuzz. Is it possible to add libc++ there?

@lizan
revert debug
feffa2a 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
Copy link
Member Author

lizan commented Nov 4, 2019

@mattklein123 this ready for review except fuzzit

mattklein123
mattklein123 reviewed Nov 4, 2019
View reviewed changes
ci/build_setup.sh
@@ -30,7 +30,7 @@ function setup_clang_toolchain() {
echo "clang toolchain configured"
}

function setup_clang_libcxx_toolchain() {
function setup_clang_toolchain() {
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is there any way to make this the default in the core .bazelrc options such that someone could disable it and go back to libstdcxx if they want? This would provide consistency across all builds in general, not just CI builds. I'm not sure if there is a good way to do this though?

/wait-any

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I will revise the script.

@lizan lizan added the waiting label Nov 5, 2019
@lizan
docs
1c4b8a7 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
fix
68c5f0c 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
fix
98ce9d9 
Signed-off-by: Lizan Zhou <[email protected]>
@yevgenypats
Copy link
Contributor

@lizan fuzzit can use any docker you specify. Is there an image that you would like the fuzzers to run on?

@lizan
Copy link
Member Author

lizan commented Nov 5, 2019

@yevgenypats then just use envoyproxy/envoy-build-ubuntu? which doesn't need extra container pull.

@lizan
Copy link
Member Author

lizan commented Nov 5, 2019

@yevgenypats actually that's already the container that we're running fuzzit, so I guess you can just run without container too?

@yevgenypats
Copy link
Contributor

@lizan it can't run without a container but it can be passed via --host envoyproxy/envoy-build-ubuntu . should I push this change?

@lizan
Copy link
Member Author

lizan commented Nov 5, 2019

ok let me try that

@yevgenypats
Copy link
Contributor

it should be added in both fuzzit create job lines (both regression and in the cloud)

mattklein123
mattklein123 reviewed Nov 5, 2019
View reviewed changes
bazel/README.md
```
export CC=clang
export CXX=clang++
bazel build --config=libc++ //source/exe:envoy-static
Copy link
Member

Choose a reason for hiding this comment

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

I think what I was wondering before is whether it's possible to just have --config-libc++ be the default?

The issue that I have found in the past is that I don't think there is a way to have a default config and then actual have a local config disable it and replace it with something else?

/wait-any

Copy link
Member Author

Choose a reason for hiding this comment

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

I will recommend having --config=libc++ in user.bazelrc file to make it default. It is possible to override but not easy, partially because toolchains are also defaulted to libstdc++.

Copy link
Member

Choose a reason for hiding this comment

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

OK, should we document that somewhere?

mattklein123
mattklein123 requested changes Nov 6, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice, some small comments.

/wait

docs/root/intro/version_history.rst Outdated
@@ -3,6 +3,7 @@ Version history

1.13.0 (pending)
================
* build: official released binary is now built against libc++.
Copy link
Member

Choose a reason for hiding this comment

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

alpha order

ci/README.md Outdated

As of November 2019 after [#8859](https://github.com/envoyproxy/envoy/pull/8859) the official released binary is
[linked against libc++ on Linux](https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#linking-against-libc-on-linux).
To override the C++ standard library in your build, set environment variable `ENVOY_STDLIB` to `libstdc++` or `libc++`.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being dense but if we have bazelrc configs, why do we also need env variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah this is for ci/do_ci.sh to inject bazelrc configs, let me make this clear.

@lizan
docs
90af0f8 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan lizan marked this pull request as ready for review November 6, 2019 18:21
mattklein123
mattklein123 approved these changes Nov 6, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice!

@lizan
Copy link
Member Author

lizan commented Nov 6, 2019

/azp run envoy-linux

@mattklein123 mattklein123 merged commit c2eae09 into envoyproxy:master Nov 6, 2019
sthagen added a commit to sthagen/envoyproxy-envoy that referenced this pull request Nov 7, 2019
@sthagen
Merge pull request #49 from envoyproxy/master
dd7829a 
build: switch to libc++ by default (envoyproxy#8859)
@lizan lizan deleted the libcpp_default branch June 2, 2020 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Link against libc++ in the official builds.
4 participants
@lizan @PiotrSikora @yevgenypats @mattklein123