Skip to content

Commit

Permalink
CHANGE(server): Remove gRPC implementation
Browse files Browse the repository at this point in the history
The gRPC implementation never left the experimental state and never
reached a properly stable state to the point where we would feel good
about enabling it by default. In addition to that, there has been no
further attempts at finding and fixing the encountered issues in the
implementation (except mumble-voip#3947 but that was discontinued).

As such we had an essentially unmaintained piece of code in our server
implementation that was known to be buggy and that nobody wanted to fix.
In addition to that the implementation itself could not be considered
very clean or elegant and therefore only represented a few smelly
corners in our code base.

For this reason, we decided to remove the gRPC support entirely from
Mumble (for now).

What we hope to gain by that is:
- Prevent people from building unstable server versions and then coming
to us complaining that it crashed/misbehaved
- Removing (essentially) dead code
- Reduce the RPC implementation complexity

That last piece is crucial: By removing gRPC support we reduce the
amount of supported RPC frameworks to only one (ignoring DBus for now).
Our future plans include a refactoring of how RPC is being handled and
implemented and only having to worry about maintaining compatibility
with one RPC system is much easier than having to worry about two (with
(slightly) different APIs).
Once the RPC implementation has been rewritten, more RPC backends may be
reintroduced and in that process we might investigate adding a proper
gRPC implementation to the code (that then hopefully is more stable than
the current one).

Fixes mumble-voip#4567
Fixes mumble-voip#4197
Fixes mumble-voip#3496
Fixes mumble-voip#3429
Fixes mumble-voip#3265
  • Loading branch information
Krzmbrzl committed Mar 16, 2022
1 parent 2dab6a2 commit b3dd3d3
Show file tree
Hide file tree
Showing 30 changed files with 10 additions and 4,086 deletions.
2 changes: 1 addition & 1 deletion .ci/azure-pipelines/build_linux.bash
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ cd $BUILD_BINARIESDIRECTORY

cmake -G Ninja -DCMAKE_INSTALL_PREFIX=appdir/usr -DCMAKE_UNITY_BUILD=ON \
-DCMAKE_BUILD_TYPE=Release -DBUILD_NUMBER=$BUILD_NUMBER \
-Dtests=ON -Dsymbols=ON -Dgrpc=ON \
-Dtests=ON -Dsymbols=ON \
-Ddisplay-install-paths=ON $BUILD_SOURCESDIRECTORY

cmake --build .
Expand Down
2 changes: 1 addition & 1 deletion .ci/azure-pipelines/build_macos.bash
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ cd $BUILD_BINARIESDIRECTORY

cmake -G Ninja -DCMAKE_TOOLCHAIN_FILE=$MUMBLE_ENVIRONMENT_TOOLCHAIN -DIce_HOME="$MUMBLE_ENVIRONMENT_PATH/installed/x64-osx" \
-DCMAKE_BUILD_TYPE=Release -DCMAKE_UNITY_BUILD=ON -DBUILD_NUMBER=$BUILD_NUMBER \
-Dtests=ON -Dstatic=ON -Dsymbols=ON -Dgrpc=ON \
-Dtests=ON -Dstatic=ON -Dsymbols=ON \
-Ddisplay-install-paths=ON $BUILD_SOURCESDIRECTORY

cmake --build .
Expand Down
3 changes: 1 addition & 2 deletions .ci/azure-pipelines/install-environment_linux.bash
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,4 @@ sudo apt-get -y install build-essential g++-multilib ninja-build pkg-config \
libasound2-dev libasound2-plugins libasound2-plugins-extra\
libogg-dev libsndfile1-dev libspeechd-dev \
libavahi-compat-libdnssd-dev libzeroc-ice-dev \
zsync appstream libgrpc++-dev protobuf-compiler-grpc \
libpoco-dev
zsync appstream libpoco-dev
2 changes: 1 addition & 1 deletion .ci/build_windows.bat
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ cmake -G Ninja -DCMAKE_TOOLCHAIN_FILE="%MUMBLE_ENVIRONMENT_TOOLCHAIN%" -DVCPKG_T
-DIce_HOME="%MUMBLE_ENVIRONMENT_PATH%\installed\%MUMBLE_ENVIRONMENT_TRIPLET%" ^
-DCMAKE_C_COMPILER=cl.exe -DCMAKE_CXX_COMPILER=cl.exe ^
-DCMAKE_BUILD_TYPE=Release -DCMAKE_UNITY_BUILD=ON -DBUILD_NUMBER=%BUILD_NUMBER% ^
-Dpackaging=ON -Dtests=ON -Dstatic=ON -Dsymbols=ON -Dgrpc=ON -Dasio=ON -Dg15=ON ^
-Dpackaging=ON -Dtests=ON -Dstatic=ON -Dsymbols=ON -Dasio=ON -Dg15=ON ^
-Ddisplay-install-paths=ON -Delevation=%MUMBLE_USE_ELEVATION% %MUMBLE_SOURCE_REPOSITORY%

if errorlevel 1 (
Expand Down
4 changes: 2 additions & 2 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ freebsd_instance:
freebsd_task:
pkg_script:
- pkg update && pkg upgrade -y
- pkg install -y git ninja pkgconf cmake qt5-buildtools qt5-qmake qt5-linguisttools qt5-concurrent qt5-network qt5-xml qt5-sql qt5-svg qt5-testlib boost-libs libsndfile protobuf ice avahi-libdns grpc poco
- pkg install -y git ninja pkgconf cmake qt5-buildtools qt5-qmake qt5-linguisttools qt5-concurrent qt5-network qt5-xml qt5-sql qt5-svg qt5-testlib boost-libs libsndfile protobuf ice avahi-libdns poco
fetch_submodules_script: git submodule --quiet update --init --recursive
build_script:
- mkdir build && cd build
# We disable translations because of a critical issue in "lupdate": it's very slow and keeps CPU usage at 100%.
- cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -Dtests=ON -Dsymbols=ON -Dtranslations=OFF -Dgrpc=ON ..
- cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -Dtests=ON -Dsymbols=ON -Dtranslations=OFF ..
- cmake --build .
test_script:
- cd build
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,4 @@ sudo apt -y install \
libzeroc-ice-dev \
zsync \
appstream \
libgrpc++-dev \
protobuf-compiler-grpc \
libpoco-dev
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ env:
# Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
BUILD_TYPE: Release
CMAKE_OPTIONS: |
-Dtests=ON -Dsymbols=ON -Dgrpc=ON -Ddisplay-install-paths=ON
-Dtests=ON -Dsymbols=ON -Ddisplay-install-paths=ON
MUMBLE_ENVIRONMENT_SOURCE: 'https://dl.mumble.info/build/vcpkg/'


Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/code-ql.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ env:
# Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
BUILD_TYPE: Release
CMAKE_OPTIONS: |
-Dtests=OFF -Dsymbols=ON -Dgrpc=ON
-Dtests=OFF -Dsymbols=ON
jobs:
CodeQL-Build:
Expand Down
1 change: 0 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ set(CMAKE_OSX_DEPLOYMENT_TARGET 10.13)

list(APPEND CMAKE_MODULE_PATH
"${CMAKE_SOURCE_DIR}/cmake"
"${CMAKE_SOURCE_DIR}/cmake/FindModules"
"${3RDPARTY_DIR}/FindPythonInterpreter"
)

Expand Down
2 changes: 1 addition & 1 deletion COMMIT_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ combine types with `/`: `FEAT/CI: <Summary>`
### Scope

What area is the change about. For now we don't have fixed scope keywords. A scope could be something like `ui`, `client`,
`server`, `ice`, `grpc`, ...
`server`, `ice`, ...


### Summary
Expand Down
6 changes: 1 addition & 5 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@ RUN apt-get update && apt-get install --no-install-recommends -y \
libcap-dev \
libprotobuf-dev \
protobuf-compiler \
protobuf-compiler-grpc \
libprotoc-dev \
libogg-dev \
libavahi-compat-libdnssd-dev \
libsndfile1-dev \
libgrpc++-dev \
libxi-dev \
libbz2-dev \
&& apt-get clean \
Expand All @@ -34,7 +32,7 @@ COPY . /root/mumble
WORKDIR /root/mumble/build

RUN git submodule update --init --recursive
RUN cmake -Dclient=OFF -DCMAKE_BUILD_TYPE=Release -Dgrpc=ON .. || \
RUN cmake -Dclient=OFF -DCMAKE_BUILD_TYPE=Release .. || \
( cat \
/root/mumble/build/CMakeFiles/CMakeOutput.log \
/root/mumble/build/CMakeFiles/CMakeError.log \
Expand All @@ -52,8 +50,6 @@ RUN apt-get update && apt-get install --no-install-recommends -y \
libcap2 \
libzeroc-ice3.7 \
'^libprotobuf[0-9]+$' \
'^libgrpc[0-9]+$' \
libgrpc++1 \
libavahi-compat-libdnssd1 \
libqt5core5a \
libqt5network5 \
Expand Down
93 changes: 0 additions & 93 deletions cmake/FindModules/FindGRPC.cmake

This file was deleted.

1 change: 0 additions & 1 deletion docs/dev/TheMumbleSourceCode.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ When cloning the repo, the source tree should look something like this:
│   ├── mumble
│   ├── mumble_exe
│   ├── murmur
│   ├── murmur_grpcwrapper_protoc_plugin
│   └── tests
└── themes
└── Default
Expand Down
3 changes: 0 additions & 3 deletions docs/dev/build-instructions/build_linux.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ sudo apt install \
g++-multilib
```

If you intend to include grpc-support for the Mumble server (murmur), you also have to install the following packages: `libgrpc++-dev` and
`protobuf-compiler-grpc`

The dependence on `g++-multilib` only applies if you are on a 64bit system and want to cross-compile overlay support for 32bit applications as well
(which is enabled by default). If you don't do this (`-Doverlay-xcompile=OFF` when invoking cmake), you also don't have to install `g++-multilib`.

Expand Down
1 change: 0 additions & 1 deletion docs/dev/build-instructions/build_static.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ qt5-base
qt5-svg
qt5-tools
qt5-translations
grpc
boost-accumulators
opus
poco
Expand Down
5 changes: 0 additions & 5 deletions docs/dev/build-instructions/cmake_options.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,6 @@ Build the g15helper executable in emulator mode. This will cause an emulated G15
Build support for Logitech G-Keys. Note: This feature does not require any build-time dependencies, and requires Logitech Gaming Software to be installed to have any effect at runtime.
(Default: ON)

### grpc

Build support for gRPC.
(Default: OFF)

### ice

Build support for Ice RPC.
Expand Down
16 changes: 0 additions & 16 deletions scripts/murmur.ini
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,6 @@ ice="tcp -h 127.0.0.1 -p 6502"
;icesecretread=
icesecretwrite=

; If you want to expose Murmur's experimental gRPC API, you
; need to specify an address to bind on.
; Note: not all builds of Murmur support gRPC. If gRPC is not
; available, Murmur will warn you in its log output.
;grpc="127.0.0.1:50051"
; Specifying both a certificate and key file below will cause gRPC to use
; secured, TLS connections.
; When using secured connections you need to also set the list of authorized
; clients. grpcauthorized is a space delimited list of SHA256 fingerprints
; for authorized client certificates.
; Get this from the command line:
; openssl x509 -in cert.pem -SHA256 -noout -fingerprint
;grpccert=""
;grpckey=""
;grpcauthorized=""

; Specifies the file Murmur should log to. By default, Murmur
; logs to the file 'murmur.log'. If you leave this field blank
; on Unix-like systems, Murmur will force itself into foreground
Expand Down
1 change: 0 additions & 1 deletion scripts/vcpkg/get_mumble_dependencies.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ $mumble_deps = "qt5-base",
"qt5-svg",
"qt5-tools",
"qt5-translations",
"grpc",
"boost-accumulators",
"poco",
"libvorbis",
Expand Down
1 change: 0 additions & 1 deletion scripts/vcpkg/get_mumble_dependencies.sh
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ mumble_deps='qt5-base,
qt5-svg,
qt5-tools,
qt5-translations,
grpc,
boost-accumulators,
poco,
libvorbis,
Expand Down
74 changes: 0 additions & 74 deletions src/murmur/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@ set(MURMUR_PLIST "${CMAKE_CURRENT_BINARY_DIR}/murmur.plist")
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/murmur.plist.in" "${MURMUR_PLIST}")
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/murmur.rc.in" "${MURMUR_RC}")

set(GRPC_FILE "${CMAKE_CURRENT_SOURCE_DIR}/MurmurRPC.proto")
set(ICE_FILE "${CMAKE_CURRENT_SOURCE_DIR}/Murmur.ice")

include(qt-utils)

option(grpc "Build support for gRPC." OFF)
option(ice "Build support for Ice RPC." ON)

find_pkg(Qt5 COMPONENTS Sql REQUIRED)
Expand Down Expand Up @@ -198,78 +196,6 @@ if(dbus AND NOT WIN32 AND NOT APPLE)
target_link_libraries(mumble-server PRIVATE Qt5::DBus)
endif()

if(grpc)
# "gRPC" is the name of the official gRPC target whereas "GRPC" is the one used by our custom FindGRPC.cmake in
# the cmake sub-dir at the project's root.
find_pkg("gRPC;GRPC" REQUIRED)

protobuf_generate(LANGUAGE cpp TARGET mumble-server PROTOS ${GRPC_FILE} OUT_VAR BUILT_GRPC_FILES)

add_subdirectory("${SHARED_SOURCE_DIR}/murmur_grpcwrapper_protoc_plugin" "${CMAKE_CURRENT_BINARY_DIR}/murmur_grpcwrapper_protoc_plugin")

get_filename_component(GRPC_FILE_NAME ${GRPC_FILE} NAME_WE)

add_custom_command(
OUTPUT "MurmurRPC.proto.Wrapper.cpp"
COMMAND protobuf::protoc
ARGS "--plugin=$<TARGET_FILE:protoc-gen-murmur-grpcwrapper>" "-I${CMAKE_CURRENT_SOURCE_DIR}" "--murmur-grpcwrapper_out=${CMAKE_CURRENT_BINARY_DIR}" ${GRPC_FILE}
DEPENDS protobuf::protoc protoc-gen-murmur-grpcwrapper ${GRPC_FILE}
COMMENT "Generating gRPC wrapper"
VERBATIM
# protoc doesn't seem to parse the plugin path correctly on Windows: it only finds the executable when in the current directory or in PATH.
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/murmur_grpcwrapper_protoc_plugin"
)

set(GRPC_GENERATED_FILES
"MurmurRPC.grpc.pb.cc"
"MurmurRPC.grpc.pb.h"
)

add_custom_command(
OUTPUT ${GRPC_GENERATED_FILES}
COMMAND protobuf::protoc
ARGS "--plugin=protoc-gen-grpc=$<TARGET_FILE:gRPC::grpc_cpp_plugin>" "-I${CMAKE_CURRENT_SOURCE_DIR}" "--grpc_out=${CMAKE_CURRENT_BINARY_DIR}" ${GRPC_FILE}
DEPENDS protobuf::protoc gRPC::grpc_cpp_plugin ${GRPC_FILE}
COMMENT "Running gRPC compiler"
VERBATIM
)

# Disable warnings for the generated source files
foreach(CURRENT_FILE IN LISTS GRPC_GENERATED_FILES BUILT_GRPC_FILES)
set_source_files_properties("${CURRENT_FILE}" PROPERTIES COMPILE_FLAGS "-w")
endforeach()

add_custom_target(generate-grpc-files
DEPENDS
"MurmurRPC.proto.Wrapper.cpp"
${GRPC_GENERATED_FILES}
)

add_dependencies(mumble-server generate-grpc-files)

target_sources(mumble-server
PRIVATE
"MurmurGRPCImpl.cpp"
"MurmurGRPCImpl.h"
${GRPC_GENERATED_FILES}
)

if(NOT MSVC)
# Disable the unused parameter warning for this file as it implicitly includes one of the generated
# files that contains a lot of unused parameters that would otherwise create a warning.
set_source_files_properties("MurmurGRPCImpl.cpp" PROPERTIES COMPILE_FLAGS "-Wno-unused-parameter")
endif()

target_compile_definitions(mumble-server PRIVATE "USE_GRPC")
target_include_directories(mumble-server PRIVATE ${CMAKE_CURRENT_BINARY_DIR})

target_link_libraries(mumble-server PRIVATE gRPC::grpc++)

if (TARGET gRPC::gpr)
target_link_libraries(mumble-server PRIVATE gRPC::gpr)
endif()
endif()

if(ice)
find_pkg(Ice
COMPONENTS
Expand Down
5 changes: 0 additions & 5 deletions src/murmur/Meta.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,11 +283,6 @@ void MetaParams::read(QString fname) {
qsIceSecretRead = typeCheckedFromSettings("icesecretread", qsIceSecretRead);
qsIceSecretWrite = typeCheckedFromSettings("icesecretwrite", qsIceSecretRead);

qsGRPCAddress = typeCheckedFromSettings("grpc", qsGRPCAddress);
qsGRPCCert = typeCheckedFromSettings("grpccert", qsGRPCCert);
qsGRPCKey = typeCheckedFromSettings("grpckey", qsGRPCKey);
qsGRPCAuthorized = typeCheckedFromSettings("grpcauthorized", qsGRPCAuthorized);

iLogDays = typeCheckedFromSettings("logdays", iLogDays);

qsDBus = typeCheckedFromSettings("dbus", qsDBus);
Expand Down
Loading

0 comments on commit b3dd3d3

Please sign in to comment.