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

Fix running of GUI apps from toolboxes #564

Closed
wants to merge 3 commits into from

Conversation

@HarryMichal
Copy link
Collaborator

@HarryMichal HarryMichal commented Sep 22, 2020

GNOME Shell 3.38 stopped using an abstract unix socket for communication with X for XWayland. This change causes GUI apps fail to start when ran from a toolbox. To fix this, the unix socket available in a container under /run/host/tmp needs to be available under /tmp.

Fixes #562

PS: I'm marking this as WIP until the final solution is found. This seems to work but I'm wondering if we could simply mount /run/host/tmp to /tmp as a whole instead of cherry-picking what goes in or not.

PPS: I'm not entirely sure which of the symlinked paths are really necessary. I'd appreciate any insight on this because my knowledge of X is very limited.

PPPS: This also relies on some other patches to work/look properly so those will probably need to be merged first.

HarryMichal added 3 commits Sep 9, 2020
The redirectPath() function does not work correctly if containerPath
does not exist (like /media). The error message often looks like this:

Error: failed to redirect /media to /run/media: remove /media: no such file or directory

This makes redirectPath try to remove containerPath only if it exists.
The existence of containerPath is done using utils.PathExist().

#556
There might be cases when a "symlink/mount candidate" need to be created
and for such occasions it's better to have a type than having to define
the struct again and again.

I'm also thinking of connecting the redirectPath() and mountBind()
functions to the types. That is currently just an idea, though.
Toolbox until now relied on the existence of an abstract unix socket
(more in unix(7)) for the communication with X server. In version 3.38
GNOME Shell stopped using this socket and instead uses the "standard"
approach of using the one found in /tmp. And since Toolbox mounts a
tmpfs to /tmp instead of the hosts, /tmp in a toolbox is empty.

With this the crucial paths /run/host/tmp are symlinked to /tmp,
enabling (again) running of GUI applications from a toolbox.
@softwarefactory-project-zuul
Copy link

@softwarefactory-project-zuul softwarefactory-project-zuul bot commented Sep 22, 2020

Build failed.

@owtaylor
Copy link
Contributor

@owtaylor owtaylor commented Sep 30, 2020

Background:

https://gitlab.gnome.org/GNOME/mutter/-/issues/1289
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1424

Basically, when mutter was made to auto-start Xwayland, it only autostarted based on the abstract socket, which broke Flatpaks - so the fix was to make it only listen/autostart on the X11 socket - not entirely sure why it couldn't do both.

I'm actually in favor of the 'bind-mount /tmp' approach (and /var/tmp') - I fairly frequently have to work around /tmp being different on the host and toolbox, and can't think of any immediate downsides.

If we want to go with the mount-by-mount approach, I'd recommend only binding in .X11-unix - .font-unix .ICE-unix and .XIM-unix are some very obsolete stuff. As I recall:

.font-unix - font server - this was to share bitmap fonts between different X servers (X terminals, probably) on a network, I think
.ICE-unix - X session protocol - support in GNOME and otherwise slowly died between 15-10 years ago
.XIM-unux - X Input Method protocol. Been replaced by multiple generations of input method framework since

Though there shouldn't be any harm in binding in the other ones.

Copy link
Member

@debarshiray debarshiray left a comment

Thanks for the patch, @HarryMichal !

{"/etc/machine-id", "/run/host/etc/machine-id", "ro"},
{"/run/libvirt", "/run/host/run/libvirt", ""},
{"/run/systemd/journal", "/run/host/run/systemd/journal", ""},
{"/var/lib/flatpak", "/run/host/var/lib/flatpak", "ro"},
{"/var/log/journal", "/run/host/var/log/journal", "ro"},
{"/var/mnt", "/run/host/var/mnt", "rslave"},
}

initContainerX11Symlinks = []initContainerSymlink{

This comment has been minimized.

@debarshiray

debarshiray Oct 1, 2020
Member

It would be better to do it as a bind mount instead. Or is there a reason why they must be symbolic links? Did I miss something?

For individual regular files we prefer symbolic links instead of bind mounts, because symbolic links can track updates to the original file. Whereas, with bound mounts, if the original file was updated atomically, which is what most programs do, then the atomic rename changes the inode, and the two diverge.

However, symbolic links are always weird and introduce various oddities (eg., stat versus lstat), so bind mount is better when possible.

@debarshiray
Copy link
Member

@debarshiray debarshiray commented Oct 1, 2020

On second thoughts, as @owtaylor suggested, let's go with bind mounting the entire /tmp. Not only does it avoid having to keep a list of aging locations for X11 support, but it also nice in the sense that you can put throwaway files in /tmp and have them shared.

I pushed #570 while discussing this with Owen in #gnome-hackers on GIMPNet.

However, I still like the idea of iterating over a list to create all the symbolic links. We can keep this PR open to pursue that idea.

@owtaylor
Copy link
Contributor

@owtaylor owtaylor commented Oct 1, 2020

On second thoughts, as @owtaylor suggested, let's go with bind mounting the entire /tmp

To be clear, this was @HarryMichal 's idea

@debarshiray
Copy link
Member

@debarshiray debarshiray commented Oct 1, 2020

On second thoughts, as @owtaylor suggested, let's go with bind mounting the entire /tmp

To be clear, this was @HarryMichal 's idea

Oops! Sorry about that.

@HarryMichal
Copy link
Collaborator Author

@HarryMichal HarryMichal commented Oct 1, 2020

On second thoughts, as @owtaylor suggested, let's go with bind mounting the entire /tmp. Not only does it avoid having to keep a list of aging locations for X11 support, but it also nice in the sense that you can put throwaway files in /tmp and have them shared.

I pushed #570 while discussing this with Owen in #gnome-hackers on GIMPNet.

However, I still like the idea of iterating over a list to create all the symbolic links. We can keep this PR open to pursue that idea.

Sounds good to me. I'll try to extend this when I have time to do so.

@HarryMichal
Copy link
Collaborator Author

@HarryMichal HarryMichal commented Feb 18, 2021

I'm closing this because #570 was merged. Let's pursue the idea with list for symbolic links in a different PR.

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

Successfully merging this pull request may close these issues.

3 participants