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

WIP: Move Mumble Socket and Overlay to $XDG_RUNTIME_DIR/mumble/ #5961

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

carlocastoldi
Copy link
Contributor

Moving MumbleSocket and MumbleOverlayPipe to a dedicated subdirectory keeps the runtime directory clean and allows flatpak applications to use the overlay by giving access only to Mumble's subdirectory. It also moves the default directory to /run/user/$UID/mumble/ when $XDG_RUNTIME_DIR is not set.

Fixes #5951

See also: flathub/com.valvesoftware.Steam#273

Checks

@carlocastoldi
Copy link
Contributor Author

I thought of using QtStandardPaths, but if $XDG_RUNTIME_DIR is not set it defaults to unpredictable directories. From the docs it should point to /run/user/$UID/, but with me it defaulted to /tmp/runtime-carlo/

Use of QtStandardPaths::RuntimeLocation
		QString runtimePath = QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation);
		QString mumbleRuntimePath = QDir(runtimePath).absolutePath() + QLatin1String("/mumble/");
		QDir().mkpath(mumbleRuntimePath);
		pipepath = QDir(mumbleRuntimePath).absoluteFilePath(QLatin1String("MumbleOverlayPipe"));

@Krzmbrzl
Copy link
Member

What is that tags file that has been added in this PR?

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

It might make sense to create a new function for obtaining the respective path so we don't have to write the logic in multiple places (with the potential of screwing up one of the times or one version getting outdated because it was overlooked).
I would propose writing a function returning a std::string and everything that needs something else can convert from that.

@Krzmbrzl
Copy link
Member

I suppose that old overlays won't work with new Mumble versions and vice versa after this PR. I guess that's fine as it seems like an unlikely scenario.

However, existing IPC scripts will also stop working, if XDG_RUNTIME is not set. But then again, from your description it seemed that this special case was ill-defined anyway, so I wouldn't consider it likely that a lot of scripts exist for this special case...
Thus, this should be fine as well 🤷

@carlocastoldi
Copy link
Contributor Author

I would propose writing a function returning a std::string and everything that needs something else can convert from that.

Yeah I like the idea. Where would I put this util function, tho? I'm not accustomed to Mumble's code organization, yet

@carlocastoldi
Copy link
Contributor Author

Btw: do you want me to include in this PR the fix for OverlayTest? So that it can be run again? I found it super useful to test the change of this PR in a flatpak environment

@carlocastoldi
Copy link
Contributor Author

I'm afraid OverlayTest does not actually test anything, does it? CI gets stuck

@davidebeatrici
Copy link
Member

Well... how can it work if Mumble is not running?

@carlocastoldi
Copy link
Contributor Author

Well... how can it work if Mumble is not running?

That is not even the issue. OverlayTest is an application for humans to check whether the overlay is properly displayed or not. I disabled it by default, but it is being built anyway.

@carlocastoldi carlocastoldi force-pushed the feature/xdg-runtime-dir branch 6 times, most recently from 69d6faf to e55e979 Compare November 16, 2022 12:17
@Krzmbrzl Krzmbrzl added the bug A bug (error) in the software label Nov 27, 2022
src/mumble/IPCUtils.h Outdated Show resolved Hide resolved
src/mumble/IPCUtils.h Outdated Show resolved Hide resolved
src/mumble/CMakeLists.txt Outdated Show resolved Hide resolved
src/mumble/CMakeLists.txt Outdated Show resolved Hide resolved
src/mumble/IPCUtils_linux.cpp Outdated Show resolved Hide resolved
src/mumble/IPCUtils_linux.cpp Outdated Show resolved Hide resolved
src/mumble/Overlay.cpp Outdated Show resolved Hide resolved
src/mumble/SocketRPC.cpp Outdated Show resolved Hide resolved
src/mumble/SocketRPC.cpp Outdated Show resolved Hide resolved
overlay_gl/overlay.c Outdated Show resolved Hide resolved
@carlocastoldi carlocastoldi force-pushed the feature/xdg-runtime-dir branch 3 times, most recently from 128423c to fedacf0 Compare November 29, 2022 18:57
@Krzmbrzl
Copy link
Member

I have not forgotten this, but I still don't have the time to look into this

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 7, 2023

sigh... let's try to ignore that it took me half a year to take another look at this. Big sorry for that 🙈

I had another look at the current implementation and I think most of the complexity comes from the fact that doing the path-related logic in C is a bit of a PITA.
Therefore, my suggestion for creating a properly maintainable implementation would be this, would be to use C++ for implementing all the logic and instead of writing cpp wrappers for the C implementations, we'll do it the other way around. So the hierarchy I'm having in mind would be something like

namespace mumble {
namespace overlay {

	std::string getIPCPath();

	std::string getSocketPath();

	std::string getPipePath();

	std::string createSocket();

	std::string createPipe();

}
}


extern "C" {

char *mumble_overlay_create_and_get_socket();

char *mumble_overlay_create_and_get_pipe();

}

The respective wrappers would then simply call the C++ implementation under the hood, allocate a new char buffer of the appropriate size, copy the contents over and then return that freshly allocated buffer for use in C world.

