-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update mercurial version #333
Conversation
Mercurial 6 doesn't have the httpclient directory any more, so the installer can ignore it.
Now the failure is "Cannot find libdl.so". https://stackoverflow.com/questions/75855053/how-to-address-crash-due-to-missing-libdl-so-on-ubuntu-22 seems relevant here. |
0cf719a
to
26274c2
Compare
Core code was using version 11.0.0-beta0077 but tests were still on LCModel version 10. Update tests to use 11.0.0-beta0077 as well.
TargetDir is defined by Visual Studio but not by the dotnet CLI tools
I've opened sillsdev/chorus#332 to get a net8.0 build of chorusmerge. |
This workflow uses `dotnet test` and `dotnet build` steps directly in the GHA runner, rather than building an LfMerge builder image. That way it's a lot easier to understand the build process, and to replicate it on dev machines (just do `dotnet build` and `dotnet test`).
Commit 528878f updates the GHA workflow to just build with |
ChorusMerge version 6.0.0-beta0044 has a net8.0 build in the NuGet package. Once I get back to this PR, I'll update the ChorusMerge dependency to 6.0.0-beta0044 and that should hopefully fix the last four failing tests. |
dotnet clean can sometimes fail if, for example, you're switching from targeting net6.0 to net8.0. That's not a problem, so we let the script continue if the `dotnet clean` step fails.
Now that ChorusMerge builds for net8.0, we should copy its net8.0 runtimeconfig.json to our build output, not the older net6.0 one.
Okay, LfMerge is building with .NET 8, Mercurial 6, and Python 3, and running the resulting Docker image doesn't immediately fall over. Now to test it in some S/R scenarios against local copies of Lexbox. |
Just got the LDAPI working against local LexBox install. The trick was adding LfMerge runs, but got an autorization error cloning the project. So I haven't yet been able to test the functionality of chorusmerge, which is the main point I want to test before merging this. So I'm not yet ready to merge it, but it's a step in the right direction. |
Mercurial 6 needs hgdemandimport to live alongside hgext.
S/R immediately fell over, several times, because we didn't have all of Mercurial copied into the final Docker image. Turns out between Mercurial 3 and 6, a lot of Mercurial files were moved around internally to different subdirectories, and our installation script didn't account for all of them. But that's sorted out now, and I've been able to prove that S/R, including running chorusmerge, works. Cloning, pulling, merging, pushing, all test out locally. Time to approve this and merge it. |
This will allow us to more easily run a local build and deploy it to local LF for testing, without having to wait for a GHA workflow.
Now that we know the GHA workflow works, we no longer need to run it on pushes to the test branch. We can simply run it on pull requests and that's it.
We used to need to upload a tarball because GitHub Actions wasn't preserving Unix file permissions in the .zip file it would upload, so things like executable bits were getting stripped. Now that there's an upload-artifact@v4 that works on different principles internally, let's see if this step is still necessary. If this procudes Docker images that still run, we might be able to save some build time. We also comment out the `dotnet test` step here to save 7 minutes of waiting for the build; the affected code is after the `dotnet test` so it will continue to pass but tell us nothing about the changes here.
Will we need it in the release step? Let's leave it out for now and see what happens.
GitHub Actions upload-artifact still strips executable bits, so we still need the step of creating a .tar.gz file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's Get This Merged.
the current version does not work properly with https