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.

Sign up for GitHub

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

Jump to bottom

Compatibility rework #1348

Closed
wants to merge 24 commits into from
Closed

Compatibility rework #1348

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4a3f2d6
First successful compile and launch. May need tweaking
rankynbass Jun 17, 2023
6b698e3
Did a better LD_PRELOAD merge
rankynbass Jun 17, 2023
6e29c1d
Removed old constants from compatibilitytools
rankynbass Jun 17, 2023
4d62a15
Move more logic to runners
rankynbass Jun 18, 2023
99f8d75
More work done
rankynbass Jun 22, 2023
0634482
Move WineRunner, DxvkRunner to FFQL repo
rankynbass Jun 24, 2023
b0a8bd3
Add unpatched wine fix
rankynbass Jun 24, 2023
c37927c
Move MacVideoFix to xlcore
rankynbass Jun 24, 2023
d8e66ce
Fixed wrong folder in DxvkRunner.cs
rankynbass Jun 25, 2023
78e02be
Revert "Move MacVideoFix to xlcore"
rankynbass Jun 25, 2023
5c02352
Merge branch 'master' into compatibility-rework
rankynbass Jun 25, 2023
c6353d5
Implemented a bunch of stuff suggested by marzent:
rankynbass Jun 28, 2023
9284ecb
Forgot to fix Environment assignment in Settings classes
rankynbass Jun 28, 2023
6b73619
Fixed broken logic for finding wine or wine64.
rankynbass Jun 29, 2023
4624e0a
Fixed missing !
rankynbass Jun 29, 2023
22b5b08
Clean up the Ensure() functions, moved duplicate
rankynbass Jun 30, 2023
3076a12
Fix logic in GetUnixProcessId(), error handling of UnixDalamudRunner
rankynbass Jul 2, 2023
a16e3b1
Add workaround for REST & proton wine
rankynbass Jul 6, 2023
c07806f
No need for winedbg command if winePid is 0.
rankynbass Jul 6, 2023
ffef16d
Re-added global LD_PRELOAD so it gets merged.
rankynbass Jul 8, 2023
11cc916
Code cleanup per marzents coments
rankynbass Jul 23, 2023
1f697f3
Merge in marzent@f50acf4, remove [DALAMUD] in console output
rankynbass Jul 23, 2023
e7794d3
Get rid of unnecessary LD_PRELOAD merge.
rankynbass Jul 23, 2023
ec316fe
Rename some vars to make their purpose more apparent.
rankynbass Jul 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 117 additions & 108 deletions src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,97 +17,75 @@ namespace XIVLauncher.Common.Unix.Compatibility;

public class CompatibilityTools
{
private DirectoryInfo toolDirectory;
private DirectoryInfo wineDirectory;

private DirectoryInfo dxvkDirectory;

private StreamWriter logWriter;

#if WINE_XIV_ARCH_LINUX
private const string WINE_XIV_RELEASE_URL = "https://github.com/goatcorp/wine-xiv-git/releases/download/8.5.r4.g4211bac7/wine-xiv-staging-fsync-git-arch-8.5.r4.g4211bac7.tar.xz";
#elif WINE_XIV_FEDORA_LINUX
private const string WINE_XIV_RELEASE_URL = "https://github.com/goatcorp/wine-xiv-git/releases/download/8.5.r4.g4211bac7/wine-xiv-staging-fsync-git-fedora-8.5.r4.g4211bac7.tar.xz";
#else
private const string WINE_XIV_RELEASE_URL = "https://github.com/goatcorp/wine-xiv-git/releases/download/8.5.r4.g4211bac7/wine-xiv-staging-fsync-git-ubuntu-8.5.r4.g4211bac7.tar.xz";
#endif
private const string WINE_XIV_RELEASE_NAME = "wine-xiv-staging-fsync-git-8.5.r4.g4211bac7";

public bool IsToolReady { get; private set; }

public WineSettings Settings { get; private set; }
public WineRunner WineSettings { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why rename WineSettings to WineRunner, when it still only holds settings and everything is actually run here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Can certainly change this back to WineSettings.

It made more sense when I started the whole thing, honestly. The names just kind of stuck around. I can certainly change it back. When I started this whole thing, Dxvk and Wine were both in the same object, but it became too complicated and janky, so I split them. I think Runner was chosen simply because that's the folder lutris stuffs wine installs into.


private string WineBinPath => Settings.StartupType == WineStartupType.Managed ?
Path.Combine(toolDirectory.FullName, WINE_XIV_RELEASE_NAME, "bin") :
Settings.CustomBinPath;
private string Wine64Path => Path.Combine(WineBinPath, "wine64");
private string WineServerPath => Path.Combine(WineBinPath, "wineserver");
public DxvkRunner DxvkSettings { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

DxvkRunner is also a Misnomer IMHO


public bool IsToolDownloaded => File.Exists(Wine64Path) && Settings.Prefix.Exists;
private Dictionary<string, string> EnvVars;

private readonly Dxvk.DxvkHudType hudType;
private readonly bool gamemodeOn;
private readonly string dxvkAsyncOn;
public string WineDLLOverrides;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be a public field?


public CompatibilityTools(WineSettings wineSettings, Dxvk.DxvkHudType hudType, bool? gamemodeOn, bool? dxvkAsyncOn, DirectoryInfo toolsFolder)
{
this.Settings = wineSettings;
this.hudType = hudType;
this.gamemodeOn = gamemodeOn ?? false;
this.dxvkAsyncOn = (dxvkAsyncOn ?? false) ? "1" : "0";
private FileInfo LogFile;

this.toolDirectory = new DirectoryInfo(Path.Combine(toolsFolder.FullName, "beta"));
this.dxvkDirectory = new DirectoryInfo(Path.Combine(toolsFolder.FullName, "dxvk"));
public DirectoryInfo Prefix { get; private set; }

this.logWriter = new StreamWriter(wineSettings.LogFile.FullName);
public bool IsToolDownloaded => File.Exists(WineSettings.RunCommand) && Prefix.Exists;

if (wineSettings.StartupType == WineStartupType.Managed)
{
if (!this.toolDirectory.Exists)
this.toolDirectory.Create();

if (!this.dxvkDirectory.Exists)
this.dxvkDirectory.Create();
}
public CompatibilityTools(WineRunner wineSettings, DxvkRunner dxvkSettings, Dictionary<string, string> environment, string wineoverrides, DirectoryInfo prefix, DirectoryInfo toolsFolder, FileInfo logfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

environment should only contain extra user specified env vars that override the standard behaviour

{
WineSettings = wineSettings;
DxvkSettings = dxvkSettings;
EnvVars = environment;
Prefix = prefix;
WineDLLOverrides = (string.IsNullOrEmpty(wineoverrides)) ? "msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi=n,b" : wineoverrides;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting wineoverrides externally and then have this internal fallback here, and the fallback doesn't even take into account dxvkSettings...
IMO they should only be set in 1 place and ideally here

wineDirectory = new DirectoryInfo(Path.Combine(toolsFolder.FullName, "beta"));
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe slightly off-topic but would be nice to rename the "beta" folder

dxvkDirectory = new DirectoryInfo(Path.Combine(toolsFolder.FullName, "dxvk"));
LogFile = logfile;

if (!wineSettings.Prefix.Exists)
wineSettings.Prefix.Create();
}
logWriter = new StreamWriter(LogFile.FullName);

public async Task EnsureTool(DirectoryInfo tempPath)
{
if (!File.Exists(Wine64Path))
{
Log.Information("Compatibility tool does not exist, downloading");
await DownloadTool(tempPath).ConfigureAwait(false);
}
if (!wineDirectory.Exists)
wineDirectory.Create();

EnsurePrefix();
await Dxvk.InstallDxvk(Settings.Prefix, dxvkDirectory).ConfigureAwait(false);
if (!dxvkDirectory.Exists)
dxvkDirectory.Create();

IsToolReady = true;
if (!Prefix.Exists)
Prefix.Create();
}

private async Task DownloadTool(DirectoryInfo tempPath)
public async Task EnsureTool(DirectoryInfo tempPath)
{
using var client = new HttpClient();
var tempFilePath = Path.Combine(tempPath.FullName, $"{Guid.NewGuid()}");

await File.WriteAllBytesAsync(tempFilePath, await client.GetByteArrayAsync(WINE_XIV_RELEASE_URL).ConfigureAwait(false)).ConfigureAwait(false);

PlatformHelpers.Untar(tempFilePath, this.toolDirectory.FullName);
// Check to make sure wine is valid
await WineSettings.Install();
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving together wine configuration and setup results in something like this, which is hard to understand.

CompatibilityTool should handle the CompatibilityTool setup.

if (!File.Exists(WineSettings.RunCommand))
throw new FileNotFoundException("The wine64 binary was not found.");
EnsurePrefix();

Log.Information("Compatibility tool successfully extracted to {Path}", this.toolDirectory.FullName);
// Check to make sure dxvk is valid
if (DxvkSettings.IsDxvk)
Copy link
Contributor

Choose a reason for hiding this comment

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

DxvkSettings.IsDxvk sounds a bit weird (what are DxvkSettings that are not DXVK?) maybe renaming it to DxvkSettings.Enabled would be nicer

await DxvkSettings.Install();
Copy link
Contributor

Choose a reason for hiding this comment

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

similar problem here


File.Delete(tempFilePath);
IsToolReady = true;
}

private void ResetPrefix()
{
Settings.Prefix.Refresh();
Prefix.Refresh();

if (Settings.Prefix.Exists)
Settings.Prefix.Delete(true);
if (Prefix.Exists)
Prefix.Delete(true);

Settings.Prefix.Create();
Prefix.Create();
EnsurePrefix();
}

Expand All @@ -118,86 +96,89 @@ public void EnsurePrefix()

public Process RunInPrefix(string command, string workingDirectory = "", IDictionary<string, string> environment = null, bool redirectOutput = false, bool writeLog = false, bool wineD3D = false)
{
var psi = new ProcessStartInfo(Wine64Path);
psi.Arguments = command;
var psi = new ProcessStartInfo(WineSettings.RunCommand);
psi.Arguments = (WineSettings.RunArguments + " " + command).Trim();

Log.Verbose("Running in prefix: {FileName} {Arguments}", psi.FileName, command);
Log.Verbose("Running in prefix (string): {FileName} {Arguments}", psi.FileName, psi.Arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if "Running in prefix (string)" and "Running in prefix (array)" is easily understood when looking at the logs

return RunInPrefix(psi, workingDirectory, environment, redirectOutput, writeLog, wineD3D);
}

public Process RunInPrefix(string[] args, string workingDirectory = "", IDictionary<string, string> environment = null, bool redirectOutput = false, bool writeLog = false, bool wineD3D = false)
{
var psi = new ProcessStartInfo(Wine64Path);
var psi = new ProcessStartInfo(WineSettings.RunCommand);
if (!string.IsNullOrEmpty(WineSettings.RunArguments))
foreach (var param in WineSettings.RunArguments.Split(null))
psi.ArgumentList.Add(param);
foreach (var arg in args)
psi.ArgumentList.Add(arg);

Log.Verbose("Running in prefix: {FileName} {Arguments}", psi.FileName, psi.ArgumentList.Aggregate(string.Empty, (a, b) => a + " " + b));
Log.Verbose("Running in prefix (array): {FileName} {Arguments}", psi.FileName, psi.ArgumentList.Aggregate(string.Empty, (a, b) => a + " " + b));
return RunInPrefix(psi, workingDirectory, environment, redirectOutput, writeLog, wineD3D);
}

private void MergeDictionaries(StringDictionary a, IDictionary<string, string> b)
private void MergeDictionaries(IDictionary<string, string> a, IDictionary<string, string> b)
{
if (b is null)
return;

foreach (var keyValuePair in b)
{
if (a.ContainsKey(keyValuePair.Key))
a[keyValuePair.Key] = keyValuePair.Value;
{
if (keyValuePair.Key == "LD_PRELOAD")
a[keyValuePair.Key] = MergeLDPreload(a[keyValuePair.Key], keyValuePair.Value);
else
a[keyValuePair.Key] = keyValuePair.Value;
}
else
a.Add(keyValuePair.Key, keyValuePair.Value);
}
}

private string MergeLDPreload(string a, string b)
{
var alist = a.Split(':');
var blist = b.Split(':');
var clist = (System.Environment.GetEnvironmentVariable("LD_PRELOAD") ?? "").Split(':');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we merging in here the system LD_PRELOAD


var merged = (alist.Union(blist)).Union(clist);

var ldpreload = "";
foreach (var item in merged)
ldpreload += item + ":";

return ldpreload.TrimEnd(':');
}

private Process RunInPrefix(ProcessStartInfo psi, string workingDirectory, IDictionary<string, string> environment, bool redirectOutput, bool writeLog, bool wineD3D)
{
psi.RedirectStandardOutput = redirectOutput;
psi.RedirectStandardError = writeLog;
psi.UseShellExecute = false;
psi.WorkingDirectory = workingDirectory;

var wineEnviromentVariables = new Dictionary<string, string>();
wineEnviromentVariables.Add("WINEPREFIX", Settings.Prefix.FullName);
wineEnviromentVariables.Add("WINEDLLOVERRIDES", $"msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi={(wineD3D ? "b" : "n")}");

if (!string.IsNullOrEmpty(Settings.DebugVars))
{
wineEnviromentVariables.Add("WINEDEBUG", Settings.DebugVars);
}

wineEnviromentVariables.Add("XL_WINEONLINUX", "true");
string ldPreload = Environment.GetEnvironmentVariable("LD_PRELOAD") ?? "";

string dxvkHud = hudType switch
{
Dxvk.DxvkHudType.None => "0",
Dxvk.DxvkHudType.Fps => "fps",
Dxvk.DxvkHudType.Full => "full",
_ => throw new ArgumentOutOfRangeException()
};

if (this.gamemodeOn == true && !ldPreload.Contains("libgamemodeauto.so.0"))
{
ldPreload = ldPreload.Equals("") ? "libgamemodeauto.so.0" : ldPreload + ":libgamemodeauto.so.0";
}

wineEnviromentVariables.Add("DXVK_HUD", dxvkHud);
wineEnviromentVariables.Add("DXVK_ASYNC", dxvkAsyncOn);
wineEnviromentVariables.Add("WINEESYNC", Settings.EsyncOn);
wineEnviromentVariables.Add("WINEFSYNC", Settings.FsyncOn);

wineEnviromentVariables.Add("LD_PRELOAD", ldPreload);

MergeDictionaries(psi.EnvironmentVariables, wineEnviromentVariables);
MergeDictionaries(psi.EnvironmentVariables, environment);
var wineEnvironmentVariables = new Dictionary<string, string>();
if (wineD3D)
wineEnvironmentVariables.Add("WINEDLLOVERRIDES", "msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi=b");
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here is completely different from WineDLLOverrides above and should be consolidated (ideally here)

else
wineEnvironmentVariables.Add("WINEDLLOVERRIDES", WineDLLOverrides);
wineEnvironmentVariables.Add("XL_WINEONLINUX", "true");

MergeDictionaries(psi.Environment, WineSettings.Environment);
MergeDictionaries(psi.Environment, DxvkSettings.Environment);
MergeDictionaries(psi.Environment, wineEnvironmentVariables);
MergeDictionaries(psi.Environment, environment);
Log.Verbose("Launching with the following environment:");
foreach (var kvp in psi.Environment)
Log.Verbose(kvp.Key + "=" + kvp.Value);

#if FLATPAK_NOTRIGHTNOW
Copy link
Contributor

Choose a reason for hiding this comment

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

In the corresponding xlcore PR FLATPAK constants are removed but they are still here, would be nice to have a unified flatpack detection/handling logic (also FLATPAK_NOTRIGHTNOW is not really used at all currently)

psi.FileName = "flatpak-spawn";

psi.ArgumentList.Insert(0, "--host");
psi.ArgumentList.Insert(1, Wine64Path);
psi.ArgumentList.Insert(1, WineSettings.RunCommand);

foreach (KeyValuePair<string, string> envVar in wineEnviromentVariables)
foreach (KeyValuePair<string, string> envVar in wineEnvironmentVariables)
{
psi.ArgumentList.Insert(1, $"--env={envVar.Key}={envVar.Value}");
}
Expand Down Expand Up @@ -265,6 +246,28 @@ public Int32 GetUnixProcessId(Int32 winePid)
return unixPids.FirstOrDefault();
}

public Int32 GetUnixProcessIdByName(string executableName)
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a pretty good heuristic, would be even nicer if this can be made private and when calling into GetUnixProcessId and it fails, it maps the wine PID back to its executable name (basically the inverse of GetProcessId) and then calls this.

With that you don't need to any kind of special handling elsewhere and it ensures the asked for PID is actually running inside the correct wine prefix.

{
int closest = 0;
var currentProcess = Process.GetCurrentProcess(); // Gets XIVLauncher.Core's process
bool nonunique = false;
foreach (var process in Process.GetProcessesByName(executableName))
{
if (process.Id < currentProcess.Id)
continue; // Process was launched before XIVLauncher.Core

// Assume that the closest PID to XIVLauncher.Core's is the correct one. But log an error if more than one is found.
if ((closest - currentProcess.Id) > (process.Id - currentProcess.Id) || closest == 0)
{
if (closest != 0) nonunique = true;
closest = process.Id;
}
if (nonunique) Log.Error($"More than one {executableName} found! Selecting the most likely match with process id {closest}.");
}
if (closest != 0) Log.Information($"Process for {executableName} found using fallback method.");
return closest;
}

public string UnixToWinePath(string unixPath)
{
var launchArguments = new string[] { "winepath", "--windows", unixPath };
Expand All @@ -282,12 +285,18 @@ public void AddRegistryKey(string key, string value, string data)

public void Kill()
{
var psi = new ProcessStartInfo(WineServerPath)
var psi = new ProcessStartInfo(WineSettings.WineServer)
{
Arguments = "-k"
};
psi.EnvironmentVariables.Add("WINEPREFIX", Settings.Prefix.FullName);
psi.Environment.Add("WINEPREFIX", Prefix.FullName);

Process.Start(psi);
}

public static bool IsDirectoryEmpty(string folder)
{
if (!Directory.Exists(folder)) return true;
return !Directory.EnumerateFileSystemEntries(folder).Any();
}
}
55 changes: 0 additions & 55 deletions src/XIVLauncher.Common.Unix/Compatibility/Dxvk.cs
View file
Open in desktop

This file was deleted.

Loading