Inside the C++ implementations, we can then make use of Qt (for lack of C++17 support in Mumble) to do the filesystem operations (in particular things like querying the user's home directory) and then only do the actual creation of the pipe and socket in a platform dependent manner. That seems to save a lot of complexity that I am currently seeing in the C implementation.

In order for all to work together, I think the easiest solution would be to make the overlay utils part of the shared codebase - that is, the code living directly under src/ - and then link the shared target to the overlay one. In order for the inclusion of the header to work, we probably have to do something like

#ifdef __cplusplus
// C++ declarations here -> including namespaces

extern "C" {
#endif

// C declarations here

#ifdef __cplusplus
}
#endif 

which is not terribly pretty but should get the job done, without devastating readability 🤔

This would of course mean, that we can then never build the overlay client without building the shared target, which in turn brings many new dependencies (not least of which is Qt). However, I don't really see a scenario where someone would want to build the overlay client, but not Mumble itself (at least the client), so I don't think this would be a problem.

And once we make the switch to C++17, we could even create another, standalone, static library that makes uses of std::filesystem to replace the Qt parts that I suggest using for the implementation.

Thoughts?
/CC @davidebeatrici

@carlocastoldi do you even still want to work on this? If not, I could totally understand.

@davidebeatrici
Copy link
Member

Alternatively, we could use cwalk.

@carlocastoldi
Copy link
Contributor Author

I rather like the idea of using a third party C library (cwalk) instead of choosing to entangle the overlay code with client's.
I personally worked on #5967 to decouple the overlay build from client's so that I could easily build a flatpak extension for mumble's overlay (see com.valvesoftware.Steam.Utility.MumbleOverlay) . I could see similar needs for other package formats (e.g. 32bit overlay)

carlocastoldi do you even still want to work on this? If not, I could totally understand.

I sure want. It's just a matter of time. I recently started a new job so I have fewer free time to spend on this.
But I'd like to close this PR myself ^^

P.S. a little reminder: I started all of this work so that it would be possible to have the overlay on flatpak steam games. The next major work I would like to work on is to move away the overlay from using shared memory, onto exclusively using local sockets (branch#socket-overlay-not-chunked and branch#socket-overlay-chunked)

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 9, 2023

Alright - using a 3rdParty library would also be fine for me. However it seems like cwalk doesn't provide a function to query the user's home directory - unless normalizing ~ will yield the expected result. But have a look at it and decide for yourself.

Another option could be to create a completely separate target that implements this functionality and that the overlay and the Mumble client both link to. Then inside this function we could use even simply use C++17 (making sure to not accidentally export STL symbols).
That way, the client and the overlay would remain unentangled as bother would simply rely on that external library.

@carlocastoldi
Copy link
Contributor Author

If I wanted to use cwalk, would it be ok for you if I used it only in ipc_utils.c for the C backend? I would then keep exporting them in C++ with IPCUtils.cpp.

However it seems like cwalk doesn't provide a function to query the user's home directory

True. However this is only needed in BSD/MacOS and it's a matter of 5 easy lines. I don't see it as a deal breaker. Moreover, the process for retrieving the home not so different from retrieving the runtime directory on Linux.

Also: are we sure both BSD and MacOS suggest using the home directory for storing this kind of files? Isn't there a standard similar to what we have on Linux?
I mean.. it's not something we have to discuss over on this PR, but the need for retrieving the home directory for BSD/Mac might just disappear in a next PR

Moving MumbleSocket and MumbleOverlayPipe to a dedicated subdirectory keeps the runtime directory clean and allows flatpak applications to use the overlay by giving access only to Mumble's subdirectory.
It also moves the default directory to /run/user/$UID/mumble/ when $XDG_RUNTIME_DIR is not set.

Fixes mumble-voip#5951
adds a test application that connects to MumbleOverlayPipe and displays the overlay as configured.
It's disabled by default as it is designed to be a check for humans
@carlocastoldi
Copy link
Contributor Author

Hell. I gave it a try and included cwalk into the PR. Now IPC utils uses an external library to handle OS-specific paths

@carlocastoldi carlocastoldi force-pushed the feature/xdg-runtime-dir branch 2 times, most recently from 2e683ff to b1c6089 Compare October 9, 2023 11:38
@carlocastoldi carlocastoldi changed the title Move Mumble Socket and Overlay to $XDG_RUNTIME_DIR/mumble/ WIP: Move Mumble Socket and Overlay to $XDG_RUNTIME_DIR/mumble/ Oct 9, 2023
@carlocastoldi
Copy link
Contributor Author

I had an hard time figuring out whether it was possible to compile and link cwalk both in 32 and 64bits in the same build as a 3rdparty library/dependency.
For this reason I opted to use snfprintf(). It basically does the same think as cwk_path_join(), but with no path resolution. I don't feel like that's a a must for this PR, however.

P.S. i marked this PR as WIP as at the moment commits and code is not merge-ready.

@carlocastoldi carlocastoldi force-pushed the feature/xdg-runtime-dir branch 4 times, most recently from f7a4466 to 4ab1b23 Compare October 9, 2023 21:34
@Hartmnt Hartmnt marked this pull request as draft October 9, 2023 21:44
@carlocastoldi
Copy link
Contributor Author

carlocastoldi commented Oct 9, 2023

Finally, since Windows doesn't rely on an actual "path" for sockets, i didn't even have to handle platform specific separators.

At this point, I'd like to request whether the code structure of this PR is roughly ok or not. Is the refactoring you asked when suggesting to write "a function returning a std::string" as you intended? Do you like the approach of having a C backend with a C++ frontend? Is the code separation ok?

Thank you in advance!

@Hartmnt
Copy link
Member

Hartmnt commented Jun 2, 2024

@carlocastoldi Hi, sorry that it took so long. Are you still willing to work on this?

@davidebeatrici @Krzmbrzl Would you be able to look at the latest changes/questions OP stated?

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Jun 2, 2024

This is one of the PRs that is still on my TODO list but for which I currently don't have the necessary time to look into.

@carlocastoldi
Copy link
Contributor Author

Hi, sorry that it took so long. Are you still willing to work on this?

Yups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software client overlay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't load the overlay over flatpak Steam
4 participants