CMake Targets #259
Comments
AFAIK that's not the proper way to select c++11. |
|
Hey there! Thanks for the comments. We will definitely update the readme. I just got back from vacation and am triaging all the CMake related issues and once they're all together I'll update all the information. The correct way to set the language level for abseil is to set CMAKE_CXX_STANDARD to 11, 14, or 17. We use it here and then here and here. |
|
Is it OK if Abseil is build with 11 and the program itself is build with 17? |
|
I haven't tried it, but that's extremely likely to go badly because the underlying types for absl::string_view are different in C++11 and C++17 compilation modes (https://github.com/abseil/abseil-cpp/blob/master/absl/strings/string_view.h#L33). Can you give us a little more background on why you'd like to compile Abseil as C++11 if the rest of your program is C++17? This isn't something we've heard of people wanting to do before. If we understand the problem better, we may be able to offer alternatives or work on a fix if the problem is on our end. |
|
It's actually related to #260 If it's a bad idea and the compiler defaults to C++14 (GCC 6) and Abseil defaults to C++11 then something is wrong. |
|
Abseil uses the value of CMAKE_CXX_STANDARD since this is the cmake way of controlling C++ language level for all targets. How are you specifying C++17 in your build? |
|
|
|
Abseil supports compilers for 5 years, which includes a lot of versions of GCC for which C++14 is not the default. If Abseil changed the default C++ version, then users of those old but supported GCC versions who rely on the default would have their builds break when they pull in the latest version of Abseil. I don't think there's a good answer here. It's really important that users specify what version of C++ they're using. I can see an argument that not specifying a version should be an error instead of a warning, though. |
If I may make a comment on this: In my experience this is the absolutely wrong approach for a library. Especially if your API changes depending on the language level, then the used standard is something that you should simply adopt from the project abseil is used from. Otherwise there are two possibilities:
The only thing a library cmake file should do is a |
How do you propose to do this besides the standard mechanism in CMake?
We don't set CMAKE_CXX_STANDARD, we only read it. The relvant code is here In fact our test scripts do what you suggested, using -DCMAKE_CXX_STANDARD in the command line.
But this is a problem in a library like abseil which changes depending on language level. We need to know what language level our dependents are using, and so we must use the tool given to us in CMake to detect C++ language level. |
That might not be right. If it's unset, you default to C++11 which might not be what the compiler defaults to. |
|
@JonathanDCohen: Sorry, my mistake. I read this as "the correct way is for abseil to use
Just do nothing. If the parent project / toolchain file sets CMAKE_CXX_STANDARD, then your targets will automatically adopt it. If your parent manually adds -std=c++11 to the global CXX flags (it shouldn't but...) your targets will adopt them. And if the parent just uses the compiler default, then so will abseil.
cxx_std_11 only sets the minimum required level to 11. If your parent sets The only situation, where this unfortunately doesn't work is if another library uses e.g.
Do you actually have to know this anywhere in the cmake scripts or does it just have to be consistent? The less individual libraries meddle around with compile flags the easier it is to compile everything with a consistent set of flags, because that is what cmake does by default automatically. |
IIRC the default has never been C++11. GCC 6 defaults to C++14, before that it was C++03 (or 98).
Won't the requirement of mylib bubble up and cause everything to use C++17? |
It will bubble up to myexec, but not down to abseil, which is why I'm still on the fence, if libraries should not even use the |
|
But if myexec sets it directly it trickles down to abseil, isn't that a bit weird? |
No it doesn't. Only if you set the language standard globally in the parent cmake file (E.g. via CMAKE_CXX_STANDARD) the setting will trickle down. |
|
Sounds like a design bug in cmake.. |
I certainly don't like the situation, but I'm not sure what a better solution would look like. A single library can be used from mutliple different other targets and not all of them might be compiled as part of same cmake project (or compiled by cmake at all), so it makes sense that "requirements" trickle up the dependency chain, but you can't realistically enforce that they are also propageted down, as that would lead to conflicts and/or you'd have to travel back in time. |
|
If modes can't be mixed you should have separate library builds for each mode.. like you have separate builds for amd64 and arm and x86. Or you shouldn't have libraries at all and build everything as part of the executable (like I think Google does). |
|
True, and cmake absolutely allows for that usage pattern. As I said: don't mess with compiler flags in library cmake files and (almost) everything is going to be fine. Instead define your toolchain parameters (compiler, c++ standard version, standard library, debug/release flags) in a toolchain file and use that toolchain to build all your libraries consistently. The remaining problems I encounter are
Pesonally I think the fundamental problem is actullay that c++ libraries like to scatter A very good talk on that topic https://www.youtube.com/watch?v=sBP17HQAQjk Sometimes I wonder if it wouldn't be a better approach, if headers got installed in a preprocessed manner, so there is no chance they get used in different ways in different translation units. |
|
So the offending bit of code is really right here .
It has to be consistent everywhere abseil is used. The issue raised aboe with usage requirements is why we didn't use them. We could just error out of CMAKE_CXX_STANDARD is 98 and else just use whatever cmake gives us I suppose. Do we have an idea of what the surface area is for breaking people if we make this change? We wouldn't be able to ship a tool to fix projects as it would depend on them setting CMAKE_CXX_STANDARD either in the cmake files or command line invocations.
This is the current recommended way to use abseil, via |
And in particular here: abseil-cpp/CMake/AbseilHelpers.cmake Line 210 in 7bd8f36
I think the projects that could get broken by this (and which aren't broken at the moment) are the ones that
If I'm not mistaken, adding a usage requirement would result in zero beakage (all consumers of abseil are already compiled with at least c++11 and so is abseil itself), which is why I suggested it, but I agree that it comes with its own bag of problems and might not be the right mechanism for abseil. Btw.: You can specify the reuqired compile features as private, so it doesn't propagate to your consumers if that makes you feel better. |
|
Coming back around. It seems like the answer is from the thread -- we can set This is a potential breaking change for Abseil users, and due to the unpredictable nature of how this global could propagate, I want to be very careful making the change, as we can't provide a fix for people's CMake builds as we promise in our developer guidelines. What do we think if this idea -- provide a flag -DABSL_NO_DEFAULT_STD_VALUE for people to try to make sure their code builds, and then after some amount of time (6 months? Probably at a particular LTS release) remove the flag |
This will result in very limited testing.. |
Why is this necessary? Just to clarify my post above: Removing the whole machinery from the cmake file without adding the requirement should only break projects that satisfy all 3 conditions I've listed at the same time. If you remove the existing machinery and add the cxx_std_11 requirement, I believe this should break no one (although as titus demonstrated so nicety in his cppcon talk - even whitespace changes can break someone somewhere). Unfortunately, I'm not sure how to best validate this claim.
As I said, if you make the requirement private it doesn't propagate at all. |
|
I did some experimenting, with compilers which default both to c++98 and to c++14. It seems that for pretty much all cases using a private requirement should work for us. The experiments are in https://github.com/JonathanDCohen/CMakeVersionExample. The specific problem mentioned by @MikeGitb I think is a non-issue, because I don't see how all of those conditions can co-exist. If the compiler defaults to C++14, but has a target which specifies via usage requirements to use C++11, and then we change Abseil to use C++11 via usage requirement, it will be the same effect, that Abseil is build with C++11. On the other hand if the client library specifies CMAKE_CXX_STANDARD=11, then Abseil was already being built with C++11 anyways, so we're fine. So all in all it seems like this is actually just a refactor and shouldn't change behavior at all, but will give us a little cleaner code. I'll write up this change tomorrow when I get back to work :) |
|
We have hit a roadblock -- the I recently pushed a change to remove the use of list(FILTER becuase it only appears in cmake 3.6, and we found that cmake 3.5 is the version bundled with, for example, ubuntu 14 LTS and ubuntu 16 LTS, so we can't reasonably introduce a cmake 3.8 feature. I think it's time to reach out to our users and see what versions of cmake everyone is using. It is pretty easy to keep cmake up to date as I've found, and so far the decisions for where to draw the line on cmake versions has been pretty ad-hoc. For now I'm going to put a hold on this until we can set a more firm version requirement. |
So the 'private' requirement upgrades but also downgrades the level the client is build with, if the client is using defaults? What's private about that? |
|
It will not downgrade, but if the project is compiled with a compiler that defaults to 14, abseil will also be compiled with 14 it just makes sure you don't get compiled with something older than c++11. It is private, because it doesn't influence the user of the library. |
|
knowing that we have: abseil-cpp/absl/copts/AbseilConfigureCopts.cmake Lines 38 to 45 in b312c3c todo: we should verify CMAKE_CXX_STANDARD >= 11, if user set 03
An other way would be to change the code here: abseil-cpp/CMake/AbseilHelpers.cmake Lines 130 to 132 in b312c3c and here abseil-cpp/CMake/AbseilHelpers.cmake Lines 217 to 218 in b312c3c by: # INTERFACE libraries can't have the CXX_STANDARD property set
# For cxx_std_11 see https://cmake.org/cmake/help/v3.8/release/3.8.html#c-c
if(${CMAKE_VERSION} VERSION_GREATER_EQUAL 3.8.2)
target_compile_features(${_NAME} PRIVATE cxx_std_11)
else()
set_property(TARGET ${_NAME} PROPERTY CXX_STANDARD ${ABSL_CXX_STANDARD})
set_property(TARGET ${_NAME} PROPERTY CXX_STANDARD_REQUIRED ON)
endif()@JonathanDCohen you could update the CL 235732173 accordingly ? |
Last result for GCC 7 @ https://github.com/JonathanDCohen/CMakeVersionExample This looks like a downgrade for the client to me.. as the default is C++14. |
|
To second @MikeGitb, why do you insist on defining Compiling |
|
@OlafvdSpek: You are right, I'm sorry. I though I tested that but apparently did something else (don't have my original testcode around anymore, so I don't know what went wrong). My appologies to you an @JonathanDCohen for spreading misinformation. Regardles of that, a private requirement doesn't influence the client and I'm not sure where you are reading that from. I agree however, that this downgrade behavior pretty much eliminates feature requirements like cxx_std_11 as a sensible option (even if it weren't a cmake >3.8 feature), because it leads to the exact same problem as the current solution (compiling abseil with an older version than the compiler default if CMAKE_CXX_STANDARD is not set). Which brings me to my original suggestion: Just remove the whole machinery. Of course it might be prudent to have a warning message for a couple of releases first. |
|
|
For the |
|
@JonathanDCohen I understand your intention. My point is that you should let the user decide how to consume absl. So instead of trying to find a solution to enforce c++ standard level propagation, flags or something else, document the way absl works and inform the user that it should not mix different c++ standard or flags while using absl.
This is debatable and you may have reasons to do that but this part of the problem. Regarding "installable" version of absl with enforcing that the same c++ standard level is used, you could provide absl for c++11, absl for c++14 and so on. |
A noble goal, but afaik not possible in cmake (at least not from within the libraries cmake file). If you think this is really important, have you considered solving this on the c++ level, by using a inline namespace whose name depends on the __cplusplus macro? Overal I think there is a general tendency for library build scripts trying to be too clever. Especially when static linking and compilation as part of the main project are the desired usecases, a build script should do little more than say: I want to create a library Foo, those are the source files, this is the include directory and that are the dependencies. That's it. No compiler or linker flags, no options that the user can configure, no enabling or disabling of sanitizers and warnings etc. Of course google could engage the cmake developer community and - if they agree - implement a feature that allows such a strict version check. |
This change was suggested by Alek base on this link: abseil/abseil-cpp#259 (comment) The idea is to not setup globally the requirements of C++ version, rather setup the minimal requirements that affects only this library.
|
The recommendation to set |
|
TL;DR: here is a pull request for an updated CMake build configuration. But neither current behavior nor this proposed update ends up enforcing the desired bazel-toolchain-style requirement for Abseil library targets to have been built with the exact same C++ standard as the targets that depend upon it. Updated CMake-related documentation at least. From this issue thread, I imagine the biggest area of debate would be whether I started looking into this when I hit linker ABI compatibility issues (Abseil targets were compiled with C++14 but then linked into a target built with C++17). Unfortunately, changing Abseil to use the FIRST EXAMPLEHere's a simplified dependency example that models my original problem:
Let's assume we're building Since So what are the behavior differences? (A) CURRENT BEHAVIOR: Explicitly setting the So (Setting the RESULT: (B) PROPOSED UPDATE: If Abseil targets use the RESULT: sadly still the same in this case. The SECOND EXAMPLELet's instead imagine a future case when Abseil drops C++11 support, and a project that uses it has its
(A) CURRENT BEHAVIOR: Building (B) PROPOSED UPDATE: All Abseil targets would now set the PROPOSALSo is the behavior of (B) better than (A)? I think it's at least slightly preferable. (A) is basically a no-op. (B) better uses the newer CMake mechanisms to declare that Abseil targets must be built with a particular minimum C++ standard and says that this requirement should propagate to all targets that depend on Abseil. Neither solution enforces what Abseil really wants here, which is to require that all targets that link (or include headers of) an Abseil library be built with the same (not minimum) C++ standard as those Abseil targets. In this pull request, I updated Abseil's CMake-related documentation to clarify this issue a little more (and provide a separate recommendation for library projects vs. root application projects). But it's probably also worth following up with some CMake experts about whether there is a way to achieve this stronger build requirement today, or if it's worth a CMake feature request to support this, as @MikeGitb suggested earlier. PRIVATE VERSUS PUBLIC PROPAGATIONSo if Abseil requires a minimum of C++11 (in the future C++14, etc.), should that requirement propagate to targets that link against Abseil? Semantically, I think it should. Since Abseil's documented usage requirements are to be compiled with the exact same C++ standard as the targets that depend upon it for ABI compatibility, this weaker CMake requirement should not change the behavior any builds correctly configured to respect the stronger documented requirement. Practically, if projects are relying on their compiler defaults rather than explicitly setting But guarding this behavior change behind an option + warning message, with a time period for clients to test their builds is the only way to change this kind of build behavior with a widely used library, right? So I think it makes sense to move to the more semantically correct Feedback on all that very welcome. |
See #259 for context. This change introduces the ABSL_PROPAGATE_CXX_STD option (default to OFF for now, though it prints a warning in CMake 2.8+ builds that a future Abseil release will default to ON). When enabled, all Abseil CMake targets will set the cxx_std_11 target meta feature (which will then propagate to targets that depend upon Abseil) rather than setting the target-level CXX_STANDARD property to CMAKE_CXX_STANDARD (which is the default value anyway), which doesn't propagate. Updates README documentation to clarify behavior and with a different example recommendation for library projects (which should generally leave CMAKE_CXX_STANDARD to the root application project). See https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/ for a useful overview of these different CMake features surrounding C++ standard version configuration.
|
Hey there Eric
First a quick note that I'm no longer a full time Abseil maintainer. These
opinions are my own and have nothing to do with the current Abseil devs.
Speaking about your first example specifically, there are two cases where
this could happen
(1) `my::lib` has a direct dependency on `another::lib`. In this case the
maintainer of `root_app` would need to set `cxx_std_17` anyways in order to
properly use it, which would propagate to `absl` (if I remember correctly,
it's been a couple years since I worked full time on Abseil)
(2) `my::lib` has a transitive dependency on `another::lib`
through `direct::dep`. In this case `direct::dep` should be noting in some
way that it requires C++17, and we then fall back to case (1).
So I'm confused why `my::lib` isn't setting `cxx_std_17` in this case.
IIUC this should coerce Abseil to C++17 and then all is right with the
world.
…On Wed, Jul 14, 2021 at 8:20 PM Eric Barndollar ***@***.***> wrote:
*TL;DR*: here <#986> is a pull
request for an updated CMake build configuration. But neither current
behavior nor this proposed update ends up enforcing the desired
bazel-toolchain-style requirement for Abseil library targets to have been
built with the exact same C++ standard as the targets that depend upon it.
Updated CMake-related documentation at least.
From this issue thread, I imagine the biggest area of debate would be
whether cxx_std_11 should be PRIVATE or PUBLIC (and propagate). I think
the latter -- rationale at the end.
------------------------------
I started looking into this when I hit linker ABI compatibility issues
(Abseil targets were compiled with C++14 but then linked into a target
built with C++17). Unfortunately, changing Abseil to use the cxx_std_11
meta feature instead of the current explicit setting of target-level
CXX_STANDARD property doesn't hugely improve this particular situation --
but I do think it's still worth updating Abseil's CMake configuration and
documentation.
FIRST EXAMPLE
Here's a simplified dependency example that models my original problem:
[image: absl_cxx_std_example1]
<https://user-images.githubusercontent.com/1811213/125708734-8f826e35-9319-4927-9963-dfcd7c0edbd9.png>
Let's assume we're building root_app's CMake project, and the other
libraries are all static libraries added to that project via
add_subdirectory() or FetchContent_MakeAvailable(), which has the same
effect.
Since another::lib requires C++17, the real solution in all cases is for
root_app to set CMAKE_CXX_STANDARD to 17 (or higher), but it would be
nice if the CMake build could catch the problem and report it via a clear
error message (rather than unresolved linker errors).
So what are the behavior differences?
*(A) CURRENT BEHAVIOR*: Explicitly setting the CXX_STANDARD target
property doesn't really make sense (and is roughly equivalent to just doing
nothing).
So absl::strings has its CXX_STANDARD target property set to
ABSL_CXX_STANDARD, which gets set to the value of CMAKE_CXX_STANDARD
(here 14). I think this is pointless, because if a target doesn't have an
explicit value for its target-level CXX_STANDARD property set, it will be
defaulted to CMAKE_CXX_STANDARD
<https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_STANDARD.html>
anyway.
(Setting the CXX_STANDARD_REQUIRED target property is a slight
improvement on CMake default behavior, just in case the root_app project
doesn't set CMAKE_CXX_STANDARD_REQUIRED as the top-level value).
RESULT: absl::strings is compiled with C++14 standard flags, but
another::lib's cxx_std_17 meta feature requirement propagates to all the
other targets, which are then compiled with C++17 standard flags. Yields
linker errors, since that affects the Abseil ABI (*e.g.* absl::string_view
will be a different type).
*(B) PROPOSED UPDATE*: If Abseil targets use the cxx_std_11 meta feature
instead as a PUBLIC (or INTERFACE) property, the main difference is that
this becomes a minimum C++ standard version requirement that propagates to
targets that depend on it. Unlike (A), this also works for header-only
libraries (which have no CXX_STANDARD property themselves).
RESULT: sadly still the same in this case. The my::lib target inherits
both the cxx_std_11 (from Abseil) and cxx_std_17 (from another::lib) meta
features (C++17 wins), so it and root_app will be built with the C++17
standard, while absl::strings is compiled as C++14. Same unhelpful linker
errors.
SECOND EXAMPLE
Let's instead imagine a future case when Abseil drops C++11 support, and a
project that uses it has its CMAKE_CXX_STANDARD set to 11:
[image: absl_cxx_std_example2]
<https://user-images.githubusercontent.com/1811213/125708746-09f96767-09ee-4af2-a3a8-1d7cab6b4482.png>
*(A) CURRENT BEHAVIOR*: Building absl::strings (with CXX_STANDARD target
property set to 11) would then likely fail with compiler errors about
unavailable C++14 language or standard library features. It would probably
make sense to add a CMake check to fail the build sooner if
ABSL_CXX_STANDARD (or CMAKE_CXX_STANDARD) is less than 14.
*(B) PROPOSED UPDATE*: All Abseil targets would now set the cxx_std_14
meta feature, which would propagate to my::lib and root_app, so the build
should succeed with all targets compiled as C++14. (If a different
dependency library was also in the picture without the C++14 meta feature,
it could still be built with the C++11 CMAKE_CXX_STANDARD default and
have its own ABI issues).
PROPOSAL
So is the behavior of *(B)* better than *(A)*? I think it's at least
slightly preferable.
*(A)* is basically a no-op. *(B)* better uses the newer CMake mechanisms
to declare that Abseil targets must be built with a particular minimum C++
standard *and* says that this requirement should propagate to all targets
that depend on Abseil.
Neither solution enforces what Abseil really wants here, which is to
require that all targets that link (or include headers of) an Abseil
library be built with the *same* (not minimum) C++ standard as those
Abseil targets. In this pull request, I updated Abseil's CMake-related
documentation to clarify this issue a little more (and provide a separate
recommendation for library projects *vs.* root application projects). But
it's probably also worth following up with some CMake experts about whether
there is a way to achieve this stronger build requirement today, or if it's
worth a CMake feature request to support this, as @MikeGitb
<https://github.com/MikeGitb> suggested earlier.
PRIVATE VERSUS PUBLIC PROPAGATION
So if Abseil requires a *minimum* of C++11 (in the future C++14, *etc.*),
should that requirement propagate to targets that link against Abseil?
Semantically, I think it should. Since Abseil's documented usage
requirements are to be compiled with the exact same C++ standard as the
targets that depend upon it for ABI compatibility, this weaker CMake
requirement should not change the behavior any builds correctly configured
to respect the stronger documented requirement.
Practically, if projects are relying on their compiler defaults rather
than explicitly setting CMAKE_CXX_STANDARD, this could cause CMake to
start building targets with *e.g.* C++11 rather than a C++14 default. I'd
argue any CMake builds relying on these defaults are not correctly
configured today, since they aren't specifying build semantics required for
correctness.
But guarding this behavior change behind an option + warning message, with
a time period for clients to test their builds is the only way to change
this kind of build behavior with a widely used library, right? So I think
it makes sense to move to the more semantically correct PUBLIC
propagation as part of that change.
Feedback on all that very welcome.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#259 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFEUHP6H5ORWLGWUJSBWTTXYSWTANCNFSM4GSLQBLA>
.
--
Jon Cohen
Software Engineer
jigsaw.google.com
|
|
Hey @JonathanDCohen, thanks for taking a look too. Just to make sure, are you clear on the distinction between how the I think another subtlety here is whether or not every target is trying to achieve the all targets in the entire build should be compiled with the same C++ standard practice. CMake is perfectly happy to build part of the dependency graph with C++17 and any targets that
To clarify, Also note that
If But neither case would affect the C++ standard that Abseil targets are built with.
Likewise, if But back to the Abseil-specific challenge -- because
It would still be nicer if there were a CMake mechanism to enforce Abseil's desired same C++ standard constraint, though, because even with all of the above, Does that all make sense? |
|
On Sat, Aug 14, 2021 at 8:25 PM Eric Barndollar ***@***.***> wrote:
Hey @JonathanDCohen <https://github.com/JonathanDCohen>, thanks for
taking a look too. Just to make sure, are you clear on the distinction
between how the CMAKE_CXX_STANDARD variable functions versus target-level
meta features like cxx_std_17?
It's possible I'm not, it's been a little while and CMake does have the
annoying problem of many competing similar ways of doing things :P
I think another subtlety here is whether or not every target is trying to
achieve the *all targets in the entire build should be compiled with the
same C++ standard* practice. CMake is perfectly happy to build part of
the dependency graph with C++17 and any targets that cxx_std_17 didn't
propagate to with C++14 (*e.g.* per the root_app project's
CMAKE_CXX_STANDARD value) -- and IIUC for parts of the graph without
Abseil (or similar libraries where C++ standard affects API + ABI)
involved, that could build and function just fine.
(1) my::lib has a direct dependency on another::lib. In this case the
maintainer of root_app would need to set cxx_std_17 anyways in order to
properly use it, which would propagate to absl (if I remember correctly,
it's been a couple years since I worked full time on Abseil)
To clarify, root_app's CMake project should set the CMAKE_CXX_STANDARD
variable to 17 to solve the issue here. (Whereas setting the cxx_std_17
target-level meta feature on root_app wouldn't accomplish that, since it
would only affect things that depended on the root_app target, not every
target in the build).
Also note that CMAKE_CXX_STANDARD doesn't actually enforce that *all*
targets in the build are built with that standard, it just provides the
default C++ standard for any targets that (a) didn't otherwise set an
explicit CXX_STANDARD target property and (b) don't require a *higher*
standard because of things a cxx_std_17 meta feature.
If I remember correctly this is why we explicitly set the CXX_STANDARD on
each library, to avoid this issue and enforce all Abseil targets to the
same standard in a very obvious explicit way.
Case (b) is how my first example ended up with Abseil compiled as C++14
even though the C++17 standard propagated down to my::lib and root_app.
(2) my::lib has a transitive dependency on another::lib through
direct::dep. In this case direct::dep should be noting in some way that
it requires C++17, and we then fall back to case (1).
If direct::dep exposes another::lib in its API, then it should link
against another::lib as PUBLIC (in target_link_libraries()), and then
cxx_std_17 would propagate to my::lib as well. If it only uses
another::lib in its implementation, then it should link as PRIVATE, and
cxx_std_17 would not propagate further.
But neither case would affect the C++ standard that Abseil targets are
built with.
So I'm confused why my::lib isn't setting cxx_std_17 in this case. IIUC
this should coerce Abseil to C++17 and then all is right with the world.
Likewise, if my::lib exposes another::lib in its API, it would link it as
PUBLIC, and then my::lib would inherit cxx_std_17 minimum itself and also
propagate it to targets that depend on my::lib like root_app. (That was
the case in my first example). So that's not enough to solve the problem.
But back to the Abseil-specific challenge -- because my::lib depends on
an Abseil target, it needs to enforce that it gets built with the *same*
C++ standard as Abseil. So I think that means it needs to:
- Figure out the highest C++ standard it requires itself or
transitively inherits from targets it depends on (cxx_std_17 from
another::lib in my first example).
Would it be useful for ABSL_CXX_STANDARD to be exported to the parent scope
so code depending on Abseil can use it?
…
- Document for its clients that they must set CMAKE_CXX_STANDARD to 17
or higher.
- Optionally add a check that fails the build if CMAKE_CXX_STANDARD is
less than 17. (As at the end of the 2nd code example here
<https://github.com/abseil/abseil-cpp/tree/master/CMake#step-by-step-instructions>,
which my change updated).
It would still be nicer if there were a CMake mechanism to enforce
Abseil's desired *same C++ standard* constraint, though, because even
with all of the above, another::lib could later upgrade to cxx_std_20 and
builds would start failing with unhelpful linker errors until someone digs
into why.
Does that all make sense?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#259 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFEUAW5TITQ45U3KEEBKTT44CP5ANCNFSM4GSLQBLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
--
Jon Cohen
Software Engineer
jigsaw.google.com
|
Code using abseil shouldn't have to worry about yet another library specific solution. Abseil should just respect CMAKE_CXX_STANDARD (maybe it already does?) Otherwise abseil would just add to the conundrum that you described yourself.
|
I don't think this had any value (my change stops doing this this, flag protected), since you get exactly the same result just letting (Also note that the higher of
I don't think so.
It already does. And agreed that Abseil should use conventional CMake mechanisms (admittedly confusing as they are) as much as possible and then document additional requirements and best practices for clients when those build mechanisms fall short. So namely:
This is mostly tricky because all of the CMake features are oriented around minimum C++ standard, but Abseil wants to enforce same standard across the link boundary. I think with the above, the only situation where projects get into trouble is when a target-level feature (like So maybe that's the right CMake feature request: provide some mechanism for detecting or failing the build with a clear error message if |
|
On Mon, Aug 16, 2021 at 9:54 AM Eric Barndollar ***@***.***> wrote:
If I remember correctly this is why we explicitly set the CXX_STANDARD on
each library, to avoid this issue and enforce all Abseil targets to the
same standard in a very obvious explicit way.
I don't think this had any value (my change stops doing this this, flag
protected), since you get exactly the same result just letting
CMAKE_CXX_STANDARD do its thing (it initializes CXX_STANDARD for every
target that doesn't otherwise set one).
*(Also note that the higher of CXX_STANDARD target property and max
inherited cxx_std_NN meta feature wins, so you can't count on CXX_STANDARD
being the exact explicit standard a target is built with anyway).*
Would it be useful for ABSL_CXX_STANDARD to be exported to the parent
scope so code depending on Abseil can use it?
I don't think so.
Code using abseil shouldn't have to worry about yet another library
specific solution. Abseil should just respect CMAKE_CXX_STANDARD (maybe it
already does?)
It already does. And agreed that Abseil should use conventional CMake
mechanisms (admittedly confusing as they are) as much as possible and then
document additional requirements and best practices for clients when those
build mechanisms fall short. So namely:
- Let application projects control C++ standard via CMAKE_CXX_STANDARD.
- Specify the minimum required C++ standard for all Abseil targets via
cxx_std_11 (which can upgraded to cxx_std_14 when C++11 support is
dropped).
- Document the requirement for targets that link against Abseil to
have been built with the exact same C++ standard as the Abseil targets.
This is mostly tricky because all of the CMake features are oriented
around *minimum* C++ standard, but Abseil wants to enforce *same*
standard across the link boundary. I think with the above, the only
situation where projects get into trouble is when a target-level feature
(like cxx_std_17) ends up bumping C++ standard to be higher than
CMAKE_CXX_STANDARD for targets that depend on Abseil.
So maybe that's the right CMake feature request: provide some mechanism
for detecting or failing the build with a clear error message if
CXX_STANDARD (usually via CMAKE_CXX_STANDARD) is superseded by a higher
standard requirement. If that was possible, then Abseil documentation could
encourage projects to use that.
I think we're in agreement here. Thanks for walking me through it. The
only change I'd make to the CMake feature request is that it should be
switchable between a warning and an actual error which stops the build.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#259 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFEUGJDGJOFFZ72TJNLW3T5EKCVANCNFSM4GSLQBLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
--
Jon Cohen
Software Engineer
jigsaw.google.com
|
Opened a CMake feature request here: |
See abseil#259 for context. This change introduces the ABSL_PROPAGATE_CXX_STD option (default to OFF for now, though it prints a warning in CMake 2.8+ builds that a future Abseil release will default to ON). When enabled, all Abseil CMake targets will set the cxx_std_11 target meta feature (which will then propagate to targets that depend upon Abseil) rather than setting the target-level CXX_STANDARD property to CMAKE_CXX_STANDARD (which is the default value anyway), which doesn't propagate. Updates README documentation to clarify behavior and with a different example recommendation for library projects (which should generally leave CMAKE_CXX_STANDARD to the root application project). See https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/ for a useful overview of these different CMake features surrounding C++ standard version configuration.
See abseil#259 for context. This change introduces the ABSL_PROPAGATE_CXX_STD option (default to OFF for now, though it prints a warning in CMake 2.8+ builds that a future Abseil release will default to ON). When enabled, all Abseil CMake targets will set the cxx_std_11 target meta feature (which will then propagate to targets that depend upon Abseil) rather than setting the target-level CXX_STANDARD property to CMAKE_CXX_STANDARD (which is the default value anyway), which doesn't propagate. Updates README documentation to clarify behavior and with a different example recommendation for library projects (which should generally leave CMAKE_CXX_STANDARD to the root application project). See https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/ for a useful overview of these different CMake features surrounding C++ standard version configuration.
and others
Could the list be completed?
https://github.com/abseil/abseil-cpp/tree/master/CMake
The text was updated successfully, but these errors were encountered: