Skip to content
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

Update grpc to 1.62.0 #152

Merged
merged 6 commits into from
Feb 26, 2024
Merged

Update grpc to 1.62.0 #152

merged 6 commits into from
Feb 26, 2024

Conversation

rainerschoe
Copy link
Member

@rainerschoe rainerschoe commented Feb 22, 2024

fixes #151

The previously used gRPC version had build errors with more modern compilers.
Stepping to the most recent release.

Signed-off-by: Rainer Schoenberger <[email protected]>
Signed-off-by: Rainer Schoenberger <[email protected]>
Signed-off-by: Rainer Schoenberger <[email protected]>
Signed-off-by: Rainer Schoenberger <[email protected]>
Signed-off-by: Rainer Schoenberger <[email protected]>
@@ -27,6 +27,7 @@
#include <optional>
// END MODIFIED

Copy link
Member Author

@rainerschoe rainerschoe Feb 22, 2024

Choose a reason for hiding this comment

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

I took the most recent cli_call.cc and cli_call.hfrom gRPC example source code and adapted it again for our needs (they had some minor updates).

Warning: Failed to parse JSON file '-': INVALID_ARGUMENT:Unexpected end of string. Expected , or } after key:value pair.

^
/Warning: Failed to parse JSON file '-': .*
Copy link
Member Author

Choose a reason for hiding this comment

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

protobuf error message changed slightly. made the test more flexible

@@ -64,6 +64,7 @@ Possible Candidates:
127.0.0.1 examples.StreamingRpcs (uni- and bi-directional streams)
127.0.0.1 examples.StatusHandling (gRPC error handling)
/^127.0.0.1 grpc.reflection.*$
/^127.0.0.1 grpc.reflection.*$
Copy link
Member Author

Choose a reason for hiding this comment

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

in latest version, grpc server adds two reflection services (one stable and one alpha)

@@ -87,7 +91,7 @@ if(GWHISPER_FORCE_BUILDING_GRPC OR GRPC_NOT_FOUND)
if(CMAKE_SYSTEM_PROCESSOR STREQUAL "s390x")
set(gRPC_SSL_PROVIDER "package" CACHE STRING "force overwritten by gWhisper, as boringSSL does not support s390x -> need to fall-back to system installed libssl" FORCE )
endif()

set(ABSL_PROPAGATE_CXX_STD ON CACHE STRING "force overwritten by gWhisper to conform to Abseil recommendations" FORCE )
Copy link
Member Author

Choose a reason for hiding this comment

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

A build warning was given to set this. So setting here.

google::protobuf::util::Status status = google::protobuf::util::MessageToJsonString(f_message, &resultString, printOptions);

auto status = google::protobuf::util::MessageToJsonString(f_message, &resultString, printOptions);
Copy link

Choose a reason for hiding this comment

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

const is your friend ;-)

Comment on lines 57 to 59
std::string fileContent = buffer.str();
auto status = google::protobuf::util::JsonStringToMessage(
google::protobuf::StringPiece(fileContent),
fileContent,
Copy link

Choose a reason for hiding this comment

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

Actually you don't need a temporay (fileContent) here, you could pass it directly as parameter to JsonStringToMessage()

Signed-off-by: Rainer Schoenberger <[email protected]>
@rainerschoe rainerschoe requested a review from ja66 February 26, 2024 07:03
@rainerschoe rainerschoe merged commit d785830 into master Feb 26, 2024
2 checks passed
@rainerschoe rainerschoe deleted the update_grpc branch February 26, 2024 10:14
@rainerschoe
Copy link
Member Author

Thanks Jens for reporting and reviewing 🥇
I assume this fixes #151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants