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

flatpak: packaging proposal #178

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

flatpak: packaging proposal #178

wants to merge 2 commits into from

Conversation

imLinguin
Copy link
Member

Why?

Current flatpak manifest we have, ships just a CLI, it doesn't provide any way for other flatpak applications to use umu.

How?

We'd simplify getting started with umu for packagers, by simply providing a module file that can be easily included in the manifest.
Packagers would need to provide a simple set of permissions required for pressure-vessel to work.

finish-args:
  - --allow=per-app-dev-shm
  # Wine uses UDisks2 to enumerate disk drives
  - --system-talk-name=org.freedesktop.UDisks2
  # Required for bwrap to work
  - --talk-name=org.freedesktop.portal.Background
  # Pressure Vessel
  # See https://github.com/flathub/com.valvesoftware.Steam/commit/0538256facdb0837c33232bc65a9195a8a5bc750
  - --env=XDG_DATA_DIRS=/app/share:/usr/lib/extensions/vulkan/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share:/usr/lib/pressure-vessel/overrides/share

With that all the application needs to do is look for umu-run in PATH. (Assuming proper PATH variable was setup in the manifest)

Disk space problem

UMU would require access to xdg-data/umu .

finish-args:
  - --filesystem=xdg-data/umu

That way, we ensure this one location is used by flatpak and not sandboxed applications. Instead of having separate copies of runtime, only one would be maintained.

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Sep 6, 2024

This change makes sense. Saving space when we can is good and we already do that for Protons by essentially using the ones available in $HOST_XDG_DATA_HOME/.local/share/Steam/compatibilitytools.d. If we decide to do this, I think it would be worth logging a warning or mention in a change log about the umu Flatpak runtime directory being obsolete so users can remove it.

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Sep 6, 2024

Also, for context, I initially wrote it so that Flatpaks would have their own runtime directory to avoid the case of the Flatpak and system umu overwriting each others changes due to the differences in their APIs. I also thought it would align more with the philosophy of Flatpak to have its own private copy.

@imLinguin
Copy link
Member Author

I also thought it would align more with the philosophy of Flatpak to have its own private copy.

That would make sense if we wanted each app using umu to have its own copy. The proposed approach assumes only umu-run gets shipped by the app and runtime files are shared.

due to the differences in their APIs

could you give an example of such differences? Are you saying there may be implications?

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Sep 6, 2024

could you give an example of such differences? Are you saying there may be implications?

Yeah, for example, Lutris is on RC5 and in that version the launcher checks if the reaper binary exists otherwise it will crash if it can't find it in umu or the clients runtime directory. However, this isn't a concern as it wouldn't lead to any crashes if the client's runtime directory is updated. But one case where it will be a concern is if logic is added that effects the contents within the umu directory because of umu-launcher's auto update mechanism and its expectations for certain contents to be within $HOME/.local/share/umu.

So like, let's suppose we decide to build and customize the Steam runtime. To that end, we decide to create subdirectories instead of having everything within the top level to better organize various runtime builds like the Steam client does within steamapps/common. So we have something like steamrt3 for Valve's, umu-steamrt3 for Open Wine Component's, etc.. In such an implementation, we must never add logic to remove old files within the top level. Otherwise, the worst case scenario is that two or more clients will overwrite each other's changes and the runtime always being downloaded due to one being at the latest while the other is behind.

For instance, let's suppose the user has both Heroic (client 1) and Lutris (client 2) installed each with two different versions of umu-launcher. The user launches client 1 to a play a game, which includes the latest version of umu-launcher and contains logic to cleanup all files within the top level and redownloads a customized runtime within umu-steamrt3.
ls $HOME/.local/share/umu:

umu-steamrt3

Later, the user launchers client 2 to play another game where that umu-launcher version will redownload the runtime because it couldn't find any runtime files in the top level leading to:
ls $HOME/.local/share/umu:

compatibilitytools.d.lock  README.md      sniper_platform_0.20240820.99315  umu           var
mtree.txt.gz               run            steampipe                         umu.lock      VERSIONS.txt
pressure-vessel            run-in-sniper  toolmanifest.vdf                  umu-steamrt3

Afterwards, the user launches client 1 again to play another game which leads back to:
ls $HOME/.local/share/umu:

umu-steamrt3

TL;DR: There needs to be care when making changes to the contents of $HOME/.local/share/umu, in particular when removing files due to differing umu-launcher versions across clients.

@imLinguin
Copy link
Member Author

For cases like that we could introduce some sort of versioning requiring minimal version of umu.
If not I guess we can just make apps download and update zipapp archive as a fallback. Though that still opens a possibility that given implementation neglects the update and the same issue occurs

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Sep 8, 2024

To be clear the concern I brought up shouldn't block this from being merged since it's packaging related and it can be avoided if umu-launcher just never cleans up obsolete files in umu. Instead, we need to just log warnings like we currently do and let the user know that they need to delete them. For example:

Version: umu-launcher v0.1.0+g2d3c948
Build ID: 18c8bfd82e159c06f7e48382e84fc35d480fe6d1b2abf657f3309ed28ed41e8d
Will install steamrt in '/home/foo/.local/share/umu/steamrt3'
'/home/foo/.local/share/umu/pressure-vessel' is obsolete in umu-launcher v0.1.0+g2d3c948

The above log statements can be more clearer though.

Though we can control that umu-launcher never cleans up obsolete files, it doesn't guarantee that the client won't. By sharing the runtime directory, we're trusting that all clients will behave responsibly. So what can't be helped is if a client decides to delete files in $HOME/.local/share/umu to clean things up for the user, thus making umu look pretty bad as it would trigger the automatic runtime restoration codepath in another client. Having each application source its own copy of the runtime is one way to mitigate this.

Another way is for steam-runtime-tools (pressure-vessel directory) and the runtime platform (e.g., sniper_platform_0.20240820.99315) installed in a system path then changing the location of the variable directory via the --variable-dir argument from pressure-vessel-wrap. Some other environment variables like $PRESSURE_VESSEL_RUNTIME_BASE path should need to change too -- see run-in-sniper for details. Though if we do that, Flatpaks will each have it's own private copy in the system path unless we figure out how it can be shared properly in Flatpak's system. As a result, the only thing that will be written in $HOME is var and a Proton directory. In effect, this would also allow removing umu-launcher's runtime restoration logic.

So I guess my question is whether we should continue with the current system and merge this PR by sharing runtime files in a single, shared path at $HOME or to consider changing it to avoid the described risks?

cc @loathingKernel for thoughts

@imLinguin
Copy link
Member Author

imLinguin commented Sep 17, 2024

Today I found that escaping sandbox like this may cause some issues, first of all xdg-data keywords creates a mapping that binds path from within the container to the host path. This causes issues with bwrap that gets confused about different paths when it compares the file descriptor. I tried to workaround it by using HOST path directly. However that made flatpak umu retry the download from time to time, even when not needed, I'll test it some more, but..
Any other ideas are welcome.

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Sep 20, 2024

I'm not too sure this is possible actually. Each Flatpak has their own private home directory, so wouldn't - --filesystem=xdg-data/umu be the running Flatpak's (e.g., HGL, Lutris, etc.) private umu directory?

Also, I just realized the umu-launcher manifest has set - --filesystem=home, which should give access to the entire home directory anyway. This change was made at cd02355

I tried to workaround it by using HOST path directly. However that made flatpak umu retry the download from time to time, even when not needed, I'll test it some more, but..
Any other ideas are welcome.

I think this is unrelated and should be already addressed in main.

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

Successfully merging this pull request may close these issues.

2 participants