crimson: fix build with GCC-10 #33233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GCC-10 and Clang choke when compiling a concept constrained with its template parameter, which is in turn another concept. as a workaround of the bug of boost::asio, we should disable concepts support in it. but it's nice to enable it when the compiler is able to use concepts to do some compile time checkings even the concepts are not compliant to C++20. Signed-off-by: Kefu Chai <kchai@redhat.com>
`md_config_obs_t` is defined as ``` using md_config_obs_t = ceph::md_config_obs_impl<ConfigProxy>; ``` in `common/config_obs.h`. it takes advantage of a fact that somebody exposes the correct version of `ConfigProxy` to the global namespace. this is intended to fulfill the needs of other components which expects `md_config_obs_t`. otherwise we need to specify `ceph::md_config_obs_impl<ceph::ConfigProxy>` or `ceph::md_config_obs_impl<crimson::common::ConfigProxy>` depending on if we are programming crimson or not. but in this case, we are actually defining `crimson::common::ConfigProxy`, so it'd be better to define `md_config_obs_t` explicitly instead relying on "somebody" which exposes `ConfigProxy`. and `ConfigObserver` is defined using the current `ConfigProxy`, so it's more correct and more readable than using `md_config_obs_t` defined in `common/config_obs.h`. Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov
commented
Feb 12, 2020
| @@ -1,6 +1,13 @@ | |||
| add_library(crimson::cflags INTERFACE IMPORTED) | |||
| set(crimson_cflag_definitions "WITH_SEASTAR=1") | |||
| # disable concepts to address https://github.com/boostorg/asio/issues/312 | |||
| if((CMAKE_CXX_COMPILER_ID STREQUAL GNU AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 10) OR | |||
There was a problem hiding this comment.
for those who are interested in a minimal reproducer, see https://www.godbolt.org/z/6igCM2
rzarzynski
approved these changes
Feb 12, 2020
| if((CMAKE_CXX_COMPILER_ID STREQUAL GNU AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 10) OR | ||
| (CMAKE_CXX_COMPILER_ID STREQUAL Clang)) | ||
| list(APPEND crimson_cflag_definitions | ||
| "BOOST_ASIO_DISABLE_CONCEPTS") |
There was a problem hiding this comment.
OK, we don't plan to use ASIO, so I'm not afraid about disabling concepts there (and forgetting to re-enable when the bug gets fixed).
There was a problem hiding this comment.
it's Graylog.h which pulls boost::asio headers in. i tried to avoid using the all-in-one convenient header of boost/asio.hpp , but no luck:
In file included from /opt/ceph/include/boost/asio/basic_socket.hpp:19,
from /opt/ceph/include/boost/asio/basic_datagram_socket.hpp:20,
from /opt/ceph/include/boost/asio/ip/udp.hpp:19,
from ../src/common/Graylog.h:7,
from ../src/common/ceph_context.cc:36:
/opt/ceph/include/boost/asio/async_result.hpp:70:20: error: a variable concept cannot be constrained
70 | BOOST_ASIO_CONCEPT completion_handler_for =
| ^~~~~~~~~~~~~~~~~~~~~~
/opt/ceph/include/boost/asio/async_result.hpp:492:20: error: a variable concept cannot be constrained
492 | BOOST_ASIO_CONCEPT completion_token_for = requires(T&& t)
| ^~~~~~~~~~~~~~~~~~~~
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox