use dev.dirs to calculate terasology directories #5284

soloturn · 2024-10-12T15:39:53Z

the library works according to the XDG standard, and offers translations for linux, macosx, windows. see for details:

https://github.com/dirs-dev/directories-jvm

so ...

❯ ls -la /home/st/.local/share/terasology/
gradle game
...
❯ ls -la /home/st/.local/share/terasology/
.rw-r--r-- 9.3k rt 12 Okt 17:56 config.cfg
drwxr-xr-x    - rt 12 Okt 17:56 configs
drwxr-xr-x    - rt 12 Okt 17:56 logs
drwxr-xr-x    - rt 12 Okt 17:56 modules
drwxr-xr-x    - rt 12 Okt 17:56 recordings
drwxr-xr-x    - rt 12 Okt 17:56 sandbox
drwxr-xr-x    - rt 12 Okt 17:56 saves
drwxr-xr-x    - rt 12 Okt 17:56 screenshots
drwxr-xr-x    - rt 12 Okt 17:56 shaders

❯ export XDG_DATA_HOME /tmp/xdg-for-test 
❯ ls -la /tmp/xdg-for-test/terasology/
.rw-r--r-- 9.3k rt 12 Okt 18:04 config.cfg
drwxr-xr-x    - rt 12 Okt 18:04 configs
drwxr-xr-x    - rt 12 Okt 18:04 logs
drwxr-xr-x    - rt 12 Okt 18:04 modules
drwxr-xr-x    - rt 12 Okt 18:04 recordings
drwxr-xr-x    - rt 12 Okt 18:04 sandbox
drwxr-xr-x    - rt 12 Okt 18:04 saves
drwxr-xr-x    - rt 12 Okt 18:04 screenshots
drwxr-xr-x    - rt 12 Okt 18:04 shaders

BenjaminAmos · 2024-10-15T18:53:00Z

