Pidgeot / python-lnp

Cross-platform re-implementation of the Lazy Newb Pack launcher.
ISC License
64 stars 10 forks source link

14a returns an error and closes on Arch Linux #179

Closed ViceroyFaust closed 1 year ago

ViceroyFaust commented 2 years ago

This is my first time using PyLNP. I have dwarf fortress and dfhack installed via my package manager. I get this error when I try to launch PyLNP.

ERROR: Could not find any Dwarf Fortress installations.
['Traceback (most recent call last):\n', '  File "python_lnp/launch.py", line 12, in <module>\n', '  File "python_lnp/core/lnp.py", line 113, in __init__\n', '  File "python_lnp/core/lnp.py", line 132, in initialize_program\n', '  File "python_lnp/core/lnp.py", line 309, in detect_basedir\n', 'SystemExit: 2\n']

I am not exactly sure what I can do here. I tried placing the executable in the ~/.dwarffortress folder, but that did not help.

Pidgeot commented 2 years ago

PyLNP expects a very specific folder layout, which your package manager presumably isn't setting things up for: specifically, it needs to be in a folder containing another folder with the Dwarf Fortress executable, or it needs to be able to navigate up from its own location to find such a folder.

Generally speaking, the preferred way to use it is to download one of the starter packs which includes both PyLNP and Dwarf Fortress, since that's already laid out correctly - this should be the current one for Linux, which I assume you're using: http://www.bay12forums.com/smf/index.php?topic=157712

If you want to use your package manager's version of Dwarf Fortress, you'll need to create the folders yourself and lay it all out. It should look something like this:

/some/folder
  Dwarf Fortress/
    df <executable>
    <other files and folders in here, like data/>
  LNP/
    <LNP files>
  PyLNP <exectuable>

The PyLNP executable itself may be in a subfolder within this hierarchy, but the Dwarf Fortress and LNP folders have to be in the same parent folder. If you do go down this route, you may still want to use the Starter Pack I mentioned as a reference for laying it all out.

Perlovka commented 1 year ago

but the Dwarf Fortress and LNP folders have to be in the same parent folder I'm sorry, but this is not true.

I have pylnp installed in system:

~ $ which pylnp
/usr/bin/pylnp

pylnp user files are installed in ~/.config/pylnp:

$ ls -1 ~/.config/pylnp/
LNP
PyLNP.user
stderr.txt

and dwarf-fortress is in ~/.dwarf-fortress:

$ ls -1 ~/.dwarf-fortress/
blueprints
data
dfhack
dfhack-config
dfhack.init
dfhack.init-example
dfhack-run
errorlog.txt
gamelog.txt
hack
libs
manipulator
onLoad.init-example
PyLNP0.14a.txt
PyLNP0.14c.txt
raw
stderr.log
stdout.log
stonesense

And all works as expected. pyLNP should be patched to conform XDG standard on linux, for example, my quick hack could be found here: https://github.com/Perlovka/portage-overlay/blob/11e8e6fad5a74a1beaa4e5cf3c56599faf79f992/games-util/pylnp/files/pylnp-0.14c-fix-df-directory.patch

This should be reworked though, because data directories should not be in ~/.config directory, but ~/.local/share or even ~/.cache Also, application should respect XDG environment variables, to be able to work in various linux distributions.

Here is XDG base dir spec: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html#basics

Perlovka commented 1 year ago

And here is execution log after the patch (v0.14c):

perlovka@jupiter ~ $ pylnp
INFO: Registering path root as /home/perlovka/.config/pylnp
INFO: Registering path lnp as /home/perlovka/.config/pylnp/LNP
INFO: Registering path keybinds as /home/perlovka/.config/pylnp/LNP/Keybinds
INFO: Registering path graphics as /home/perlovka/.config/pylnp/LNP/Graphics
INFO: Registering path utilities as /home/perlovka/.config/pylnp/LNP/Utilities
INFO: Registering path colors as /home/perlovka/.config/pylnp/LNP/Colors
INFO: Registering path embarks as /home/perlovka/.config/pylnp/LNP/Embarks
INFO: Registering path tilesets as /home/perlovka/.config/pylnp/LNP/Tilesets
INFO: Registering path baselines as /home/perlovka/.config/pylnp/LNP/Baselines
INFO: Registering path mods as /home/perlovka/.config/pylnp/LNP/Mods
WARNING: JSONConfiguration: File PyLNP.json does not exist
INFO: Registering path df as /home/perlovka/.dwarf-fortress
INFO: Registering path data as /home/perlovka/.dwarf-fortress/data
INFO: Registering path init as /home/perlovka/.dwarf-fortress/data/init
INFO: Registering path save as /home/perlovka/.dwarf-fortress/data/save
INFO: Registering path extras as /home/perlovka/.config/pylnp/LNP/Extras
INFO: Registering path defaults as /home/perlovka/.config/pylnp/LNP/Defaults
INFO: Registering path dfhack_config as /home/perlovka/.dwarf-fortress
INFO: Read installed graphics (Phoebus) from log
Pidgeot commented 1 year ago

And all works as expected.

Sure, if you patch it, it can do that! But the unmodified code doesn't do that because the primary use case for the tool is to work in Starter Packs, bundles that include everything in a portable archive. The XDG spec doesn't apply to that scenario.

If you do want to bring in PyLNP with a package manager, DF itself wouldn't come from there, because package managers don't install programs to user-writable directories. That seems to defeat the purpose of having PyLNP in the package manager in the first place, because the user still has to download DF manually and place it in the exact right location. Might as well download a Starter Pack and use that instead.

Perlovka commented 1 year ago

Yes, package managers will not install anything to user directories. But they can make launcher to do that on user behalf :) For example, on my system DF launcher looks like this:

~ $ cat /usr/bin/dwarf-fortress-bin
#!/bin/sh

src_dir=/opt/dwarf-fortress-bin
dst_dir="$HOME/.dwarf-fortress"

if [ ! -d "$dst_dir" ]; then
    mkdir -p "$dst_dir"
    cp -a "$src_dir"/* "$dst_dir"/
    if [[ -f "$dst_dir"/dfhack.init-example ]]; then
        cp "$dst_dir"/dfhack.init-example "$dst_dir"/dfhack.init
    fi
fi

cd "$dst_dir" || exit
exec ./libs/Dwarf_Fortress "$@"

Thats it, if destination directory is not exist, so copy files there, including DF Hack, if installed.

Anyway, DF directory lookup code logic seems to be too complicated and redundant, and command line parameter for custom DF directory does not work anyway. So please find patch attached and consider to apply it.

The new logic is following: if there is command line parameter then check if it is valid DF directory. If true, add this path to lnp.folders and continue to launch program. else try to find DF directories from base path upward, as before.

Also, I dropped unneeded (IMO) multiple reassigments of self.BASEDIR variable, because it hard to debug such a code.

Additionally, maybe lnp.folders tuple should be changed just to something like lnp.df_folder variable, because anyway only first one is selected to work with. But here I'm not sure. Maybe it's better to allow user to choose which one to launch.

pylnp-0.14c-fix_find_df.patch.txt

diff -urN python-lnp-0.14c-orig/core/df.py python-lnp-0.14c/core/df.py
--- python-lnp-0.14c-orig/core/df.py    2022-09-27 02:03:07.000000000 +0300
+++ python-lnp-0.14c/core/df.py 2022-10-01 18:56:03.137623792 +0300
@@ -17,24 +17,17 @@
 from . import hacks, paths, log
 from .lnp import lnp, VERSION

-def find_df_folders():
+def is_df_folder(path):
+    if os.path.isdir(path) and os.path.exists(os.path.join(path, 'data', 'init', 'init.txt')):
+        return True
+    return False
+
+def find_df_folders(path):
     """Locates all suitable Dwairf Fortress installations (folders starting
     with "Dwarf Fortress" or "df")"""
     lnp.folders = tuple([
-        os.path.basename(o) for o in glob(os.path.join(lnp.BASEDIR, '*')) if
-        os.path.isdir(o) and os.path.exists(os.path.join(
-            o, 'data', 'init', 'init.txt'))])
-
-def find_df_folder():
-    """Tries to select a Dwarf Fortress folder. The set of valid folders is
-    first detected. If a folder name is passed as the first argument to the
-    script, that folder will be used. Otherwise, if only one valid folder was
-    detected, that one will be selected."""
-    find_df_folders()
-    if len(lnp.folders) == 1:
-        set_df_folder(lnp.folders[0])
-    if lnp.args.df_folder and lnp.args.df_folder in lnp.folders:
-        set_df_folder(lnp.args.df_folder)
+        o for o in glob(os.path.join(path, '*')) if
+        is_df_folder(o)])

 def set_df_folder(path):
     """
diff -urN python-lnp-0.14c-orig/core/lnp.py python-lnp-0.14c/core/lnp.py
--- python-lnp-0.14c-orig/core/lnp.py   2022-09-27 02:03:07.000000000 +0300
+++ python-lnp-0.14c/core/lnp.py    2022-10-01 17:30:41.844578154 +0300
@@ -128,7 +129,6 @@
     def initialize_program(self):
         """Initializes the main program (errorlog, path registration, etc.)."""
         from . import paths, utilities, errorlog
-        self.BASEDIR = '.'
         self.detect_basedir()
         paths.clear()
         paths.register('root', self.BASEDIR)
@@ -202,9 +202,8 @@
         """Initializes the DF folder and related variables."""
         from . import df
         self.df_info = None
-        self.folders = []
         self.settings = None
-        df.find_df_folder()
+        df.set_df_folder(lnp.folders[0])

     def initialize_ui(self):
         """Instantiates the UI object."""
@@ -287,13 +286,17 @@

         from . import df
         try:
-            while os.path.abspath(self.BASEDIR) != prev_path:
-                df.find_df_folders()
+            if lnp.args.df_folder and df.is_df_folder(lnp.args.df_folder):
+                self.folders = tuple([lnp.args.df_folder])
+                return
+            path = self.BASEDIR
+            while os.path.abspath(path) != prev_path:
+                df.find_df_folders(path)
                 if len(self.folders) != 0:
                     return
                 # pylint:disable=redefined-variable-type
-                prev_path = os.path.abspath(self.BASEDIR)
-                self.BASEDIR = os.path.join(self.BASEDIR, '..')
+                prev_path = os.path.abspath(path)
+                path = os.path.join(path, '..')
         except UnicodeDecodeError:
             # This seems to no longer be an issue, but leaving in the check
             # just in case
Perlovka commented 1 year ago

P.S. updated previos comment with better patch, shorter _find_dffolders function version.

Pidgeot commented 1 year ago

For example, on my system DF launcher looks like this:

And what happens when you update DF? Your old copy doesn't get replaced in that script.

Yes, I know it would be trivial for you to just add the version number to the folder you copy to and leave the old folder in place. But because you used a hidden folder, the user doesn't know where to find their saves, and if you make it visible, then you're cluttering up their home directory with visible directories. So importing your old save isn't going to work here, and that's an important feature.

Also, I dropped unneeded (IMO) multiple reassigments of self.BASEDIR variable

BASEDIR is also used by the automatic updating code for the starter packs, in core/update.py. If you're going to claim it's unneeded, then please show me that this code doesn't break that in a Starter Pack context.

Additionally, maybe lnp.folders tuple should be changed just to something like lnp.df_folder variable, because anyway only first one is selected to work with. But here I'm not sure. Maybe it's better to allow user to choose which one to launch.

That's already a thing; if multiple valid folders are detected it brings up a selection dialog (see ensure_df() in tkgui/tkgui.py). That also needs to keep working.

If you want to keep working on this, then by all means - if you can come up with something solid, I'm happy to consider it. But please make a proper pull request if you're going to do that, and show that you've fully understood what effect your changes have, because this is too big a change for me to try to hack something together based on incomplete patches.

Perlovka commented 1 year ago

LOL, seems like I should start from reading documentation first and not jump directtly to code, because I thought BASEDIR is where PyLNP configuration is stored. Things become more complicated than I thought ) Maybe I'll start from fixing the current issue with command line parameter. Not touching anything else. Thanks for the explanations.

And what happens when you update DF? Your old copy doesn't get replaced in that script. That script is not for updating, only ensure that there is at least one copy of DF to run Anyway, it's my private setup. Just an example.

Pidgeot commented 1 year ago

That script is not for updating, only ensure that there is at least one copy of DF to run Anyway, it's my private setup. Just an example.

Sure, I get it - and in that context it's totally fine. But in order to make system-wide installs a properly supported feature, I need to be able to explain how to make everything work, including how upgrades should be handled. Many distros would probably have to leave that to the user anyway, since DF itself is not free software, but it still needs to be documented how they can do it.

(That's also the sort of thing that could become very relevant for the Steam version, so it could have uses beyond Linux - but I'm not sure if PyLNP is going to be able to meaningfully support that one anyway. Hard to say at this point in time.)

Perlovka commented 1 year ago

BTW, I found a bug in current version:

When DF directory is located at least one level up from PyLNP executable, it is impossible to open DF directories from UI Probably in https://github.com/Pidgeot/python-lnp/blob/f7dbee14a7c6c8c7d01ec891fa87717e1c995bbb/core/launcher.py#L149 Some debug here (trying to open save dir, DF dir is one level up):

root path:  ./..
folder path:  <df>/data/save
replaced folder path:  ./../df/data/save
joined path:  ./.././../df/data/save
xdg-open: file '../../df/data/save' does not exist

If I change this to just

open_file(lnp.config['folders'][i][1].replace('<df>', paths.get('df')))

Issue is gone ( at least I think so, if there is no something related, that I'm missing again ;) )

Perlovka commented 1 year ago

As for system wide install, I'm thinking about PyLNP itself. Currently it not supported because of the way it searches DF: from it's binary up in directory tree.

I think it could be better to work like any other applications:

Should be one default config directory, for example ~/.config/pylnp LNP directory should be stored either in standard data dir like ~/.local/share/pylnp dir, or in games dir, preferrably should be configurable too. And so on, cache dir etc. And those should be default paths for every OS, appdirs module could help with this: https://pypi.org/project/appdirs/ Application should have at least one default games directory, where DFs are installed, like ~/pylnp or ~/Games/pylnp User should be able to change games directory via UI or in ~/.config/pylnp/pylnp.json|user Also, user should have ability to use multiple games directories. All stored in config.

As for installing DF, it could be manually, or PyLNP could have ability to download releases directly from upstream or any other URL.

This way it will work both as standalone app, unpacked in any directory you like, or as system application. Also, in this way all paths will be more clear, like /home/user/Games/df and not ../../../../df, although this could be achieved even now.

Thats my honest opinion )