Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set X-AppImage-Version when creating AppImage #156

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

felipemarins
Copy link
Contributor

Addresses #155.

Uses the VERSION environment variable to set a X-AppImage-Version key for the .desktop file inside the AppImage.

Program.cs Outdated
@@ -620,7 +620,7 @@ private static void findSolutionPath()
solutionPath = path;
}

private static bool runCommand(string command, string args, bool useSolutionPath = true)
private static bool runCommand(string command, string args, bool useSolutionPath = true, bool setXAppImageVersion = false, string? version = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the bool is required. You can just always set it.

Copy link
Contributor Author

@felipemarins felipemarins Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the conditional entirely.
Edit: I'm just unsure if there is any other program that uses this environment variable.

I left the conditional for the case where the version parameter is null.

Edit: maybe the conditional is really unnecessary as theoretically it would not be possible to have a pre-existing VERSION env var since UseShellExecute is set to false. Sorry for the 2 force-pushes.

@felipemarins felipemarins force-pushed the game-version-on-appimage branch 2 times, most recently from 13f8084 to e4a3b35 Compare January 10, 2024 04:58
Program.cs Outdated
Comment on lines 637 to 638
if (version != null)
psi.EnvironmentVariables["VERSION"] = version;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this is a little ad-hoc... Sticking this in this generic helper is a little icky.

I'd probably do something like

diff --git a/Program.cs b/Program.cs
index 869914e..1956892 100644
--- a/Program.cs
+++ b/Program.cs
@@ -325,7 +325,11 @@ namespace osu.Desktop.Deploy
                     // osu.AppImage.zsync is AppImage update information file, that is generated by the tool
                     // more information there https://docs.appimage.org/packaging-guide/optional/updates.html?highlight=update#using-appimagetool
                     runCommand(appImageToolPath,
-                        $"\"{stagingTarget}\" -u \"gh-releases-zsync|ppy|osu|latest|osu.AppImage.zsync\" \"{Path.Combine(Environment.CurrentDirectory, "releases")}/osu.AppImage\" --sign", false, version);
+                        $"\"{stagingTarget}\" -u \"gh-releases-zsync|ppy|osu|latest|osu.AppImage.zsync\" \"{Path.Combine(Environment.CurrentDirectory, "releases")}/osu.AppImage\" --sign", false,
+                        new Dictionary<string, string>
+                        {
+                            ["VERSION"] = version
+                        });
 
                     // mark finally the osu! AppImage as executable -> Don't compress it.
                     runCommand("chmod", $"+x \"{Path.Combine(Environment.CurrentDirectory, "releases")}/osu.AppImage\"");
@@ -620,7 +624,7 @@ namespace osu.Desktop.Deploy
             solutionPath = path;
         }
 
-        private static bool runCommand(string command, string args, bool useSolutionPath = true, string? version = null)
+        private static bool runCommand(string command, string args, bool useSolutionPath = true, Dictionary<string, string>? environmentVariables = null)
         {
             write($"Running {command} {args}...");
 
@@ -634,8 +638,11 @@ namespace osu.Desktop.Deploy
                 WindowStyle = ProcessWindowStyle.Hidden
             };
 
-            if (version != null)
-                psi.EnvironmentVariables["VERSION"] = version;
+            if (environmentVariables != null)
+            {
+                foreach (var pair in environmentVariables)
+                    psi.EnvironmentVariables.Add(pair.Key, pair.Value);
+            }
 
             Process? p = Process.Start(psi);
             if (p == null) return false;

Copy link
Contributor Author

@felipemarins felipemarins Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing something like that, but since it would be used only one time, I thought it would be better if it was more simple. But I will implement it like that if you prefer it that way.

Copy link
Contributor Author

@felipemarins felipemarins Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically did the same thing as you. I hope it's fine, but maybe it'd be better if you made the commit since I did almost nothing.

Edit: Sorry, I forgot to add the changes.

@felipemarins felipemarins force-pushed the game-version-on-appimage branch from e4a3b35 to 40cbe88 Compare January 11, 2024 20:10
@felipemarins felipemarins force-pushed the game-version-on-appimage branch from 40cbe88 to 43e038f Compare January 11, 2024 20:23
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to set the envvar correctly. @peppy plz check that you're OK with the changes at the code level whenever convenient

@peppy peppy merged commit 8af03d2 into ppy:master Jan 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants