build: support linking against libc++ on Linux. #5563

PiotrSikora · 2019-01-10T10:27:09Z

Signed-off-by: Piotr Sikora piotrsikora@google.com

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2019-01-10T10:38:20Z

bazel/unix_cc_configure.bzl is a copy of Bazel's tools/cpp/unix_cc_configure.bzl with changes to support auto-detection of libc++ (see: bazelbuild/bazel#7074).

I've added libc++ build to the compile_time_options target on the CI, which means that now all its dependencies are rebuilt for each PR (since they must be linked against c++), though it didn't affect build times.

cc @mattklein123 @htuch @ggreenway @lizan

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch

Is there a specific reason to do this the hacky way, before we have BAZEL_CXXOPTS? I.e. is this time sensitive? If not, I'd advocate not doing it, since it's extending the hard-to-maintain cc_configure.bzl fork, which wasn't great (but necessary) in the first place. CC @jmillikin-stripe

PiotrSikora · 2019-01-10T15:03:26Z

It's not time sensitive (from my point of view), so I'm fine with putting it on hold, but the "hacky way" is basically 7 lines of dispatch code in cc_configure.bzl and a local copy of unix_cc_configure.bzl, so I'm not sure if that's problematic enough to justify blocking this.

Also, CentOS support is blocked on libc++, though I don't particularly care about that. cc @lizan

htuch · 2019-01-10T15:53:24Z

@PiotrSikora will we have BAZEL_CXXOPTS at the next point release? This is only a couple of weeks away last time I checked.

PiotrSikora · 2019-01-10T16:05:44Z

@htuch I don't think so. I've opened bazelbuild/bazel#7074 a few hours ago, and I believe that the next release is already branched, so this will probably be available in February release (assuming it lands at all, since they are actively rewriting cc_configure nowadays).

jmillikin-stripe

I'm opposed to vendoring unix_cc_configure.bzl into Envoy. That file is not part of Bazel's public API and version skew against the version in @bazel_tools was historically a source of substantial build toil for Envoy.

I also don't understand why BAZEL_CXXOPTS is being used here, and am in general uncomfortable with using environment variables to smuggle critical configuration past the Bazel crosstool. Why can't this be done with --cxxopt=-stdlib=libc++?

PiotrSikora · 2019-01-10T18:29:40Z

@jmillikin-stripe BAZEL_CXXOPTS is being used when auto-detecting local CC toolchain, because you need to pass -stdlib=libc++ to $CC -E -xc++ - -v in order to get libc++ include paths, otherwise you get libstdc++ include paths, and no amount of --cxxopt and/or --linkopt is going to help you build against libc++, when your toolchain imports headers from different standard library, and libc++ headers are outside of Bazel sandbox.

See bazelbuild/bazel#7074.

lizan · 2019-01-10T19:51:44Z

It is not time sensitive for me either so I’m fine to hold it for a while. Is it possible to get around forked unix_configure by adding -stdlib=libc++ in our cc_wrapper.py conditionally?

PiotrSikora · 2019-01-12T16:15:08Z

@lizan even though it should, cc_wrapper is actually never executed for those calls...

htuch · 2019-01-14T04:22:44Z

@PiotrSikora so there is no other way to divert to our cc_wrapper.py? This provides an accessible low-level insertion point as well.

PiotrSikora · 2019-01-14T07:53:17Z

I've tried this again, and it turns out that my original test was wrong and cc_wrapper is being called after all (unix_cc_configure.bzl ignores repository_ctx.execute() exit status and continued with an empty response, so I assumed that cc_wrapper isn't called, but it was), so we can do it there.

I have something working, but it still needs CXXFLAGS="-stdlib=libc++" in the environment for the dependencies. I'll push it after some tests.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

…build

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2019-01-14T13:25:02Z

/retest

repokitteh-read-only · 2019-01-14T13:25:06Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5563 (comment) was created by @PiotrSikora.

see: more, trace.

jmillikin-stripe · 2019-01-14T16:06:27Z

bazel/cc_wrapper.py

@@ -6,6 +6,7 @@

 envoy_real_cc = {ENVOY_REAL_CC}
 envoy_real_cxx = {ENVOY_REAL_CXX}
+envoy_cxxflags = {ENVOY_CXXFLAGS}


This will change the content of the compiler wrapper when $CXXFLAGS changes, which will have significant negative impact on build caching.

When the crosstool bootstrap runs this script, does it propagate environment variables? If yes, then you can use envoy_cxxflags = os.environ.get("CXXFLAGS", "")

Well, that's kind of the point, isn't it? Same is true for changing $CC and $CXX.

Bazel won't cache when flags changed so I don't think it is negative anyway?

lizan · 2019-01-14T22:16:33Z

bazel/cc_wrapper.py

-        pass
+        if "-stdlib=libc++" in envoy_cxxflags:
+          arg.append("-lc++")
+        else:


you don't need else and pass.

I know, I've tried to be explicit that there are 2 cases we're supporting here. It's slightly more readable, IMHO, but I'm happy to remove it if you prefer it one way or another.

Prefer removing unless you want to add a comment as well explaining this, one of these should be done IMHO.

lizan · 2019-01-14T22:19:36Z

ci/build_container/build_recipes/gperftools.sh

@@ -15,4 +15,6 @@ tar xf gperftools-"$VERSION".tar.gz
 cd gperftools-"$VERSION"

 LDFLAGS="-lpthread" ./configure --prefix="$THIRDPARTY_BUILD" --enable-shared=no --enable-frame-pointers --disable-libunwind


Need -lc++ here if stdlib is libc++, it overrides existing LDFLAGS and will result failure when I tried to build prebuilt deps with libc++.

Huh, it works for me locally and on the CI.

How are you building that, exactly? Can you paste the command and/or environment you're building this in? Thanks!

I have slightly modified build_container_common.sh to pass CXXFLAGS/LDFLAGS to build the image, and only this one will fail to link (as it compiled with libc++ but not linking).

Does changing this to LDFLAGS="${LDFLAGS} -lpthread" fix your build?

Yes that would be good

Done, let me know if this works for you.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2019-01-15T03:20:20Z

@htuch this should be good to go, PTAL.

htuch

Thanks, just some minor nits, will merge when these are fixed unless @jmillikin-stripe has further objection.

htuch · 2019-01-16T05:49:10Z

bazel/cc_wrapper.py

+  if "-static-libstdc++" in sys.argv[1:] or "-stdlib=libc++" in envoy_cxxflags:
+    # Append CXXFLAGS to all C++ targets (this is mostly for dependencies).
+    if envoy_cxxflags and "-std=c++" in str(sys.argv[1:]):
+      argv = envoy_cxxflags.split(" ")


Should we be using shlex.split here?

It shouldn't matter for CXXFLAGS, but sure, changed.

htuch · 2019-01-16T05:49:48Z

bazel/cc_wrapper.py

-        pass
+        if "-stdlib=libc++" in envoy_cxxflags:
+          arg.append("-lc++")
+        else:


Prefer removing unless you want to add a comment as well explaining this, one of these should be done IMHO.

htuch · 2019-01-16T05:52:17Z

ci/build_setup.sh

+  # Force Bazel to link using lld, since GNU ld has trouble linking against libc++fs.
+  export BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS} --linkopt -fuse-ld=lld"
+  export BAZEL_TEST_OPTIONS="${BAZEL_TEST_OPTIONS} --linkopt -fuse-ld=lld"
+  echo "libc++ configured"


Meta-question; do we have upstream Bazel tracking for a better way to select libc++ vs. libstdc++? Even if we can do what is needed in this PR, I think Bazel should add much cleaner support to handle this, it should be a simple CLI switch..

I've added it to bazelbuild/bazel#6926, since it seems that cc_configure is undergoing major rewrite.

I'll also add this to our Bazel team biweekly sync, thanks.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch

LGTM, one last ask, can you add a section in https://github.com/envoyproxy/envoy/blob/master/bazel/README.md explaining how developers can enable this on the build CLI? Ready to merge after that.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2019-01-16T09:50:53Z

bazel/README.md

+```
+Note: this assumes that both: clang compiler and libc++ library are installed in the system,
+and that `clang` is available in `$PATH`. On some systems, this might need to be changed to
+`CC="clang-7"` and `CXX="clang++-7"`.


@htuch Let me know if this looks reasonable.

To be honest, I'm kind of tempted to put all those commands in .bazelrc and expose this as --config=libc++, ideally hardcoding CC=clang-7 and CXX=clang++-7, though it would make overriding compilers a bit difficult.

What do you think?

+1, I think --action_env=CXX=clang++-7 would work?

Yes, everything works in --action_env, so the whole thing would boil down to bazel build --config=libc++ //source/... //test/...

lizan · 2019-01-16T10:36:36Z

/retest

repokitteh-read-only · 2019-01-16T10:36:40Z

🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5563 (comment) was created by @lizan.

see: more, trace.

PiotrSikora · 2019-01-16T11:04:57Z

/retest

repokitteh-read-only · 2019-01-16T11:05:03Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5563 (comment) was created by @PiotrSikora.

see: more, trace.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Notably, tcmalloc_debug crashes without -ggdb3 -fno-omit-frame-pointer. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jmillikin-stripe

This looks reasonable to me.

htuch

Thanks!

Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

build: allow linking against libc++ on Linux.

f58755f

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora force-pushed the libcxx_build branch from 2bc9c35 to f58755f Compare January 10, 2019 10:28

review: update build image.

5ca7e8a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch suggested changes Jan 10, 2019

View reviewed changes

htuch assigned htuch and jmillikin-stripe Jan 10, 2019

htuch requested a review from jmillikin-stripe January 10, 2019 14:52

jmillikin-stripe suggested changes Jan 10, 2019

View reviewed changes

PiotrSikora added 8 commits January 14, 2019 08:15

review: remove local unix_cc_configure.bzl.

15c0bd7

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: override flags in cc_wrapper.

dae2e74

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Merge remote-tracking branch 'origin/master' into PiotrSikora/libcxx_…

9e098c9

…build

review: fix spelling.

4c1b43c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix bad staging.

43eb5b0

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use CXX for clang as well.

0af9f40

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: move CXXFLAGS.

abefdc3

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: style.

49e9621

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jmillikin-stripe reviewed Jan 14, 2019

View reviewed changes

lizan reviewed Jan 14, 2019

View reviewed changes

review: support Lizan's custom build rules.

1fe552c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

lizan previously approved these changes Jan 15, 2019

View reviewed changes

PiotrSikora changed the title ~~build: allow linking against libc++ on Linux.~~ build: support linking against libc++ on Linux. Jan 15, 2019

htuch suggested changes Jan 16, 2019

View reviewed changes

PiotrSikora added 2 commits January 16, 2019 06:28

review: remove empty pass.

068d04c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use shlex.split.

766dbd2

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora dismissed lizan’s stale review via 766dbd2 January 16, 2019 07:40

htuch reviewed Jan 16, 2019

View reviewed changes

PiotrSikora added 2 commits January 16, 2019 09:15

review: add build instructions to the README.

1117553

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: add note about versioned clang binaries.

e630ec8

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora commented Jan 16, 2019

View reviewed changes

PiotrSikora added 4 commits January 16, 2019 11:58

review: --config=libc++.

c1cf85d

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: don't hardcode CC and CXX after all.

ebf6543

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: pass existing CC, CXX and PATH variables.

dd9de95

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: append common CXXFLAGS when building dependencies.

914138f

Notably, tcmalloc_debug crashes without -ggdb3 -fno-omit-frame-pointer. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jmillikin-stripe approved these changes Jan 16, 2019

View reviewed changes

htuch approved these changes Jan 17, 2019

View reviewed changes

htuch merged commit 6674c03 into envoyproxy:master Jan 17, 2019

PiotrSikora mentioned this pull request Jan 21, 2019

Consult BAZEL_CXXOPTS when auto-detecting local CC toolchain. bazelbuild/bazel#7074

Closed

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

build: support linking against libc++ on Linux. (envoyproxy#5563)

1d6b134

Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

SanjayVas mentioned this pull request Feb 20, 2021

System headers not found when using clang with libc++ bazelbuild/bazel#13071

Closed

		@@ -15,4 +15,6 @@ tar xf gperftools-"$VERSION".tar.gz
		cd gperftools-"$VERSION"

		LDFLAGS="-lpthread" ./configure --prefix="$THIRDPARTY_BUILD" --enable-shared=no --enable-frame-pointers --disable-libunwind

build: support linking against libc++ on Linux. #5563

build: support linking against libc++ on Linux. #5563

Conversation

PiotrSikora commented Jan 10, 2019

PiotrSikora commented Jan 10, 2019 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

PiotrSikora commented Jan 10, 2019

htuch commented Jan 10, 2019

PiotrSikora commented Jan 10, 2019

jmillikin-stripe left a comment

Choose a reason for hiding this comment

PiotrSikora commented Jan 10, 2019

lizan commented Jan 10, 2019

PiotrSikora commented Jan 12, 2019

htuch commented Jan 14, 2019

PiotrSikora commented Jan 14, 2019

PiotrSikora commented Jan 14, 2019

repokitteh-read-only bot commented Jan 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora commented Jan 15, 2019

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Jan 16, 2019

repokitteh-read-only bot commented Jan 16, 2019

PiotrSikora commented Jan 16, 2019

repokitteh-read-only bot commented Jan 16, 2019

jmillikin-stripe left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

PiotrSikora commented Jan 10, 2019 •

edited

Loading