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

Build Murmur with gRPC by default #3429

Closed
2 tasks
HeroCC opened this issue Jun 28, 2018 · 9 comments · Fixed by #5552
Closed
2 tasks

Build Murmur with gRPC by default #3429

HeroCC opened this issue Jun 28, 2018 · 9 comments · Fixed by #5552
Labels

Comments

@HeroCC
Copy link

HeroCC commented Jun 28, 2018

Hello!

I saw that Murmur now has support for gRPC instead of Ice for communications as of #1196. However, it doesn't look like the publicly build image has support for that built in. Would it be possible for gRPC to be enabled when building Murmur on all platforms? I am testing from a Mac and would love to not have to compile mumble and murmur manually.

Thanks!


Status

@ghost ghost added the gRPC label Jun 28, 2018
@HeroCC
Copy link
Author

HeroCC commented Jun 28, 2018

This might be a separate issue, but in the wiki enabling gRPC as a step would be helpful as well.

@davidebeatrici
Copy link
Member

Hello!

gRPC support is still experimental:

mumble/INSTALL

Lines 149 to 156 in 6195761

CONFIG+=grpc (Murmur, Linux)
Include experimental support for gRPC.
We do not currently recommend that packagers
ship packages to end users with gRPC enabled.
The gRPC library's API is not yet stable, and
Murmur's gRPC interface is not yet frozen, and
will most likely change in the future.
Support for gRPC requires Qt 5.

I added a link to the INSTALL file on the Wiki.

@Kissaki

This comment has been minimized.

@Kissaki

This comment has been minimized.

@Kissaki
Copy link
Member

Kissaki commented Jul 1, 2018

gRPC library's API is not yet stable

Is that still the case?

It’s way past a 1.0 release and I see no mention of unstabledness on the grpc website, about or FAQ.

@HeroCC HeroCC changed the title Build Murmur with gRPC Build Murmur with gRPC by default Jul 1, 2018
@HeroCC
Copy link
Author

HeroCC commented Jul 1, 2018

I'm not a Mumble developer by any means, but looking at gRPC when that comment was written and now, there have been 20672 commits to the repo; and they have released multiple stable releases for all OSes since then. The MurmurRPC.proto file hasn't been touched (except for license header updates) since 2015, suggesting that it is reasonably frozen for now. I would say especially now with issues using Ice on newer platforms this would be a good time prior to the full 1.3 release to re-enable gRPC by default.

@bontibon, I saw you worked on most of the gRPC features in Mumble, would you mind weighing in as well?

@HeroCC
Copy link
Author

HeroCC commented Jul 2, 2018

I made a PR that would resolve this ticket, if you could take a look that would be much appreciated!

@Krzmbrzl
Copy link
Member

If I compare https://github.com/mumble-voip/mumble/blob/4976c1ad2e0802755c15d051783403afc440b173/src/murmur/Murmur.ice with https://github.com/mumble-voip/mumble/blob/master/src/murmur/MurmurRPC.proto I'd say that the gRPC API doesn't provide the same functionality as the ICE interface.

From what I understand, gRPC shall replace the ICE framework. And as such the least I'd expect is that it provides the same functionality as is currently provided by ICE. But as I said I don't believe this is the case (yet).

If the above is correct I'm not sure whether it already makes sense to add the gRPC just yet 🤔

@Kissaki
Copy link
Member

Kissaki commented May 21, 2020

I added a status section to the original post and linked the (undefined but vaguely mentioned) segfaults and incompleteness as open. The first of the two being the blocker.

Krzmbrzl added a commit to Krzmbrzl/mumble that referenced this issue Feb 6, 2022
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
Krzmbrzl added a commit to Krzmbrzl/mumble that referenced this issue Mar 16, 2022
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
Krzmbrzl added a commit that referenced this issue Mar 16, 2022
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 #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 #4567
Fixes #4197
Fixes #3496
Fixes #3429
Fixes #3265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants