search

sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Update Chorus to use Mercurial 6.5.1 and Python 3 #329

Closed rmunn closed 8 months ago

rmunn commented 9 months ago

https://github.com/sillsdev/Mercurial4Chorus/pull/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 Reviewable

github-actions[bot] commented 9 months ago

Test Results

       2 files  ±0     202 suites  ±0   1h 22m 44s :stopwatch: + 38m 26s    876 tests ±0     854 :heavy_check_mark: ±0  22 :zzz: ±0  0 :x: ±0  1 998 runs  ±0  1 932 :heavy_check_mark: ±0  66 :zzz: ±0  0 :x: ±0 

Results for commit 13626ad9. ± Comparison against base commit 74871cb4.

:recycle: This comment has been updated with latest results.

hahn-kev commented 9 months ago
Ran a simple performance test on my machine using the new mercurial version for a large ~700mb project. Version Speed
v3.3 2 min 40 sec
v6.5.1 2 min 0 sec

this gave about a 25% speed increase. In practice we might even see better if the speed boost comes from different network usage.

rmunn commented 9 months ago

src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs line 255 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…
So this is needed in the latest mercurial to get the correct behavior?

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 https://github.com/sillsdev/chorus/commit/0893b36802a659027b8c382d91c71053db2db601 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.

rmunn commented 9 months ago

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, library.zip is now under lib/ instead of being in the root folder).

rmunn commented 9 months ago

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.

rmunn commented 8 months ago

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 https://github.com/sillsdev/Mercurial4Chorus/pull/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).

rmunn commented 8 months ago

Submitted the new PR I mentioned: https://github.com/sillsdev/Mercurial4Chorus/pull/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.

rmunn commented 8 months ago

Finally a green build with commit https://github.com/sillsdev/chorus/pull/329/commits/2a3d10111fafa36afb7f43b5bdb8bcc2717074d4 — 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.

rmunn commented 8 months ago

@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?