-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adds fabric attached memory support #136
base: develop
Are you sure you want to change the base?
Conversation
new option OPENFAM (requires OpenFAM and gRPC packages) FAM source files
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #136 +/- ##
===========================================
- Coverage 63.80% 63.76% -0.04%
===========================================
Files 1069 1065 -4
Lines 55387 55350 -37
Branches 4100 4084 -16
===========================================
- Hits 35337 35295 -42
- Misses 20050 20055 +5 ☔ View full report in Codecov by Sentry. |
Private downstream CI failed. |
Private downstream CI succeeded. |
Private downstream CI succeeded. |
Private downstream CI failed. |
find_package( protobuf CONFIG REQUIRED ) | ||
find_package( gRPC CONFIG REQUIRED ) |
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.
They should specify the desired minimum version to search for.
${LIB_UUID_ROOT} | ||
${LIB_UUID_DIR} | ||
${LIB_UUID_PATH} | ||
ENV LIB_UUID_ROOT | ||
ENV LIB_UUID_DIR | ||
ENV LIB_UUID_PATH | ||
PATH_SUFFIXES uuid |
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.
are we working with custom compiled / custom locations for libuuid? if not the default search locations should be sufficient to pick up libuuid
cmake/FindLibUUID.cmake
Outdated
set(LIB_UUID_LIBRARIES ${LIB_UUID_LIBRARY}) | ||
set(LIB_UUID_INCLUDE_DIRS ${LIB_UUID_INCLUDE_DIR}) | ||
if(NOT TARGET LibUUID) | ||
add_library(LibUUID UNKOWN IMPORTED) | ||
set_target_properties(LibUUID PROPERTIES | ||
IMPORTED_LOCATION "${LIB_UUID_LIBRARY}" | ||
INTERFACE_INCLUDE_DIRECTORIES "${LIB_UUID_INCLUDE_DIR}" | ||
INTERFACE_LINK_LIBRARIES "${LIB_UUID_LIBRARY}") | ||
endif() | ||
endif() |
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.
I suggest that your custom find package follows the one make uses internally for liquid, see: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/Modules/FindLibUUID.cmake
Ofc you can drop msgs/cygwin stuff.
std::string_view view() const noexcept { return std::string_view(buffer_, size_); } | ||
|
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.
Buffer
is a byte buffer and while it could be used as a c-string it think this should not be done. Especially. since the only usage I can see is in tests.
friend std::ostream& operator<<(std::ostream& out, const FamConfig& config) { | ||
out << "endpoint=" << config.endpoint << ", sessionName=" << config.sessionName; | ||
return out; | ||
} |
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.
This does not need to be a friend.
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
FAM support via OpenFAM