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.

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

Slash and burn: build complexity begone! #284

Merged
merged 30 commits into from
Aug 19, 2022

Conversation

rmunn
Copy link
Collaborator

@rmunn rmunn commented Aug 18, 2022

This PR radically simplifies the build by:

  • Getting rid of the split between FW 8 and FW 9 builds now that we no longer need to build FW 8 versions
  • Getting rid of mono5-sil entirely
  • Getting rid of NuGet.targets in favor of a simple dotnet restore
  • Getting rid of MsBuild targets in LfMerge.proj in favor of a simple dotnet build
  • Getting rid of Test target in LfMerge.proj in favor of a simple dotnet test
  • Getting rid of build/LfMerge.proj entirely and using LfMerge.sln as the entry point to the project
  • Getting rid of the entire build directory tree as it's no longer needed!

The result is a build that runs much faster, won't fail when TeamCity goes down, and uses standard dotnet tools to build. With this change, we'll be able to run local builds on developer machines, without needing to run them inside a Docker image. That should allow running unit tests in VS Code using standard VS Code test-runner extensions.


This change is Reviewable

rmunn added 25 commits August 18, 2022 10:24
We now build for FW9 only, which mean we no longer need the entire
fieldworks8-* branch system. The calculate-branches and pbuild scripts
can become way simpler.
This lets us get rid of the NuGet.targets file and its complexity.
A lot of failures, but now `dotnet test` is able to find and run the
tests. We'll work on the test failures later.
Now that mono-sil5 and its complexity are gone, we can have pbuild.sh
create the base images directly since that doesn't take very long. This
will allow us to iterate more quickly on base image changes.
We no longer need to use LfMerge.proj at all! We can now rely entirely
on the standard `dotnet build` and `dotnet test` build process.
We can move most things into a single build-and-test.sh script.
We only need a few environment variables (such as adjusting the PATH to
point to the version of Mercurial that we need to use), and soon we'll
be able to get rid of those as well.
Instead of using a non-standard `/storage/nuget` folder that won't be on
most people's computers, use the NuGet package cache that they alreday
have, located in `~/.nuget/packages`. Because if they're using VS Code
or any other IDE that knows about .NET and NuGet, that folder is
probably already populated with the appropriate packages, which makes
even the first build go faster.
PrivateAssets="All" is a simpler way to pull in the build targets we
need from that package.
Set TEST_SPEC to a partial (or full) name of a test class or test method
and `dotnet test` will use a "name contains X" filter to locate tests.
To use this, choose the "Run Test Task" command in VS Code. The dotnet
test will run, then pause waiting for a debugger to attach. Go to the
debug menu and choose the "Attach" configuration, then enter the PID
printed on the test console. (There will be two, one for LfMerge.Tests
and one for LfMerge.Core.Tests; most of the tests are in LfMerge.Core so
that's the one you'll need most of the time). The debugger will attach
to the test process, which is still in a paused state. Set any
breakpoints you want to hit in the unit tests, then unpause the debugger
and it will start running the unit tests.
LfMergeSettings was caching the value of environment variables, but some
strings like BaseDir were being accessed in the constructor, before
derived classes like LfMergeSettingsDouble had a chance to change them.
This meant that many tests, which relied on testlangproj or other
projects being copied into the test's temp dir, were failing because
LfMergeSettings was using the default /var/lib/languageforge/... base
directory instead. By removing the caching, we ensure that the
LfMergeSettingsDouble class is able to change the BaseDir correctly.
ProcessingState tests need their expected results updated to include the
new Error property, and the MercurialServer code needs to update its
"shutdown the server" logic to expect "kill" to be a shell builtin
rather than a separate binary. That fixes a total of 9 failing tests.
By removing the `git checkout` and `git clean` steps, we ensure that we
can run the build process against uncomitted code in the repository.
This will make testing changes much simpler.

A leter commit will probably remove the copying of the Git repo into a
temporary working directory, and simply run the build in the user's repo
via a bind mount. That will mean that build outputs (such as TestResult
directories) will end up in your repo, which will be useful in any
situation where you want to debug the unit tests.
Now that LfMergeSettings uses environment variables, we no longer need
to copy lfmerge.conf into the final Docker image. This resolves a
long-standing TODO.
Now that LfMerge logs to the console rather than syslog, we don't need
rsyslog or its config files in the final Docker image.
One step was still using version 2.4.0 even though the rest of the
workflow had moved to version 3.0.2.
This was needed when we were building FW8 and FW9 builds from different
branches, but it's no longer useful. The only thing it was still being
used for is in figuring out which branch to tag in the release workflow,
but since we only run that step when the branch is "live" anyway, we can
just hardcode the branch name. Yet more build complexity, begone!
This fulfills a long-standing TODO comment.
This allows the build to create the installation tarball directly in the
bind-mounted repo, eliminating a copy step.
@rmunn
Copy link
Collaborator Author

rmunn commented Aug 19, 2022

With all these changes, the build has essentially boiled down to three (or four) steps:

  1. dotnet build
  2. (Optional): dotnet test
  3. docker/scripts/create-installation-tarball.sh
  4. docker build -t ghcr.io/sillsdev/lfmerge -f Dockerfile.finalresult .

The only bit of complexity left is that we do need to be able to build different lfmerge images for different model versions in the future, so when model version 7000073 comes out, we'll need to run steps 1-3 in a for loop:

# This is an array; see https://www.gnu.org/software/bash/manual/html_node/Arrays.html
DBMODEL_VERSIONS=(7000072 7000073)

# Run the build once for each DbVersion
for DbVersion in ${DBMODEL_VERSIONS[@]}; do
    dotnet build /v:m /property:Configuration=Release /property:DatabaseVersion=${DbVersion} LfMerge.sln
    docker/scripts/create-installation-tarball.sh
done

# Build final Docker image
docker build -t ghcr.io/sillsdev/lfmerge -f Dockerfile.finalresult .

@rmunn
Copy link
Collaborator Author

rmunn commented Aug 19, 2022

You could almost skip running the build in a Docker container at all, but that causes issues with shared libraries. For example, when I ran the build with dotnet build on an Ubuntu 22.04 machine, then ran the lfmerge image (which uses Microsoft's dotnet/sdk:6.0 image, which is based on Debian 11), lfmergeqm failed to run and I got the following error messages:

/usr/lib/lfmerge/7000072/LfMergeQueueManager: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by /usr/lib/lfmerge/7000072/LfMergeQueueManager)
/usr/lib/lfmerge/7000072/LfMergeQueueManager: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /usr/lib/lfmerge/7000072/LfMergeQueueManager)

So building in the same environment that the image will run in is still a good idea, and we can't quite radically simplify the build down to a simple six-line shell script as shown in the comment above. BUT... the dotnet build and dotnet test steps do work well enough that you can debug unit tests without needing a Docker container involved. Which is a big win.

rmunn added 3 commits August 19, 2022 14:49
This will avoid the possibility of creating a "mixed" lfmerge
installation containing files from a previous successful build,
alongside a build that only partially succeeded and overwrote some but
not all files from before.

This also remvoes just a little bit of unnecessary chattiness from the
build output.
We no longer need the lines about LfMerge.proj or the ones about build
configurations for DbVersions 68, 69, or 70.
We're never going to use MonoDevelop in the future, as VS Code is so
much better. So the whole MonoDevelopProperties section in the .sln file
is a waste of space and processing time. Let's dump the whole thing.
Copy link
Contributor

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

What a great PR! A long time in coming. Thanks also to @josephmyers for laying the foundation, making this slash and burn possible.

.vscode/tasks.json Outdated Show resolved Hide resolved
Co-authored-by: Christopher Hirt <chris@hirtfamily.net>
@rmunn rmunn merged commit a47416d into master Aug 19, 2022
@rmunn rmunn deleted the chore/slash-and-burn-complexity-begone branch August 19, 2022 08:48
@rmunn rmunn self-assigned this Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants