Skip to content

Miscellaneous google header insulation #346

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 3, 2025
Merged

Miscellaneous google header insulation #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a1dc6f
remove google headers from registry.hpp
lia-viam Jan 2, 2025
070c340
use forward decls in rpc server
lia-viam Jan 2, 2025
e349ca0
rename and update comments for fwd header
lia-viam Jan 2, 2025
0f81a8d
add misc include
lia-viam Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ if ((VIAMCPPSDK_GRPCXX_VERSION VERSION_GREATER 1.51.1) AND (VIAMCPPSDK_PROTOC_VE
endif()

if (VIAMCPPSDK_GRPCXX_VERSION VERSION_LESS 1.32.0)
set(VIAMCPPSDK_GRPCXX_LEGACY_CLIENT_FWD 1)
set(VIAMCPPSDK_GRPCXX_LEGACY_FWD 1)
endif()

include(FetchContent)
Expand Down
4 changes: 2 additions & 2 deletions src/viam/sdk/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ message(WARNING "api proto label is ${VIAMCPPSDK_API_PROTO_LABEL}")
configure_file(common/private/version_metadata.hpp.in common/private/version_metadata.hpp @ONLY)

# Configure the grpc client forward declarations file
configure_file(common/grpc_client_fwd.hpp.in common/grpc_client_fwd.hpp)
configure_file(common/grpc_fwd.hpp.in common/grpc_fwd.hpp)

# Set compile and link options based on arguments
if (VIAMCPPSDK_USE_WALL_WERROR)
Expand Down Expand Up @@ -192,7 +192,7 @@ target_sources(viamsdk
../../viam/sdk/spatialmath/geometry.hpp
../../viam/sdk/spatialmath/orientation.hpp
../../viam/sdk/spatialmath/orientation_types.hpp
${CMAKE_CURRENT_BINARY_DIR}/../../viam/sdk/common/grpc_client_fwd.hpp
${CMAKE_CURRENT_BINARY_DIR}/../../viam/sdk/common/grpc_fwd.hpp
)

set_target_properties(
Expand Down
2 changes: 1 addition & 1 deletion src/viam/sdk/common/client_helper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pragma once

#include <viam/sdk/common/exception.hpp>
#include <viam/sdk/common/grpc_client_fwd.hpp>
#include <viam/sdk/common/grpc_fwd.hpp>
#include <viam/sdk/common/private/utils.hpp>
#include <viam/sdk/common/proto_value.hpp>

Expand Down
63 changes: 0 additions & 63 deletions src/viam/sdk/common/grpc_client_fwd.hpp.in
View file
Open in desktop

This file was deleted.

89 changes: 89 additions & 0 deletions src/viam/sdk/common/grpc_fwd.hpp.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
#pragma once

#cmakedefine VIAMCPPSDK_GRPCXX_LEGACY_FWD

// Forward declaration file grpc client and server types.
// This file provides includes for recent (>= 1.32.0) versions of grpc or older
// versions, depending on if `VIAMCPPSDK_GRPCXX_LEGACY_FWD` is defined.
// The default behavior is for the variable to be undefined, providing the behavior for recent
// versions. If you are experiencing compilation errors in an installed version of the SDK it may be
// due to a mismatch with the configured version of this header and the grpc version found by the
// compiler. You may wish to comment/uncomment the define above as needed, or add the definition
// with `-D` to the compiler.
Comment on lines +11 to +12
Copy link
Member

@stuqdog stuqdog Jan 3, 2025

Choose a reason for hiding this comment

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

(q) just to confirm, is this a standard practice we'd expect people to be comfortable with/know how to do? Specifically, the idea (and syntax) of defining the variable for the compiler, and also the fact that we don't care how it's defined just so long as it is defined? I think the latter is probably pretty clear, but for the former I'd definitely be running to the internet to get more advice on how to proceed. That could just be my ignorance though!

Copy link
Collaborator Author

@lia-viam lia-viam Jan 3, 2025

Choose a reason for hiding this comment

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

I think so yes! This is CMake's version of Make/autoconf-generated config.h type headers, where a bunch of stuff will be #define'd or commented out.

The default case which I tried to expand on here is that we expect the variable to not be defined, because that corresponds to some pretty ancient grpc versions.

The other thing I tried to expand on was that this is a configured file which will then be installed with make install, so it's possible but a little weird that the user configures it with one version of grpc, does make install, and then later wants to use that installed version with a different grpc version, in which case they can manipulate the macro definitions with commetning/uncommenting or -D to make it work

#ifndef VIAMCPPSDK_GRPCXX_LEGACY_FWD

namespace grpc {

class Channel;

class ClientContext;

template <typename>
class ClientReaderInterface;

class Server;

class ServerBuilder;

class ServerCredentials;

} // namespace grpc

namespace viam {
namespace sdk {

using GrpcChannel = ::grpc::Channel;

using GrpcClientContext = ::grpc::ClientContext;

template <typename T>
using GrpcClientReaderInterface = ::grpc::ClientReaderInterface<T>;

using GrpcServer = ::grpc::Server;

using GrpcServerBuilder = ::grpc::ServerBuilder;

using GrpcServerCredentials = ::grpc::ServerCredentials;

} // namespace sdk
} // namespace viam

#else

namespace grpc_impl {

class Channel;

class ClientContext;

template <typename>
class ClientReaderInterface;

class Server;

class ServerBuilder;

class ServerCredentials;

} // namespace grpc_impl

namespace viam {
namespace sdk {

using GrpcChannel = ::grpc_impl::Channel;

using GrpcClientContext = ::grpc_impl::ClientContext;

template <typename T>
using GrpcClientReaderInterface = ::grpc_impl::ClientReaderInterface<T>;

using GrpcServer = ::grpc_impl::Server;

using GrpcServerBuilder = ::grpc_impl::ServerBuilder;

using GrpcServerCredentials = ::grpc_impl::ServerCredentials;

} // namespace sdk
} // namespace viam

#endif
2 changes: 1 addition & 1 deletion src/viam/sdk/module/module.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include <viam/sdk/common/grpc_client_fwd.hpp>
#include <viam/sdk/common/grpc_fwd.hpp>
#include <viam/sdk/module/handler_map.hpp>
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/resource/resource_manager.hpp>
Expand Down
21 changes: 12 additions & 9 deletions src/viam/sdk/registry/registry.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,29 @@

#include <string>

#include <google/protobuf/descriptor.h>
#include <google/protobuf/message.h>
#include <grpcpp/impl/service_type.h>
#include <grpcpp/server.h>

#include <viam/sdk/common/grpc_client_fwd.hpp>
#include <viam/sdk/common/grpc_fwd.hpp>
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/resource/resource_api.hpp>
#include <viam/sdk/resource/resource_manager.hpp>
#include <viam/sdk/resource/resource_server_base.hpp>
#include <viam/sdk/rpc/server.hpp>

namespace google {
namespace protobuf {

class ServiceDescriptor;

}
} // namespace google

namespace viam {
namespace sdk {

// TODO(RSDK-6617): one class per header
class ResourceServerRegistration {
public:
ResourceServerRegistration(const google::protobuf::ServiceDescriptor* service_descriptor);
ResourceServerRegistration(const ::google::protobuf::ServiceDescriptor* service_descriptor);

virtual ~ResourceServerRegistration();

Expand All @@ -36,10 +39,10 @@ class ResourceServerRegistration {
std::shared_ptr<ResourceManager> manager, Server& server) const = 0;

/// @brief Returns a reference to the `ResourceServerRegistration`'s service descriptor.
const google::protobuf::ServiceDescriptor* service_descriptor() const;
const ::google::protobuf::ServiceDescriptor* service_descriptor() const;

private:
const google::protobuf::ServiceDescriptor* service_descriptor_;
const ::google::protobuf::ServiceDescriptor* service_descriptor_;
};

/// @class ResourceClientRegistration registry.hpp "registry/registry.hpp"
Expand Down
3 changes: 2 additions & 1 deletion src/viam/sdk/robot/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
/// @brief gRPC client implementation for a `robot`.
#pragma once

#include <atomic>
#include <string>
#include <thread>

#include <viam/sdk/common/grpc_client_fwd.hpp>
#include <viam/sdk/common/grpc_fwd.hpp>
#include <viam/sdk/common/pose.hpp>
#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/common/world_state.hpp>
Expand Down
2 changes: 1 addition & 1 deletion src/viam/sdk/rpc/dial.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

#include <boost/optional.hpp>

#include <viam/sdk/common/grpc_client_fwd.hpp>
#include <viam/sdk/common/grpc_fwd.hpp>

namespace viam {
namespace sdk {
Expand Down
3 changes: 3 additions & 0 deletions src/viam/sdk/rpc/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
#include <sstream>

#include <boost/log/trivial.hpp>

#include <grpcpp/impl/service_type.h>
#include <grpcpp/security/server_credentials.h>
#include <grpcpp/server_builder.h>

#include <viam/sdk/common/exception.hpp>
#include <viam/sdk/registry/registry.hpp>
Expand Down
19 changes: 11 additions & 8 deletions src/viam/sdk/rpc/server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@
/// @brief Defines the `Server` class.
#pragma once

#include <grpcpp/impl/service_type.h>
#include <grpcpp/security/server_credentials.h>
#include <grpcpp/server_builder.h>

#include <viam/sdk/common/grpc_fwd.hpp>
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/resource/resource_api.hpp>
#include <viam/sdk/resource/resource_server_base.hpp>

namespace grpc {

class Service;

} // namespace grpc

namespace viam {

// needed for later `friend` declaration.
Expand All @@ -35,7 +38,7 @@ class Server {
/// @brief Registers a gRPC service.
/// @param service The gRPC service to be registered.
/// @throws `Exception` if called after the server has been `start`ed.
void register_service(grpc::Service* service);
void register_service(::grpc::Service* service);

/// @brief Returns reference to managed resource server.
/// @param api The api of the managed resource server.
Expand All @@ -52,7 +55,7 @@ class Server {
/// @param creds The server credentials; defaults to a insecure server credentials.
/// @throws `Exception` if called after the server has been `start`ed.
void add_listening_port(const std::string& address,
std::shared_ptr<grpc::ServerCredentials> creds = nullptr);
std::shared_ptr<GrpcServerCredentials> creds = nullptr);

/// @brief waits on server close, only returning when the server is closed.
void wait();
Expand All @@ -65,8 +68,8 @@ class Server {

private:
std::unordered_map<API, std::shared_ptr<ResourceServer>> managed_servers_;
std::unique_ptr<grpc::ServerBuilder> builder_;
std::unique_ptr<grpc::Server> server_;
std::unique_ptr<GrpcServerBuilder> builder_;
std::unique_ptr<GrpcServer> server_;
};

} // namespace sdk
Expand Down
Loading