Update Chorus to use Mercurial 6.5.1 and Python 3 #329
Conversation
With the changes in Mercurial 6.5.1 and Python3, ChorusMerge now gets correctly-encoded Unicode filenames as input, without needing to go through a step where UTF-8 is double-encoded as CP1252. We also modify one unit test that relied on the old, broken behavior.
A behavior change between Mercurial 3.3 and 6.5.1 means that these four tests now fail due to Mercurial complaining that the username isn't set. Since the absence of a username has nothing to do with the matter under test, we should just change how these four tests are set up to include a username so that Mercurial is satisfied.
|
Ran a simple performance test on my machine using the new mercurial version for a large ~700mb project.
this gave about a 25% speed increase. In practice we might even see better if the speed boost comes from different network usage. |
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1, all commit messages.
Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @rmunn)
src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs line 255 at r1 (raw file):
uiSection.Set("merge", mergetoolname); uiSection.Set("interactive", "True");
So this is needed in the latest mercurial to get the correct behavior?
|
Previously, jasonleenaylor (Jason Naylor) wrote…
Yes. When Mercurial sees a conflict where one side changed a file while the other one deleted it, it prompts the user for what to do, taking input on stdin if stdin looks like a real tty, i.e. not connected to a pipe. If stdin is piped, Mercurial assumes it's not safe to take input from it, and so it will use the defaults for any prompt and not read stdin at all. The defaults for changed/deleted conflicts used to be "(c)hanged", but in Mercurial 3.7 those defaults were changed to "leave (u)nresolved", which breaks two rather important unit tests. (Specifically, FileDeletedLocallyAndChangedRemotelyKeepsChanged and FileDeletedRemotelyAndChangedLocallyKeepsChanged from HgWrappingTests). However, if you have ui.interactive set to true, you can override Mercurial's "is stdin a tty?" check and force it to take input from stdin even if it's a pipe. At which point the code you wrote in 0893b36 to handle that conflict actually gets a chance to run, and correctly informs Mercurial that we do, in fact, want to keep the changed file. So by setting "ui.interactive = True" we get the status quo ante. |
|
Looks like I'll need to update the file paths in the .wixproj file, as Mercurial 6.5.1 places files in different locations than Mercurial 3.3 did (for example, |
|
I'm going to need some help with updating the GeneratedMercurial.wxs file (made with WiX, apparently), because I'm completely unfamiliar with the WiX toolset and don't know where to start. @jasonleenaylor - any pointers on how I can get started updating the installer definition files? For example, what tool was used to generate them? I can't find the tool name in the Git logs, just a commit that checks in the generated files. |
This automatically creates the GeneratedMercurial.wxs and GeneratedMercurialExtensions.wxs files used in later build steps.
This is needed for the ChorusHub installer to build.
|
Figured out the generated .wxs files. Now they're just missing the .pyc files for the fixutf8 extension, because they didn't get included in the SL.Chorus.Mercurial package. I'll submit a new PR over at Mercurial4Chorus (or maybe just tweak sillsdev/Mercurial4Chorus#12 to include the necessary .pyc files) so that the .pyc files will be present. (We do want to distribute them, so that the Python interpreter doesn't have to create them on first run after users install this). |
|
Submitted the new PR I mentioned: sillsdev/Mercurial4Chorus#13 Once that's merged, I'll push a new commit to this PR that will update the installer with the correct .pyc file locations (they moved and got new names between Python 2 and Python 3), and then this PR should finally be able to build. |
The location where Python stores .pyc files changed between Python 2 and Python 3, so we need to point the installer at the new location.
Apparently the NuGet package being pulled in by the 6.5.1-* qualifier is too old and isn't including the recent fixes.
Now that we know that version 6.5.1.22 is building correctly, we can set up to use that version or any later one.
|
Finally a green build with commit 2a3d101 — it seems I had the syntax wrong for the NuGet package dependency so I was pulling in an older version of SIL.Chorus.Mercurial that didn't have my bugfixes. I'm going to push one more commit going back to a wildcard package version, so that it's easier to pull in any subsequent bugfixes to the SIL.Chorus.Mercurial package, and then this PR will finally be ready for final review and merge. |
The ChorusMerge code no longer matches the comment; update the comment so we don't wonder in the future why the encoding games have gone away.
There was a problem hiding this comment.
I've dismissed old discussions in Reviewable so that they don't hold up the merge. Once the build completes, this will finally be ready for approval and merging.
Dismissed @ermshiperete from 2 discussions.
Reviewable status: 1 of 9 files reviewed, all discussions resolved (waiting on @ermshiperete and @jasonleenaylor)
|
@ermshiperete - While we wait for the build to complete (should be done about an hour from the time of this comment), would you like to look at this again? @jasonleenaylor - Any other concerns, or do you feel this is ready now? |
+semver: major
Fix typo Co-authored-by: Eberhard Beilharz <[email protected]>
There was a problem hiding this comment.
Dismissed @ermshiperete from a discussion.
Reviewable status: 1 of 10 files reviewed, all discussions resolved (waiting on @jasonleenaylor)
There was a problem hiding this comment.
Reviewed 4 of 6 files at r1, 2 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jasonleenaylor)
sillsdev/Mercurial4Chorus#11 will need to be merged first, then this PR will be able to build.
This updates the included version of Mercurial to 6.5.1, using Python 3 instead of Python 2 (which hasn't received any security fixes in nearly four years, since it went end-of-life on January 1st, 2020).
This change is