Skip to content

buffer: use compiler version rather than other platform ifdefs to determine library call to use to serialize doubles#29532

Merged
ggreenway merged 4 commits intoenvoyproxy:mainfrom
jmarantz:use-cxx-compiler-version
Sep 11, 2023
Merged

buffer: use compiler version rather than other platform ifdefs to determine library call to use to serialize doubles#29532
ggreenway merged 4 commits intoenvoyproxy:mainfrom
jmarantz:use-cxx-compiler-version

Conversation

@jmarantz
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz commented Sep 9, 2023

Commit Message: In #29500 I introduced a conditional-compilation for using the best available library API to serialize doubles. This is because clang library v14, the primary development platform, has a better API available than older compilers.

The conditional compilation structure I used focused on excluding GCC and Apple, and that passed on CI.

However, the ifdefing didn't quite work for people that develop with older version of clang, or hybrid library environments. I did not think people would be doing that, but they are :) So this changes the conditional compilation strategy to look specifically for clang library versions >= 14.
Additional Description: n/a
Risk Level: low
Testing: just CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

…erialization mechanism

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #29532 was opened by jmarantz.

see: more, trace.

@jbohanon
Copy link
Copy Markdown
Contributor

jbohanon commented Sep 9, 2023

#29500

@jmarantz jmarantz changed the title use compiler version rather than other planform ifdefs to determine s… use compiler version rather than other platform ifdefs to determine s… Sep 9, 2023
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title use compiler version rather than other platform ifdefs to determine s… buffer: use compiler version rather than other platform ifdefs to determine library call to use to serialize doubles Sep 9, 2023
@jbohanon
Copy link
Copy Markdown
Contributor

jbohanon commented Sep 9, 2023

Hey Joshua, thanks for jumping on this so quick!

This change does not fix the build in the case I was hitting (which is the default configuration in the devcontainer). Namely: clang-14 with libstdc++-9.

I'm fairly ignorant about compilers/stdlibs, but is the GCC/clang differentiation important here, or is it just a requirement of the libraries having the correct function/template defined? As I mentioned in the other PR comment, this works with clang-14 and libstdc++-11

@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented Sep 9, 2023

Thanks for the heads up. What exactly is the 'devcontainer'? Is that this: https://github.com/envoyproxy/envoy/blob/main/.devcontainer/README.md ?

Are libstd++ versions supposed to match clang versions? What works best for me is to download the version of clang that matches what's used in Envoy CI, 14.06, which is found at https://github.com/llvm/llvm-project/releases/tag/llvmorg-14.0.6 . That includes a library (libclang.so.14.0.0). Why would you use a compiler of one version with a library from a different version?

@jbohanon
Copy link
Copy Markdown
Contributor

jbohanon commented Sep 9, 2023

Yeah, that is the one. It seems pretty loosely supported, but it hasn't completely languished. I had been using vim with VERY spotty LSP for several months. After recently looking at #27680 I gave up and switched to vscode+devcontainer, which has been working pretty much flawlessly.

That said though, the documentation in bazel/README.md instructs users to download llvm, run bazel/setup_clang.sh to set up the clang.bazelrc, then add --config=clang to user.bazelrc to make it default. Just below that, it indicates that --config=clang means clang with libstdc++, while you have to use --config=libc++ to get clang with libc++. That's corroborated by the relevant sections in .bazelrc

I really don't have any practical or ideological preference here, just following what documentation/issue resolution I've been able to find has led me to this sorta Frankenstein's monster of a toolchain setup that purports to be the best practice for Envoy dev. I'm happy to be instructed otherwise and help update the documentation!

…of std::to_chars on a double

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Copy Markdown
Contributor Author

jmarantz commented Sep 10, 2023

Thanks for testing this @jbohanon -- can you see if this latest version compiles for you?

Copy link
Copy Markdown
Contributor

@jbohanon jbohanon left a comment

Choose a reason for hiding this comment

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

Yes, that works for my combo

@jmarantz
Copy link
Copy Markdown
Contributor Author

Thanks @jbohanon !

@jmarantz jmarantz marked this pull request as ready for review September 10, 2023 22:51
@ggreenway ggreenway merged commit 154219f into envoyproxy:main Sep 11, 2023
@jmarantz jmarantz deleted the use-cxx-compiler-version branch September 11, 2023 16:34
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.

4 participants