Youda008 / DoomRunner

Preset-oriented graphical launcher of various ported Doom engines (an alternative to ZDL)
GNU General Public License v3.0
205 stars 13 forks source link

Experimental support for sandbox environments (Flatpak and Snap) #109

Closed Youda008 closed 12 months ago

mbugni commented 1 year ago

I added the CONFIG+=flatpak option, please notice what the current flatpak-build command runs:

FB: Running: flatpak build --die-with-parent --env=FLATPAK_BUILDER_BUILDDIR=/run/build/doomrunner-build --nofilesystem=host:reset --filesystem=/live/github/flatpak/DoomRunnerFlatpak/.flatpak-builder/build/doomrunner-build-2 --bind-mount=/run/build/doomrunner-build=/live/github/flatpak/DoomRunnerFlatpak/.flatpak-builder/build/doomrunner-build-2 --build-dir=/run/build/doomrunner-build/_flatpak_build --bind-mount=/run/ccache=/live/github/flatpak/DoomRunnerFlatpak/.flatpak-builder/ccache --env=SOURCE_DATE_EPOCH=1688057705 '--env=CFLAGS=-O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer ' '--env=CXXFLAGS=-O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer ' '--env=LDFLAGS=-L/app/lib -Wl,-z,relro,-z,now -Wl,--as-needed ' --env=CCACHE_DIR=/run/ccache/disabled --env=PATH=/app/bin:/usr/bin --env=LD_LIBRARY_PATH=/app/lib --env=PKG_CONFIG_PATH=/app/lib/pkgconfig:/app/share/pkgconfig:/usr/lib/pkgconfig:/usr/share/pkgconfig --env=FLATPAK_BUILDER_N_JOBS=8 /live/github/flatpak/DoomRunnerFlatpak/.flatpak-builder/rofiles/rofiles-kp8Qkt qmake 'PREFIX='\''/app'\''' ../DoomRunner.pro 'CONFIG+=flatpak'

The qmake 'PREFIX='\''/app'\''' ../DoomRunner.pro 'CONFIG+=flatpak' should be the relevant part.

Right now I'm getting an error:

../Sources/MainWindow.cpp:3242:43: error: ‘engine’ was not declared in this scope; did you mean ‘Engine’?
 3242 |                 if (getAbsoluteDirOfFile( engine.path ) != QApplication::applicationDirPath())
      |                                           ^~~~~~
      |                                           Engine
Youda008 commented 1 year ago

Sorry, will fix soon.

mbugni commented 1 year ago

Basically it works.

I changed the build:

If the host engine is in $HOME/bin/my_path/my_engine, the command line is:

flatpak-spawn --host my_engine [OTHER PARAMS]

It doesn't work because it's not an absolute path, and the binary is not in one of $PATH folders.

I notice a strange behaviour about engine config folder. It gets the same value of engine path, and it becomes red because the folder does not exist.

Cosmetic fixes

The "Intial setup" should be renamed as "Settings" (if a default config is provided, there is no initial setup).

Please take a look at https://github.com/Youda008/DoomRunner/issues/108 for window icon. Current icons are in:

I can inject more icons if you need.

Youda008 commented 1 year ago

I will take a look, but I'm leaving for vacation in few hours, so it will have to wait.

The "Intial setup" should be renamed as "Settings" (if a default config is provided, there is no initial setup).

I'm against this. There is intentionally no default config, because the settings dialog is supposed to automatically open on first run, as most of the things entered there need to be entered only once.

mbugni commented 1 year ago

Another issue, related to an app engine. I'm using Crispy Doom from Flathub [1]. It should be able to save settings in its own app data folder. Anyway, even if I leave the "Config directory" field empty, the command is:

flatpak-spawn --host "/home/myuser/bin/crispydoom-run.sh" -iwad "/home/myuser/doom/freedoom1.wad" -savedir "/app/bin/./CrispyDoom"

The engine works, but the final part -savedir "/app/bin/./CrispyDoom is preventing the app save data, because it cannot write. Maybe a better approach is do not add the -savedir parameter when the config dir is empty, leaving the engine to use its default.

There is intentionally no default config, because the settings dialog is supposed to automatically open on first run, as most of the things entered there need to be entered only once.

