Replace Platform.IsMono with actual OS checks #356
Conversation
Some cases of Platform.IsMono are workarounds for bugs in older versions of mono. Those can probably be removed, but for safety's sake, we'll leave them alone for now. Other cases are clearly trying to check the platform, to run Linux-only or Windows-only code. Those cases should be replaced by Platform.IsUnix or Platform.IsWindows as appropriate, because nearly everywhere that Chorus is being run on Linux these days, it's running under dotnet rather than mono, and the Platform.IsMono check returns false.
|
@hahn-kev - This bug is "in the wild" in LfMerge. If LfMerge ever encounters a merge conflict between two .txt files, it will try to run Windows-only code and promptly error out, resulting in a Mercurial rollback because the merge conflict wasn't handled. Thankfully it's extremely unlikely that anyone will check .txt files into their FieldWorks project, let alone end up with a merge conflict in those files. However, I want to prioritize reviewing and merging this bugfix so that I can push a new LfMerge release that won't try to run Windows-only code. |
| { | ||
| Process p = new Process(); | ||
| ProcessStartInfo startInfo = new ProcessStartInfo(); |
There was a problem hiding this comment.
this startInfo isn't being used as far as I can tell
There was a problem hiding this comment.
Yep, that was leftover from an earlier attempt to solve the bug with quoting filenames with spaces. I'll remote it.
There was a problem hiding this comment.
Done in commit 3b4f7d1.
| quotedArgs.Add(arg); | ||
| } | ||
| } | ||
| p.StartInfo.Arguments = string.Join(" ", quotedArgs); |
There was a problem hiding this comment.
I don't love the idea of changing this, was there an issue with how it worked before?
There was a problem hiding this comment.
Yes, this code was not working, because the "test projéct" project has spaces in the filename and the quoting was incorrect. See https://github.com/sillsdev/chorus/actions/runs/11571890533/job/32210729290#step:7:4936 for an example of the errors that were happening: "ChorusMerge Error: Got error 2 diff3: extra operand '/tmp/hgmerge-ha1g3_90/one~other.txt'". That extra operand is because "test projéct", unquoted, was becoming two parameters, "test" and "projéct".
Furthermore, this failure was going unnoticed in the tests, because the test in question was expecting ChorusMerge to fail (due to an unresolvable merge conflict). But the diff3 code here was completely wrong, and so the ChorusMerge code these tests were supposed to test was never actually getting called.
| } | ||
|
|
||
| protected static string SurroundWithQuotes(string path) | ||
| { | ||
| return "\"" + path + "\""; | ||
| } | ||
|
|
||
| public static string RunProcess(string filePath, string arguments) | ||
| public static string RunProcess(string filePath, string[] arguments) |
There was a problem hiding this comment.
this is a public API so it may be called from outside this project making this a breaking change, is it really required or can we just leave it as is.
There was a problem hiding this comment.
I can back the quoting code up to before the RunProcess call, I suppose. However, this is such an internal method that if anyone is calling it from outside, despite its being public, they're doing the wrong thing. This method really should be private. Note how it checks the exit code of the process of specific values and returns specific error messages (e.g. if the exit code is 2, then it throws an ApplicationException about merging text files). This is not a general-purpose method and should never be considered part of the API.
There was a problem hiding this comment.
Commit 4932740 restores RunProcess to its original state and puts the quoting logic in the method that calls it.
Fix #355.
Some cases of Platform.IsMono are workarounds for bugs in older versions of mono. Those can probably be removed, but for safety's sake, we'll leave them alone for now.
Other cases are clearly trying to check the platform, to run Linux-only or Windows-only code. Those cases should be replaced by Platform.IsUnix or Platform.IsWindows as appropriate, because nearly everywhere that Chorus is being run on Linux these days, it's running under dotnet rather than mono, and the Platform.IsMono check returns false.
This change is