-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Propagate CXX version used to compile abseil #1117
Conversation
@h-vetinari not sure how much of the past context you've read through on this topic, but I don't think this change would enforce the C++ standard ABI constraint that Abseil really wants to express. It's been awhile, so please correct me if I'm missing something. When I last get into the weeds on this, my conclusion was that it would really require a new CMake feature to enforce a same C++ standard constraint (as opposed to minimum C++ standard). I filed https://gitlab.kitware.com/cmake/cmake/-/issues/22592 as a feature request, which has received no attention (as evidently not that common of an issue for typical C++ projects). I think with your proposed change, for example, nothing would prevent a downstream target from setting a higher I think |
Yes it does, though the way I understand things, the minimum might actually be enough. The ABI issues come from abseil types not being ABI-compatible with the later stdlib-types they are intended to backport. However, even a newer standard can use the backported types (rather than the stdlib ones), which is why |
CC @coryan since I've seen you propose this in a few places in recent discussions. :) |
# Abseil libraries require C++11 as the current minimum standard. | ||
# Top-level application CMake projects should ensure a consistent C++ | ||
# standard for all compiled sources by setting CMAKE_CXX_STANDARD. | ||
target_compile_features(abseil_dll PUBLIC cxx_std_${ABSL_CXX_STANDARD}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ABSL_CXX_STANDARD
always defined? I think the default is set here:
set(ABSL_CXX_STANDARD "${CMAKE_CXX_STANDARD}") |
And that looks like it can be the empty string? And if is the empty string, then the C++ standard in effect depends on the compiler (and compiler version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coryan see the example patterns in the Abseil CMake README here:
https://github.com/abseil/abseil-cpp/tree/master/CMake#step-by-step-instructions
I think library authors (including Abseil itself) should NOT set CMAKE_CXX_STANDARD
, which should be left up to the end application project.
When a library project is being directly developed or CI tested, then it can safely set those things without affecting its clients (which is the pattern the if(CMAKE_SOURCE_DIR STREQUAL my_lib_project_SOURCE_DIR)
check in the 2nd README example shows; it can also be set via the cmake
command-line in build scripts).
I don't think so, but maybe a concrete example would help work through the details here--do you have a specific scenario that is currently broken but your change would cause to build correctly? Are you seeing issues in an application project or in a library that other clients then depend on? Guess I have 2 concerns here:
On philosophical semantics, I still think the bullet point list in this issue comment is the right approach re: what Abseil should set as a library author and what end application projects are in control of through Practically, when a given target is built, the C++ standard used is the maximum of This also wouldn't help the scenario described in my CMake feature request, since another depended upon library ( |
The way I see the situation: an abseil built with (say) C++11 can be used in a downstream project compiling with But if abseil is built with C++17, a downstream project consuming that artefact may never go below that version (because it will not have the stdlib symbols that abseil compiled in this way will use by default; edits to The point is that the abseil artefacts generated by a given build configuration should encode this constraint, because the alternative (to a clearly actionable "compile with a higher C++ standard" message from the build system) are obscure linker errors that make diagnosing the correct cause of the issue very difficult. I've been there on many projects within conda-forge, where I help package things. |
In general, I think Abseil should be changed such that the installed artifacts reflect their requirements. What does that mean?
I think this change is not quite right. It does not change the @h-vetinari sorry if I mislead you in the Conda discussions. The situation there is different because the package recipes have more control over the environment. Abseil operating in the wild needs to be |
@coryan @h-vetinari I think there are distinct issues here: [A] Before being built, what requirements can/should Abseil targets express through the CMake build system? I'm arguing that The It sounds like you want Abseil's CMake configuration to make properly packaging it in binary distributions through But as far as I know, setting |
Maybe. But it is clearly insufficient too. In the same project, even if using Conversely, (and this is more contrived) with some compilers the default is C++17. So, Abseil may have been compiled with C++17, but it currently sets its
I am not sure what the default CMake build configure should assume or not. What I am sure of, is that the artifacts installed by Abseil are very fragile. They break when the consuming project uses a mismatched C++ version. I know that Abseil's documentation says that one should not do this. But Abseil's behavior is surprising, it leads to breakage (see #696), and could be improved upon.
I would like Abseil to install artifacts that are safe to use. Currently it installs artifacts that cause trouble. You could argue that should be the work of the package recipes. Maybe, but perusing #696 shows that the current situation causes more trouble than not. I think I already articulated how that it is possible (maybe not easy) to provide safer installed targets:
If what I described somehow causes trouble for you [A] scenario please show how. I don't think it does, and in fact I think it improves that scenario too.
AFAIK it does. The installed CMake configuration files include the In any case, this discussion is moot. This PR is not sufficient as-is. |
Thanks for the insightful comments!
The PR hasn't been touched in half a year and makes no claims to completeness. It's also easy to fix once there is a direction of what to do, and the discussion for that direction is not moot IMO. |
Fair enough. Let me try to implement my proposals above. |
Agreed on that. Just a few more thoughts here: To restate big picture, it sounds like you (and many other clients) want the Abseil project to be more opinionated + helpful about how it gets packaged through its CMake build + install support. In that scenario, Abseil is being built directly, so it will be the top-level CMake project, right? It should probably be more opinionated about
Alternatively it could fail to build if none was set and link to documentation around it. Setting All these other details (e.g. It seem like Abseil itself could take a few positions on this, such as:
Got it, I was missing this aspect before. Is there an easy way to override these properties more explicitly per installed target (when hopefully you know more about the actually built versions)? I guess the best way to do this also largely depends on the higher-level question above. |
FWIW, I created a branch with the changes that (I believe) fix #696: master...coryan:abseil-cpp:fix-installed-artifacts-reflect-their-ABI-requirements I am happy if those changes get copied here, or just serve as inspiration. I am also happy to start my own PR. In the last case, as a Googler, I will be asked to make the change in the internal Google SCM and that will get exported "eventually". |
If you sent those changes to me as a Piper CL I would accept them (maybe after I tested them). By the way, exports from Piper to GitHub now happen automatically in minutes. |
Sounds good. I think I will have a CL by the end of today.
TIL. |
@h-vetinari I think you can safely close this PR. My changes got pushed as part of 308bbf3 |
Fixes #1116
Second commit optional