It looks like the directories-jvm library uses a Powershell script to try and identify special folders on Windows, which may fail in restricted environments (see also the pinned issue https://github.com/dirs-dev/directories-jvm/issues/49). In general the library would probably work well for macOS and Linux but I have reservations about it continuing to work under Windows. Our current approach using JNA to call the supported Win32 API is much more robust.

This may still be the better approach overall but I need to be sure that it will not make things worse for Windows users.

BenjaminAmos

At a cursory glance, these changes appear to break several existing workflows, especially those using a different home directory.

engine/src/main/java/org/terasology/engine/core/PathManager.java

BenjaminAmos · 2024-10-15T19:26:14Z

engine/src/main/java/org/terasology/engine/core/PathManager.java

@@ -67,7 +65,7 @@ public final class PathManager {

    private PathManager() {
        installPath = findInstallPath();
-        homePath = installPath;
+        homePath = PROJECT_PATH;


Suggested change

homePath = PROJECT_PATH;

homePath = installPath;

findInstallPath() provides a fallback path for unusual scenarios that I think we should keep. Defaulting to the current directory is normal for development anyway, I think. You can default to PROJECT_PATH in findInstallPath if needed.

returning projectpath in a function called findinstallpath i thought is too much :) but let me digest your suggestion ...

BenjaminAmos · 2024-10-15T19:27:49Z

facades/PC/src/main/java/org/terasology/engine/Terasology.java

-        if (homeDir != null) {
-            logger.info("homeDir is {}", homeDir);
-            PathManager.getInstance().useOverrideHomePath(homeDir);
-            // TODO: what is this?
-            //   PathManager.getInstance().chooseHomePathManually();
-        } else {
-            PathManager.getInstance().useDefaultHomePath();
-        }
+        PathManager.getInstance().useDefaultHomePath();


This should not have been removed. Having the ability to override the game's home directory is important for testing with multiple clients.

how is this test done, or in other words, why setting XDG environment variables is not good enough?

BenjaminAmos · 2024-10-15T19:28:44Z

engine/src/main/java/org/terasology/engine/core/PathManager.java

-        logPath = homePath.resolve(LOG_DIR);
-        shaderLogPath = logPath.resolve(SHADER_LOG_DIR);
+        logPath = Paths.get(LOG_DIR);
+        shaderLogPath = Paths.get(SHADER_LOG_DIR);


Why are logPath and shaderLogPath absolute paths when all the other paths are relative to the home directory?

Why are logPath and shaderLogPath absolute paths when all the other paths are relative to the home directory?

the reason why i like the new appdir / xdg / macos directory structure so much is that one can properly separate stuff which should be in a backup and what not. config i want to backup. shared data as well. local data not, but depends on the use case. cache and logs i never want a backup. coming back to your question: i did not see any harm at the moment to leave e.g. config in homedirectory, even it is not xdg compliant.

soloturn · 2024-10-16T02:36:58Z

It looks like the directories-jvm library uses a Powershell script to try and identify special folders on Windows, which may fail in restricted environments (see also the pinned issue https://github.com/dirs-dev/directories-jvm/issues/49). In general the library would probably work well for macOS and Linux but I have reservations about it continuing to work under Windows. Our current approach using JNA to call the supported Win32 API is much more robust.

This may still be the better approach overall but I need to be sure that it will not make things worse for Windows users.

i share your concern, and put a comment on dirs-dev/directories-jvm#61, am thinking that one could make a pull request towards that library even ... and, i left a rant at microsofts openjdk, which i am less fond of: microsoft/openjdk#628.

the library works according to the XDG standard, and offers translations for linux, macosx, windows. see for details: https://github.com/dirs-dev/directories-jvm fixes #5281. Co-authored-by: BenjaminAmos <[email protected]>

sideeffffect · 2024-10-23T11:50:58Z

engine/src/main/java/org/terasology/engine/core/PathManager.java

-        logPath = homePath.resolve(LOG_DIR);
-        shaderLogPath = logPath.resolve(SHADER_LOG_DIR);
+        logPath = Paths.get(LOG_DIR);
+        shaderLogPath = Paths.get(SHADER_LOG_DIR);
        screenshotPath = homePath.resolve(SCREENSHOT_DIR);
        nativesPath = installPath.resolve(NATIVES_DIR);
        configsPath = homePath.resolve(CONFIGS_DIR);


Suggested change

configsPath = homePath.resolve(CONFIGS_DIR);

configsPath = Paths.get(CONFIGS_DIR);

sideeffffect · 2024-10-23T11:51:37Z

engine/src/main/java/org/terasology/engine/core/PathManager.java

    private static final String MODULE_DIR = "modules";
-    private static final String MODULE_CACHE_DIR = "cachedModules";
+    private static final String MODULE_CACHE_DIR = PROJECT_DIRS.cacheDir + "/cachedModules";
    private static final String SCREENSHOT_DIR = "screenshots";
    private static final String NATIVES_DIR = "natives";
    private static final String CONFIGS_DIR = "configs";


Suggested change

private static final String CONFIGS_DIR = "configs";

private static final String CONFIGS_DIR = PROJECT_DIRS.configDir;

soloturn force-pushed the xdg-paths branch 3 times, most recently from fe98759 to a688903 Compare October 12, 2024 15:52

soloturn requested a review from BenjaminAmos October 13, 2024 07:20

BenjaminAmos requested changes Oct 15, 2024

View reviewed changes

soloturn mentioned this pull request Oct 16, 2024

Deal with Windows support being an ongoing shit show dirs-dev/directories-jvm#49

Closed

soloturn mentioned this pull request Oct 16, 2024

Use Java 22's foreign function API for Windows dirs-dev/directories-jvm#61

Merged

soloturn force-pushed the xdg-paths branch from 0dbbab8 to 5ca9ba0 Compare October 16, 2024 03:22

use dev.dirs to calculate terasology directories

ff59f29

the library works according to the XDG standard, and offers translations for linux, macosx, windows. see for details: https://github.com/dirs-dev/directories-jvm fixes #5281. Co-authored-by: BenjaminAmos <[email protected]>

soloturn force-pushed the xdg-paths branch from 5ca9ba0 to ff59f29 Compare October 23, 2024 05:01

sideeffffect reviewed Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use dev.dirs to calculate terasology directories #5284

use dev.dirs to calculate terasology directories #5284

soloturn commented Oct 12, 2024 •

edited

Loading

BenjaminAmos commented Oct 15, 2024

BenjaminAmos left a comment

BenjaminAmos Oct 15, 2024

soloturn Oct 16, 2024

BenjaminAmos Oct 15, 2024

soloturn Oct 16, 2024 •

edited

Loading

BenjaminAmos Oct 15, 2024

soloturn Oct 16, 2024 •

edited

Loading

soloturn commented Oct 16, 2024 •

edited

Loading

sideeffffect Oct 23, 2024

sideeffffect Oct 23, 2024

	configsPath = homePath.resolve(CONFIGS_DIR);
	configsPath = Paths.get(CONFIGS_DIR);

	private static final String CONFIGS_DIR = "configs";
	private static final String CONFIGS_DIR = PROJECT_DIRS.configDir;

use dev.dirs to calculate terasology directories #5284

Are you sure you want to change the base?

use dev.dirs to calculate terasology directories #5284

Conversation

soloturn commented Oct 12, 2024 • edited Loading

BenjaminAmos commented Oct 15, 2024

BenjaminAmos left a comment

Choose a reason for hiding this comment

BenjaminAmos Oct 15, 2024

Choose a reason for hiding this comment

soloturn Oct 16, 2024

Choose a reason for hiding this comment

BenjaminAmos Oct 15, 2024

Choose a reason for hiding this comment

soloturn Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

BenjaminAmos Oct 15, 2024

Choose a reason for hiding this comment

soloturn Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

soloturn commented Oct 16, 2024 • edited Loading

sideeffffect Oct 23, 2024

Choose a reason for hiding this comment

sideeffffect Oct 23, 2024

Choose a reason for hiding this comment

soloturn commented Oct 12, 2024 •

edited

Loading

soloturn Oct 16, 2024 •

edited

Loading

soloturn Oct 16, 2024 •

edited

Loading

soloturn commented Oct 16, 2024 •

edited

Loading