I mean the menu item, not the initial window title. If you access it from the menu, is no more "initial".

PS: have a nice vacation.

[1] https://flathub.org/it/apps/io.github.fabiangreffrath.Doom

Youda008 commented 1 year ago

I notice a strange behaviour about engine config folder. It gets the same value of engine path, and it becomes red because the folder does not exist.

Hmm, i have difficulties reproducing this. I copied my installation of gzdoom from /opt to ~/bin, pointed DoomRunner to that path and got config dir /home/youda/.config/gzdoom.

I use this code to generate the path of the config directory of an engine. The QStandardPaths::writableLocation( QStandardPaths::GenericConfigLocation ) usually returns ~/.config in Linux, it is even in the official examples here. The only way how this could lead to the executable dir is if you installed the executable into a directory that is considered to be a config dir.

I suspect that this is caused by interference of some environment variables that are used under the hood. Can you please dump me relevant environment variables under this setup?

mbugni commented 1 year ago

I use this code to generate the path of the config directory of an engine. The QStandardPaths::writableLocation( QStandardPaths::GenericConfigLocation ) usually returns ~/.config in Linux

I'm trying to add a new launcher script: new-engine-config 1) I receive a warn because the folder doesn't exist. So I have to create it manually. The path seems ok, I don't know if QStandardPaths::AppConfigLocation can do a better job. Honestly I don't know if this is the default behaviour. 2) The bad is if you want to use such kind of folder for another app: sandbox folders are denied, because are private space. CrispyDoom cannot access to DoomRunner files, and viceversa. So I left it blank. But the result is -savedir "/app/bin/./CrispyDoom", which causes errors because /app/bin/ is read-only.

To summarize: if the Config directory field remains blank, then the -savedir option should not be forced. The engine should fallback to defaults.

Flatpak environment honor the XDG desktop specification [1]. This is the XDG environment:

[📦 io.github.Youda008.DoomRunner ~]$ env | grep -i xdg
XDG_CONFIG_DIRS=/app/etc/xdg:/etc/xdg
XDG_SESSION_PATH=/org/freedesktop/DisplayManager/Session3
XDG_MENU_PREFIX=kf5-
XDG_DATA_HOME=/home/massi/.var/app/io.github.Youda008.DoomRunner/data
XDG_CONFIG_HOME=/home/massi/.var/app/io.github.Youda008.DoomRunner/config
XDG_SEAT=seat0
XDG_SESSION_DESKTOP=KDE
XDG_SESSION_TYPE=x11
XDG_CURRENT_DESKTOP=KDE
XDG_SEAT_PATH=/org/freedesktop/DisplayManager/Seat0
XDG_CACHE_HOME=/home/massi/.var/app/io.github.Youda008.DoomRunner/cache
XDG_SESSION_CLASS=user
XDG_VTNR=2
XDG_SESSION_ID=6
XDG_STATE_HOME=/home/massi/.var/app/io.github.Youda008.DoomRunner/.local/state
XDG_RUNTIME_DIR=/run/user/1000
XDG_DATA_DIRS=/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share

Also notice:

[📦 io.github.Youda008.DoomRunner ~]$ env | grep -i flat
FLATPAK_ID=io.github.Youda008.DoomRunner
XAUTHORITY=/run/flatpak/Xauthority
container=flatpak
AT_SPI_BUS_ADDRESS=unix:path=/run/flatpak/at-spi-bus
PULSE_CLIENTCONFIG=/run/flatpak/pulse/config
FLATPAK_SANDBOX_DIR=/home/massi/.var/app/io.github.Youda008.DoomRunner/sandbox
PS1=[📦 $FLATPAK_ID \W]\$ 
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/flatpak/bus
PULSE_SERVER=unix:/run/flatpak/pulse/native

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Youda008 commented 12 months ago

Ok, now i see where the problem is comming from. Thanks.

But please note, that empty "Config directory" is invalid configuration. Without the config dir, some features cannot work properly. The launcher tries to guess where it could be, but that guess can be wrong and the user is responsible for entering the correct directory. Of course i can make it little bit more bullet-proof so that with empty or invalid config dir the launcher does not produce invalid or weird command, but the features that depend on config dir will still not work.

