[Linux Support] Convert windows file paths into platform agnostic paths; use agnostic executables and commands #58

StevenLares · 2024-02-20T20:02:12Z

These commits address these tasks found in my comment under issue #12:

Convert windows file paths into platform agnostic paths.
Remove "exe" from executable file path names if the user is on Linux.

In addition, the final commit in this PR changes the download link to point to the latest release instead of a specific release.

The commit descriptions go into more detail on the exact changes made.

…bility

… name corresponding to user's OS. This function is now used by other classes to store the executable names in member variables. Relevant code now refers to the member variables instead of the previously hardcoded exe names. In addition, I adjusted download links to point to executables (with the platform agnostic exe names) although that will require building the launcher for Linux on each release.

…older release

StevenLares · 2024-02-22T01:57:12Z

So I ran this on a windows machine, and I got an error regarding pathlib Path 😅 .
I'll investigate this when I have more time, don't merge this request. I modify the request later.

Zedb0T · 2024-02-22T02:45:01Z

No worries! I can fixup/upload some linux built binary on one of the mods repos if youd like to have something to test end to end with.

StevenLares · 2024-02-22T03:04:04Z

Yeah that'd be a big help. Thanks

…

-------- Original Message --------

On Feb 21, 2024, 9:45 PM, ZedB0T wrote: No worries! I can fixup/upload some linux built binary on one of the mods repos if youd like to have something to test end to end with. — Reply to this email directly, [view it on GitHub](#58 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADKOBSQS6CZUK6P2H2WOTXTYU2WLRAVCNFSM6AAAAABDRY37ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGU2TSMRQGE). You are receiving this because you authored the thread.Message ID: ***@***.***>

…nged platform.system() to sys.platform for consistency with the rest of the code

… never used. launcherUtils.py now makes use of file path variables declared at the top, removes some file path hardcoding

… with autoupdater.py' on both Linux and Windows

amarzot

Not a maintainer here, but wrote #56 so thought I'd leave some feedback. Feel free to disregard anything you see as out of scope for this PR.

amarzot · 2024-02-22T18:03:39Z

utils/launcherUtils.py

+    if sys.platform == "win32":
+        call = "TASKLIST", "/FI", "imagename eq %s" % process_name
+        try:
+            # use buildin check_output right away
+            output = subprocess.check_output(call).decode()
+            # check in last line for process name
+            last_line = output.strip().split("\r\n")[-1]
+            # because Fail message could be translated
+            return last_line.lower().startswith(process_name.lower())
+        except:
+            return False
+    else:
+        call = ["pgrep", "--list-name", "^" + process_name + "$"]
+        try:
+            output = subprocess.check_output(call).decode()
+            return len(output) > 1
+        except:
+            return False


I wonder if it is worth bringing in psutil for this functionality

If this behaves the same on both Windows and Linux, I think it is worth using it. However I think maintainers should decide if adding a dependency is ok: @Zedb0T

amarzot · 2024-02-22T18:06:48Z

utils/launcherUtils.py

+        if sys.platform == "win32":
+            os.system("taskkill /f /im " + process_name)
+        else:
+            os.system("pkill " + "^" + process_name + "$")


Same comment about psutil here

If this behaves the same on both Windows and Linux, I think it is worth using it. However I think maintainers should decide if adding a dependency is ok: @Zedb0T

amarzot · 2024-02-23T18:55:55Z

utils/launcherUtils.py

+    if sys.platform == "win32":
+        subprocess.check_call('mklink /J "%s" "%s"' % (link, target), shell=True)
+    else:
+        subprocess.check_call('ln -s "%s" "%s"' % (link, target), shell=True)


Should be able to use pathlib

Suggested change

subprocess.check_call('ln -s "%s" "%s"' % (link, target), shell=True)

link.symlink_to(target)

The windows version creates a directory junction.
The pathlib library does not have a junction command.

As far as I can tell, the overall code doesn't need a junction, and will function the same with a call to:

Path.symlink_to(target, target_is_directory=True)

The second parameter is used only on Windows, ignored everywhere else

While there is a function is_junction() in launcherUtils, I think this new implementation will work just the same on Windows

def is_junction(path: str) -> bool: try: return bool(os.readlink(path)) except OSError: return False

iirc there was a reason we went with junctions, I think the pathing somewhere in opengoal itself broke when using symlinks? might be misremembering this

see:
https://github.com/OpenGOAL-Mods/OG-ModLauncher/pull/37/files
https://github.com/OpenGOAL-Mods/OG-ModLauncher/pull/43/files

but I think the key issue was # symlink isodata for custom levels art group (goalc doesnt take -f flag)

I see, thanks for the links.

I will leave the commands as-is, although I corrected the order of Linux's symlink parameters in bab64d0

amarzot · 2024-02-23T18:56:29Z

utils/launcherUtils.py

+    if sys.platform == "win32":
+        subprocess.check_call('mklink /H "%s" "%s"' % (link, target), shell=True)
+    else:
+        subprocess.check_call('ln -s "%s" "%s"' % (link, target), shell=True)


Same here with link.symlink_to

Agreed. I'll add it after a decision on the other similar function is made

I think it is worth keeping it this way to be consistent with the other symlink function

amarzot · 2024-02-23T19:32:51Z

Launcher with autoupdater.py

        window['update_status'].update("Starting Update...")
-        try_remove_dir(AppdataPATH + "/temp")
-        if not os.path.exists(AppdataPATH + "/temp"):
-            os.makedirs(AppdataPATH + "/temp")
+        try_remove_dir(AppdataPATH/"temp")
+        if not os.path.exists(AppdataPATH/"temp"):
+            os.makedirs(AppdataPATH/"temp")

        window['update_status'].update("Downloading update from " + latest_release_assets_url)
        file = urllib.request.urlopen(latest_release_assets_url)
-        urllib.request.urlretrieve(latest_release_assets_url, AppdataPATH + "/temp/OpengoalModLauncher.exe", show_progress)
+        urllib.request.urlretrieve(latest_release_assets_url, AppdataPATH/"temp"/OpengoalModLauncher_exe, show_progress)
        window['update_status'].update("Done downloading")

-        window['update_status'].update("Removing previous installation " + AppdataPATH)
-        try_remove_dir(AppdataPATH + "/data")
-        try_remove_file(AppdataPATH + "/gk.exe")
-        try_remove_file(AppdataPATH + "/goalc.exe")
-        try_remove_file(AppdataPATH + "/extractor.exe")
+        window['update_status'].update(f"Removing previous installation {AppdataPATH}")
+        try_remove_dir(AppdataPATH/"data")
+        try_remove_file(AppdataPATH/gk_exe)
+        try_remove_file(AppdataPATH/goalc_exe)
+        try_remove_file(AppdataPATH/extractor_exe)

        window['update_status'].update("Extracting update")
-        temp_dir = AppdataPATH + "/temp"
-        try_remove_file(temp_dir + "/updateDATA.zip")
+        temp_dir = AppdataPATH/"temp"
+        try_remove_file(temp_dir/"updateDATA.zip")


maybe move temp_dir to the top for reuse, and change the os functions to temp_dir.exists() and temp_dir.mkdir()

See ca63a7d

Also, there are other instances where the code uses os.path and shutil functions where pathlib could be used, although for now I left them as is

amarzot · 2024-02-23T19:47:12Z

utils/launcherUtils.py

+    mod_name_settings_path = AppdataPATH / "OpenGOAL" / GAME / "settings" / (MOD_NAME + "-settings.gc")
+    if mod_name_settings_path.exists():
        # just to be safe delete the migrated settings file if it already exists (shouldn't happen but prevents rename from failing below)
-        if exists(AppdataPATH + "\OpenGOAL" + "//" + GAME + "//" + "settings\\" + MOD_ID + "-settings.gc"):
-            os.remove(
-                AppdataPATH + "\OpenGOAL" + "//" + GAME + "//" + "settings\\" + MOD_ID + "-settings.gc"
-            )
+        mod_id_settings_path = AppdataPATH / "OpenGOAL" / GAME / "settings" / (MOD_ID + "-settings.gc")
+        if mod_id_settings_path.exists():
+            mod_id_settings_path.unlink()


Using AppdataPATH here is an issue as it is derived from appdirs.user_data_dirs which points to ~/.local/share/ on linux when we should really be using ~/.config aka appdirs.user_config_dir. Both the data_dir and config_dir point to Appdata on windows, so that won't break.

See https://github.com/ActiveState/appdirs/blob/master/appdirs.py#L186-L189

Great catch!

Are there any other instances of the code that should also be modified?

Also, I am not sure if mods made from https://github.com/OpenGOAL-Mods/OG-Mod-Base would need to be updated to read their settings from .config on Linux.

amarzot · 2024-02-23T19:50:37Z

Launcher with autoupdater.py

+from utils import launcherUtils
+
+dirs = AppDirs(roaming=True)
+AppdataPATH = Path(dirs.user_data_dir)/"OpenGOAL-UnofficialModLauncher"


This probably needs a rename as this is not the path to the AppData folder, but to the install directory. Also launcherUtils.py has this same variable, but it does point just to AppData

See c44b200

…ved temp_dir earlier to top of code block. It is used instead of hardcoding

…ates, found in both check_for_updates() and download_newest_mod(). Also, I broke up the code into several reusable functions that are each responsible for one task only. Finally, I added a TODO that describes a future code improvement needed for Linux compatibility, which should be easier to implement and debug with this new code structure

StevenLares · 2024-02-24T22:31:57Z

Where this is at:

In 781524c I decompose some redundant code into separate functions. I also added a TODO regarding a future improvement needed once Linux executables are created for the opengoallauncher
Window buttons in the mainframe are broken on Linux for some reason. Haven't been able to debug yet
Open suggestions from this ticket
Any other task from what about making it cross-platform? #12 that doesn't seem to have a commit associated with it

Zedb0T · 2024-02-26T04:01:41Z

Yeah great work here - sorry I've been slow to comment/review etc just been a very busy time for me but I do see the changes and want to get involved/help.

Hopefully soon I'll be able to look at this stuff in full.

If yall aren't already in the modding discord, it might good to join so we can all kind of make sure everyone's on the same page and make a plan for Linux support.

StevenLares · 2024-02-26T19:00:20Z

@Zedb0T Hey no worries, I appreciate the help you have provided so far.

I will join the Discord later today and introduce myself. Can't wait to meet you guys there!

layoutUtils has one public method, generate(), along with several private helper methods. My goals with this commit were to: Separate layout code from the main code. This improves readability for both sets of code. Break up the layout code into separate functions and variables so that it is easier to read and modify. Assemble the layout elements so that they resemble the actual UI, as it is suggested in the pysimplegui docs. Read the returns for side_panel(), main_panel(), and main_panel()'s variable button_panel_with_mod_info for this idea in action.

… temp fix to accomodate large screens with high resolutions. Therefore, I added a TODO on high level next steps as that is out of scope for this pull request

… it results in a keyerror when the executable is run. I have added a default value in case the variable doesn't exist

…o be kept in

StevenLares · 2024-02-29T19:34:04Z

These are my last commits on the launcher, for now.

I am open to responding to code review(s) when they're ready, but no rush.
From Discord, I understand there are higher priority items related to the OG-Mod-Base. At the moment, I don't have the time to contribute much on that front.

There are a few commits here that are slightly out of scope (like changing the dimensions of one of the columns, the introduction of layoutUtils.py, decomposing autoupdater.py into several reusable functions, and TODOs on possible improvements). No new python dependencies have been introduced to the project.

Of course, feel free to discard changes that you see fit. However, some of these changes make it easier for a new contributer to understand the code while improving maintainability for current maintainers.

Let me know if you have any questions.

Thanks

amarzot and others added 5 commits February 20, 2024 14:43

Update all paths to use pathlib Path objects

83cff10

Update autoupdater.py too

08cc70e

Adjusted one additional path to be pathlib, in prep for Linux compati…

2012947

…bility

Changed download links to point to the latest release, instead of an …

a002281

…older release

StevenLares mentioned this pull request Feb 21, 2024

what about making it cross-platform? #12

Open

StevenLares added 3 commits February 22, 2024 11:41

Updated utils functions to include linux / unix commands. Also, I cha…

a1b7675

…nged platform.system() to sys.platform for consistency with the rest of the code

Removed lines that created a mod directory on the filesystem that was…

025befd

… never used. launcherUtils.py now makes use of file path variables declared at the top, removes some file path hardcoding

Fixed bug where the use of pathlib Path caused a crash with 'launcher…

75ff8ef

… with autoupdater.py' on both Linux and Windows

StevenLares changed the title ~~Convert windows file paths into platform agnostic paths; make executables have platform agnostic names~~ [Linux Support] Convert windows file paths into platform agnostic paths; use agnostic executables and commands Feb 22, 2024

amarzot reviewed Feb 23, 2024

View reviewed changes

StevenLares added 3 commits February 24, 2024 17:22

Changed some os path functions to pathlib builtin functions. Also, mo…

ca63a7d

…ved temp_dir earlier to top of code block. It is used instead of hardcoding

Renamed AppdataPATH to LauncherInstallPath

c44b200

StevenLares added 6 commits February 27, 2024 14:42

Merge branch 'layout_utils' into linux_compat

93c1871

Increased Y size of column containing buttons and mod info. This is a…

39f7353

… temp fix to accomodate large screens with high resolutions. Therefore, I added a TODO on high level next steps as that is out of scope for this pull request

Since Linux does not have a COMPUTERNAME variable in its environment,…

f393896

… it results in a keyerror when the executable is run. I have added a default value in case the variable doesn't exist

Created bash scripts that build the launcher executables for Linux

8b2c3a8

Removed TODO's I added earlier in earlier commits, that do not need t…

9948fa9

…o be kept in

Corrected order of Linux symlinks

bab64d0

amarzot mentioned this pull request Mar 6, 2024

[Linux Support] Update all paths to use pathlib Path objects #56

Closed

	subprocess.check_call('ln -s "%s" "%s"' % (link, target), shell=True)
	link.symlink_to(target)

[Linux Support] Convert windows file paths into platform agnostic paths; use agnostic executables and commands #58

Are you sure you want to change the base?

[Linux Support] Convert windows file paths into platform agnostic paths; use agnostic executables and commands #58

Uh oh!

Conversation

StevenLares commented Feb 20, 2024

Uh oh!

StevenLares commented Feb 22, 2024

Uh oh!

Zedb0T commented Feb 22, 2024

Uh oh!

StevenLares commented Feb 22, 2024 via email

Uh oh!

amarzot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dallmeyer Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StevenLares commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zedb0T commented Feb 26, 2024

Uh oh!

StevenLares commented Feb 26, 2024

Uh oh!

StevenLares commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dallmeyer Feb 29, 2024 •

edited

Loading

StevenLares commented Feb 24, 2024 •

edited

Loading

StevenLares commented Feb 29, 2024 •

edited

Loading