regolith-linux / ilia

A GTK-based Desktop Executor
Apache License 2.0
83 stars 11 forks source link

fix: Escape unit names for systemd-run #93

Closed nicmongeau closed 1 month ago

nicmongeau commented 2 months ago

When calling systemd-run, ilia is currently using the application id as unit name and passing it directly on the command line. Some desktop shortcuts generated by programs such as Steam, will contain spaces in their application ids. This causes an issue, preventing the application to run at all. This could be fixed by enclosing the unit name within double quotes, but then you would still get a warning like this one: Invalid unit name "Cool Game Name" escaped as "Cool\x20Game\x20Name" (maybe you should use systemd-escape?).

It seems like the recommended way of dealing with this is to escape those unit names with the systemd-escape command line tool, which is what I attempted to do here.

This would mean that whenever an application is launched, systemd-escape would also be launched first. Alternatively, enclosing the unit name in double-quotes would probably work for pretty much all cases, and the warning is not seen by anyone, so it might be a better way. I proposed the solution with systemd-escape only because it is what is recommended.

Note that if systemd-escape fails to launch, this would fallback to using the application id as unit name directly, as before.

kgilmer commented 2 months ago

What a wonderful breakdown of the issue and description of the solution @nicmongeau , thank you. I'd like to reproduce this issue myself, is there some specific (freely available) app that exhibits the problem? (I have steam as well)

While I don't see anything incorrect in your PR, I hesistate to take this without exploring more options. In particular I wonder if we can avoid launching another process in the critical path of app launch.. Perhaps the escape functions in systemd-escape can be called directly, or a Vala implementation of the escape algorithm is available..

kgilmer commented 2 months ago

Also, FYI @SoumyaRanjanPatnaik

kgilmer commented 2 months ago

Possibly related: https://github.com/systemd/systemd/issues/14202

nicmongeau commented 2 months ago

What a wonderful breakdown of the issue and description of the solution @nicmongeau , thank you. I'd like to reproduce this issue myself, is there some specific (freely available) app that exhibits the problem? (I have steam as well)

It should be reproducible using any Steam game that has a space in its name, trying to launch it directly through Ilia instead of through the Steam application.

While I don't see anything incorrect in your PR, I hesistate to take this without exploring more options. In particular I wonder if we can avoid launching another process in the critical path of app launch.. Perhaps the escape functions in systemd-escape can be called directly, or a Vala implementation of the escape algorithm is available..

I agree. Also as I mentionned it might be enough to just add double-quotes around the unit name, which results in the aforementioned warning but does fix the issue, at least for whitespace characters...

kgilmer commented 2 months ago

Repro:

Sep 07 16:20:39 ana regolith-session-wayland[110920]: Text ended before matching quote was found for '. (The text was “systemd-run --user --scope --unit run_ilia_Don't Starve Together.desktop_a42a7209.scope steam steam://rungameid/322330 ”)
nicmongeau commented 2 months ago

Repro:

Sep 07 16:20:39 ana regolith-session-wayland[110920]: Text ended before matching quote was found for '. (The text was “systemd-run --user --scope --unit run_ilia_Don't Starve Together.desktop_a42a7209.scope steam steam://rungameid/322330 ”)

This one is also interesting as it gives a different error than just spaces, because the game name contains an actual single quote...

kgilmer commented 1 month ago

Closing in favor of https://github.com/regolith-linux/ilia/pull/94