To summarize: if the Config directory field remains blank, then the -savedir option should not be forced. The engine should fallback to defaults.

The -savedir is never forced. This argument comes from the "Save dir" field on the second tab, if you clear the text, the parameter will disappear. Using the preset name as target directory is one of the features that require config dir to be set, because the config dir is always the base dir for all the files the engine works with.

mbugni commented 12 months ago

I think we are almost there. It works with other app engines. I added a mini-guide to the README file [1], please take a look.

[1] https://github.com/mbugni/DoomRunnerFlatpak#readme

Youda008 commented 12 months ago

Awesome 🙂 But imho those Advanced Usage steps shouldn't be needed anymore. The launcher will now automatically add flatpak run when you select engine that is inside Flatpak directory, and it will automatically add --filesystem= for every dir it needs to access.

Youda008 commented 12 months ago

By the way, do you really need this line in my project file? I noticed you have own install rules here, so you should be able to install the executable where-ever you want, as you already do with the icon. I would prefer if the installation directory of all files (executable, icon, .desktop file) was defined together in 1 repository and not be split between 2.

mbugni commented 12 months ago

The launcher will now automatically add flatpak run when you select engine that is inside Flatpak directory, and it will automatically add --filesystem= for every dir it needs to access

Let's be careful about this. In my experience it's hard to share something among apps, except if it placed in a common space (and this space doesn't belong to any sandbox). For example: the ~/download folder. Did you try this approach? When I tryed last time I couldn't access to app engine executable from Doom Runner. From an end-user perspective, except for managing app engines, the behaviour of Doom Runner app it's right now very close to the native one.

I noticed you have own install rules here, so you should be able to install the executable where-ever you want, as you already do with the icon.

Probably you don't need that because, as I mentioned, basically the Flatpak build run this:

qmake 'PREFIX='\''/app'\''' ../DoomRunner.pro

Remember that inside the sandbox the filesystem has predefined layout and permissions [1]. App files should stay under /app root.

I would prefer if the installation directory of all files (executable, icon, .desktop file) was defined together in 1 repository and not be split between 2.

If you want to provide desktop file and icons, we can also move it at build time in the right place with the right name (the app identifier), please take a look at [2].

[1] https://docs.flatpak.org/en/latest/sandbox-permissions.html [2] https://docs.flatpak.org/en/latest/freedesktop-quick-reference.html

mbugni commented 12 months ago

Just to clarify, I tested some commands to launch the CrispyDoom app.

The following commands work from the Linux host:

$ flatpak run io.github.fabiangreffrath.Doom -version
Crispy Doom 6.0.0

$ /var/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom -version
Crispy Doom 6.0.0

$ flatpak run --filesystem="$HOME/doom" io.github.fabiangreffrath.Doom -iwad "$HOME/doom/data/freedoom1.wad" -savedir "$HOME/doom/config/CrispyDoom"
                           Crispy Doom 6.0.0
Z_Init: Init zone memory allocation daemon.
...

The following commands don't work from the Flatpak sandbox:

[📦 io.github.Youda008.DoomRunner ~]$ /var/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom -version
bash: /var/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom: No such file or directory

[📦 io.github.Youda008.DoomRunner ~]$ /var/run/host/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom -version
bash: /var/run/host/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom: No such file or directory

The following commands work from the Flatpak sandbox:

[📦 io.github.Youda008.DoomRunner ~]$ flatpak-spawn --host flatpak run io.github.fabiangreffrath.Doom -version
Crispy Doom 6.0.0

[📦 io.github.Youda008.DoomRunner ~]$ flatpak-spawn --host /var/lib/flatpak/exports/bin/io.github.fabiangreffrath.Doom -version
Crispy Doom 6.0.0

[📦 io.github.Youda008.DoomRunner ~]$ flatpak-spawn --host flatpak run --filesystem="$HOME/doom" io.github.fabiangreffrath.Doom -iwad "$HOME/doom/data/freedoom1.wad" -savedir "$HOME/doom/config/CrispyDoom"
                           Crispy Doom 6.0.0
Z_Init: Init zone memory allocation daemon.
...

So, I suppose you are approaching one of the last command, isn't it?

Youda008 commented 12 months ago

Let's be careful about this. In my experience it's hard to share something among apps, except if it placed in a common space (and this space doesn't belong to any sandbox). For example: the ~/download folder. Did you try this approach? When I tryed last time I couldn't access to app engine executable from Doom Runner. From an end-user perspective, except for managing app engines, the behaviour of Doom Runner app it's right now very close to the native one.

Ok, fair enough. Let's keep it as is. At least the user will have more options.

Probably you don't need that because, as I mentioned, basically the Flatpak build run this:

qmake 'PREFIX='\''/app'\''' ../DoomRunner.pro

Remember that inside the sandbox the filesystem has predefined layout and permissions [1]. App files should stay under /app root.

If you want to provide desktop file and icons, we can also move it at build time in the right place with the right name (the app identifier).

My though process is the following: Currently we have your project and AUR package, and it is possible that in the future there will be more packages (someone might create Snap package, someone might create native distro package, ...). Each of those packages will need their own specific install directory and .desktop file (which points to installed icon). It seems illogical to have those things defined in my repository, because i would have to define it for every packages people make. It seems better if it's defined by the packages right away. So if you don't mind, i will remove the package-specific install rules from my project file.

Youda008 commented 12 months ago

Alright. I will merge this now, so that i don't have to solve conflicts with master. I will tune the details or fix bugs in the master branch.

mbugni commented 12 months ago

Each of those packages will need their own specific install directory and .desktop file (which points to installed icon). It seems illogical to have those things defined in my repository, because i would have to define it for every packages people make.

Regarding desktop files: I left a comment in [1]. Basically, you are the author and maybe you want to control name, description, icons etc. You don't need to install them, maintainers can do it.

So if you don't mind, i will remove the package-specific install rules from my project file.

It doesn't work:

/usr/bin/qmake -install qinstall -exe DoomRunner /usr/bin/DoomRunner
Error copying DoomRunner to /usr/bin/DoomRunner: Cannot create /usr/bin/DoomRunner for output
make: *** [Makefile:1620: install_target] Error 3

In the Makefile I see this:

QINSTALL      = /usr/bin/qmake-qt5 -install qinstall
QINSTALL_PROGRAM = /usr/bin/qmake-qt5 -install qinstall -exe

As far I understand it's trying to write in /usr/bin which is protected in the sandbox. Is there a way to override it (without a patch)? Is there any chance to use the PREFIX parameter?

[1] https://github.com/Youda008/DoomRunner/pull/65

mbugni commented 12 months ago

/usr/bin/qmake -install qinstall -exe DoomRunner /usr/bin/DoomRunner Error copying DoomRunner to /usr/bin/DoomRunner: Cannot create /usr/bin/DoomRunner for output make: *** [Makefile:1620: install_target] Error 3

I found a workaround for this:

- name: doomrunner-build
    buildsystem: qmake
    builddir: true
    make-opts:
      - CONFIG+=release
    make-install-args:
      - INSTALL_ROOT+=/app/tmp
    sources:
      - type: git
        url: https://github.com/Youda008/DoomRunner
        branch: master
    post-install:
      - mv /app/tmp/usr/bin/DoomRunner /app/bin/DoomRunner
      - rm /app/tmp -rf

It's a bit dirty, but I can survive. But I'm facing another problem, it seems that the launcher doesn't prefix the command with flatpak-spawn --host, isn't it?

Youda008 commented 12 months ago

Try qmake DESTDIR=. If that doesn't work, i will add another project variable that can be set on command line.

mbugni commented 12 months ago

But I'm facing another problem, it seems that the launcher doesn't prefix the command with flatpak-spawn --host, isn't it?

Nevermind, my fault. I missed the CONFIG+=flatpak option during some test. :-|

Try qmake DESTDIR=. If that doesn't work, i will add another project variable that can be set on command line.

It seems DESTDIR affects the destination dir for executable (build fase). The problem is on install phase. It's just aesthetic: I can get the result.

Any plan for a release version? I think it's enough, we could start the app submission.

mbugni commented 12 months ago

Try qmake DESTDIR=. If that doesn't work, i will add another project variable that can be set on command line.

Probably INSTALL_ROOT is the right variable to make the path configurable. From Makefile:

####### Install

install_target: first FORCE
    @test -d $(INSTALL_ROOT)/usr/bin || mkdir -p $(INSTALL_ROOT)/usr/bin
    $(QINSTALL_PROGRAM) $(QMAKE_TARGET) $(INSTALL_ROOT)/usr/bin/$(QMAKE_TARGET)
    -$(STRIP) $(INSTALL_ROOT)/usr/bin/$(QMAKE_TARGET)

uninstall_target: FORCE
    -$(DEL_FILE) $(INSTALL_ROOT)/usr/bin/$(QMAKE_TARGET)
    -$(DEL_DIR) $(INSTALL_ROOT)/usr/bin/
Youda008 commented 12 months ago

I've added an option to specify install directory, you can use it like qmake "INSTALL_DIR=/opt/whatever". If not specified, /usr/bin will be used.

Before release I need to sort out few more issues, but with luck it could be next weekend. I will first release 1.7.3 with bug-fixes only and then 1.8.0 with the new features and Flatpak support.

Youda008 commented 12 months ago

Also please note that i changed the location of DoomRunner's data files (options.json) from application config dir to application data dir: that is from ~/.config to ~/.local/share on Linux and from %UserProfile%\AppData\Local to %UserProfile%\AppData\Roaming on Windows, because i believe those are the more appropriate locations for it.

mbugni commented 11 months ago

I've added an option to specify install directory, you can use it like qmake "INSTALL_DIR=/opt/whatever". If not specified, /usr/bin will be used.

It works.

I will first release 1.7.3 with bug-fixes only and then 1.8.0 with the new features and Flatpak support.

OK, it's not for stressing, just to know.

Also please note that i changed the location of DoomRunner's data files (options.json) from application config dir to application data dir

I adjusted the app accordingly, the only drawback is that now the DoomRunner folder appear in the map pack area, because data and configs are mixed now.

that is from ~/.config to ~/.local/share on Linux

The ~/.local/share should be a "data folder", for something like "recently files opened" or local/temporary/session storage. IMO ~/.config was better, but it's ok. I'm thinking to a different layout for the app.

Youda008 commented 11 months ago

Well, options.json is not really a config file. I imagine a config file as some text file that the user is encouraged to open with notepad and manually edit the settings to modify the program behaviour, like .ssh/config or simmilar. The options.json is more like a dump of the application's internal state, and it is only my design decision and implementation detail that it is a readable text file (JSON), i was also considering a binary format. That's why i think it is more of a data file.

mbugni commented 11 months ago

that is from ~/.config to ~/.local/share on Linux

I moved the app data dir to $XDG_DATA_HOME/share to be closer to default settings, it works fine.

pizzadude commented 10 months ago

@Youda008 @mbugni The flatpak doesn't work when using any engine with flatpak-spawn, for me. It always gives an error like this:

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

image

pizzadude commented 10 months ago

Hmm, it seems the popup always gives a path like /run/user/doc/__ , but flatpak-spawn doesn't like /run/user/doc/__ .

So you have to type the "real" path manually, because the "browse" button always gives the "portal path" (/run/user/doc____)

Also, you have to give the flatpak access to /var/lib/flatpak/app:ro in flatseal, otherwise it can't find other flatpak engines, like crispy doom.

And it still doesn't work, because it fails to make the config directory.

mbugni commented 10 months ago

@pizzadude where is located the engine you are trying to use? Please, take a look at Sandbox Permissions for Flatpak limitations. Also take a look at this README.

mbugni commented 10 months ago

@Youda008 we are online: https://flathub.org/apps/io.github.Youda008.DoomRunner

pizzadude commented 10 months ago

@pizzadude where is located the engine you are trying to use? Please, take a look at Sandbox Permissions for Flatpak limitations. Also take a look at this README.

Okay, thanks to your readme I got it working. I was confused at first because DoomRunner itself gives different instructions on how to add flatpak engines, if you hover over the "help" icon. IMO you should put a link to the flatpak specific readme in the app's flathub description.

mbugni commented 10 months ago

@pizzadude you are right, maybe help it's outdated. Sorry I didn't notice it. The same approach (creating local scripts) should work for both native and sandboxed engines. I mainly tested it with sandboxed engines. @Youda008 I suggest to simply link to the Flathub README, I already opened a PR for adding Flathub links.