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: native runners detection in flatpak #53

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Conversation

imLinguin
Copy link
Member

this should fix issues in flatpak where permissions to detect native runners aren't available

@nirvdrum
Copy link

I started working on a fix for this before realizing you had a PR open. GOG games using DOSBox won't currently launch on Heroic 2.15.1 (latest release) if dosbox-staging is installed as a Flatpak. A mitigation in the meanwhile is to uninstall dosbox-staging, but that's an unobvious solution. It'd be great if we could get this merged and released.

if spawn_test.returncode == 0:
new_process_command = ["flatpak-spawn", "--host"]
process_command = new_process_command + process_command
try:

Choose a reason for hiding this comment

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

Suggested change
try:
# It's possible the current working directory is path from which we are unable to execute `flatpak-spawn`.
# To ensure the command succeeds, switch to a known good path.
os.chdir("/tmp")
try:

Choose a reason for hiding this comment

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

After fixing the missing , in the "--host", "ls" argument list, I found I still had problems invoking flatpak-spawn. I logged stderr and what I got was:

Portal call failed: Failed to start command: Failed to change to directory "/run/flatpak/doc/c0447613" (No such file or directory)

The directory definitely exists within the sandbox. I'm unsure if there's a permissions issue preventing execution of arbitrary commands or if the --host option to flatpak-spawn requires a mapping that isn't permitted by the sandbox. I'm still fairly new to the Flatpak execution model. But, I found when I changed to /tmp, things worked fine. The path used is arbitrary, so feel free to change to something else if you'd like.

I'm not sure if you added FileNotFoundError handling for the same reason. If so, I think your handler will end up skipping the native dosbox-staging. The error handler will skip execution of the native dosbox-staging, so I think it's better if we can avoid hitting it when possible.

I did the chdir at this location so it would only be executed if we know we're running in an environment where flatpak-spawn should be used. We can move that, too, if needed -- I was trying to be conservative. I'm doing all of my testing on a sandoboxed Steam Deck with a custom built gogdl.

Copy link

Choose a reason for hiding this comment

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

I've confirmed that running the code in commit 77ecc22 fixes the crash but does not use the native dosbox-staging. In order to use dosbox-staging on my Steam Deck (i.e., with flatpak-spawn), the os.chdir line I suggested is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made a change. Though instead of static chdir to /tmp its done to game directory. That way if command still fails it suggests permission issue that would prevent the game from running nevertheless

Choose a reason for hiding this comment

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

Oops. I guess I didn't post my comment. I had pulled your changes and built it again and it was all working. Thank you for the fix.

@imLinguin imLinguin merged commit 151f05f into main Sep 10, 2024
4 checks passed
@imLinguin imLinguin deleted the fix/native-runners branch September 10, 2024 15:21
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.

3 participants