Optimize the build process #1112
Comments
|
DocFx is used to produce the documentation website, which doesn't work on .NET Core but requires the full .NET Framework or Mono. In my experience, contrary to what Microsoft wants us all to believe through their marketing, running .NET Core on Windows is still more streamlined, more optimized, and better supported due to its history. Linux support has caught up in the last years, but there are still gaps due to fundamental differences in hardware/memory architecture, file systems, multi-threading, etc. In the runtime, there are various low-level constructs such as memory barriers and volatile fields that provide stronger guarantees on x86/x64 than documented, which has become something existing code is relying on. Just as we rely on the fact that We build against Linux to protect ourselves against Windows-only line breaks or encodings, slashes in the wrong direction, etc. in an attempt to support multiple platforms. That doesn't mean the platform we produce our binaries on is irrelevant. I'd like to avoid analyzing subtle bugs in the runtime/compiler infrastructure, which are reported quite frequently, such as here. |
|
OK, if we say that the documentation website and the NuGet packages must be produced on the Windows image, would you be open to exploring the other solution options? Specifically, would you be fine with the first solution that I've called "Run on Windows Only and Possibly Fail Just a Little Later While Creating a Much Better Developer Experience"? Just to be sure, note that "Run on Windows Only" means that |
|
Using a separate repo, I have played a little with appveyor to see how we could optimize further. Here's a sample appveyor.yml file that tries to show the logic. The key idea is to use three jobs:
This gives you the fastest possible failure for build and code style-related issues while saving the time required for running the code inspection on Linux. environment:
matrix:
- APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
PERFORM_CODE_INSPECTION: yes
PERFORM_TEST: no
PERFORM_PUBLISH_STEP: no
- APPVEYOR_BUILD_WORKER_IMAGE: Ubuntu
PERFORM_CODE_INSPECTION: no
PERFORM_TEST: yes
PERFORM_PUBLISH_STEP: no
- APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
PERFORM_CODE_INSPECTION: no
PERFORM_TEST: yes
PERFORM_PUBLISH_STEP: yes
matrix:
fast_finish: true
# We are just using the install phase to display information for demo purposes
install:
- pwsh: |
Write-Host "Image = $Env:APPVEYOR_BUILD_WORKER_IMAGE`n"
Write-Host "PERFORM_CODE_INSPECTION = $Env:PERFORM_CODE_INSPECTION"
Write-Host "PERFORM_TEST = $Env:PERFORM_TEST"
Write-Host "PERFORM_PUBLISH_STEP = $Env:PERFORM_PUBLISH_STEP`n"
before_build:
- pwsh: Write-Host "BEFORE-BUILD PHASE`n"
- pwsh: Write-Host "dotnet tool restore EXECUTED`n"
build_script:
- pwsh: Write-Host "BUILD PHASE`n"
- pwsh: Write-Host "dotnet build EXECUTED`n"
after_build:
- pwsh: Write-Host "AFTER-BUILD PHASE`n"
- pwsh: |
if ($Env:PERFORM_CODE_INSPECTION -eq "yes") {
Write-Host "RunInspectCode function EXECUTED"
Write-Host "RunCleanupCode function EXECUTED`n"
}
test: off
before_test:
- pwsh: Write-Host "BEFORE-TEST PHASE`n"
- pwsh: |
if ($isWindows) {
if ($Env:PERFORM_TEST -eq "yes") {
Write-Host "Starting Azure Cosmos Emulator on Windows`n"
}
}
- sh: |
if [[ "$PERFORM_TEST" == "yes" ]]; then
echo "Starting Azure Cosmos Emulator on Linux"
fi
test_script:
- pwsh: Write-Host "TEST PHASE`n"
- pwsh: |
if ($Env:PERFORM_TEST -eq "yes") {
Write-Host "dotnet test EXECUTED`n"
}
for:
- # Publishing of documentation and NuGet packages
matrix:
only:
- APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
PERFORM_PUBLISH_STEP: yes
# Using README.md as a placeholder for the NuGet package
artifacts:
- path: README.md
# Using FTP provider as a placeholder for the NuGet provider
deploy:
- provider: FTP
protocol: ftp
host: speedtest.tele2.net
username: anonymous
password: anonymous
before_deploy:
- pwsh: Write-Host "BEFORE-DEPLOY PHASE`n"
- pwsh: |
if ($Env:PERFORM_PUBLISH_STEP -eq "yes") {
Write-Host "docfx INSTALLED`n"
}
deploy_script:
- pwsh: Write-Host "DEPLOY PHASE`n"
- pwsh: |
if ($Env:PERFORM_PUBLISH_STEP -eq "yes") {
Write-Host "Documentation PUBLISHED`n"
} |
|
In my opinion, you're trying to solve the wrong problem. Allow me to explain. The cibuild is intended as a final check when all development has been completed. Checks such as: does building in release mode produce compiler warnings, do all the tests succeed on all platforms, does the code style comply with the rules, does the documentation website build, is the NuGet package produced properly, etc. All these checks are based on a fixed set of tool versions so that multiple developers using different versions locally can work together towards a single outcome. Asking for a PR review with the intent to get it merged because the developer believes all the work has been completed is pointless when the cibuild still fails. In all cases, running the cibuild takes a long time. At least long enough that it's not something to go sit and wait for, staring at your screen doing nothing else. Its an async process by nature and its not uncommon for cibuilds to take several hours, such as in the runtime (example here) and the compiler (example here). Based on that, it makes the most sense to optimize the duration of the cibuild for the common case: all green. Today we have a single build script that runs all the stages, which can be run locally too. Running it locally is many times faster than on AppVeyor, so if anyone must wait for its outcome, running locally is by far the fastest way to find out. But usually, there is no reason to have to wait for it. Any issues resulting from it can be addressed at a later time. Now it may be tempting to (mis)use the cibuild for kicking off work that is best run locally, because running locally means you'd need to wait for it, interrupting your coding process. Not only does pushing all the time pollute commit history, it also takes up VM resources that are scarce. In that case, the solution is to be able to run that (on your own fast system) without blocking. For example, you can continue writing code when the compiler is running in the background, tests are running in the background, Resharper is adding squiggles in the background, Windows Updates are running in the background, etc. We already provide various ways to run partial steps of the full build locally, such as One solution to make them non-blocking is to add a local script (with a desktop shortcut pointing to it) that copies the repo into a temp folder, then runs all the checks on the copy in the background. The way the styling settings are configured today is actually optimized to not distract while writing code: lots of "issues" that result in a cibuild failure are intentionally suppressed in Visual Studio and Rider. I always got quite distracted by the pointless warnings added by StyleCop when a single space was missing. That's not at all what I care about when trying to write a complex algorithm! When the cibuild runs cleanupcode for a PR, it runs on only the changed files. This can be accomplished locally too using ReGitLint, passing it the commit hash of your base branch and HEAD, which makes it a lot faster. So to summarize: the cibuild is and will always be very slow by nature, compared to running locally. Our project does not have funding to buy parallel jobs, but if I had some extra money I'd get a faster computer instead of spending it on agents. So if you need fast feedback, run locally (there's room for improvement there). On the other hand, if you want final checks and don't care how long that takes, use the cibuild. |
|
I simply acted on your earlier comments, some of which are at least partially repeated in the description of the issue. In the context of my current PR ##1103, the very long build times and your focus on "code style first" posed an issue. I am trying to contribute back to this project. However, my contribution is originally developed and fully integration-tested on Windows, not Linux. Therefore, running the integration tests on Linux as well is something that I am doing only for this project. Thus, my main concern was to make it work on Linux, which proved to be particularly hard with the Azure Cosmos Emulator for Linux on appveyor specifically. What worked on Ubuntu on WSL locally did not necessarily work on appveyor. Therefore, the only (!) way to find out whether a different configuration or some other change worked was to run it on appveyor (after it had already worked locally of course). When the code style check (which is run before the first integration test is performed) adds 18 to 22 minutes to the build time on Linux, you'll get value-adding feedback much less frequently, and the turnaround time decreases dramatically. On to issue ##1109, where @ktoim asked whether it would be "possible to get a version of .net6.0 on the AppVeyor feed"? You (@bart-degreed ) responded:
I responded and told you about the fact that
I then proposed a change in which we run those two functions only on Windows, where they are MUCH FASTER. This means we are saving 18 to 22 minutes while still running those code-style checks. We would just fail a little later (in ca. 24 minutes rather than 18 to 22 minutes) in case of a code-style issue. If you see the cibuild as the last check, code style issues should actually be an exception. Failing fast on those code-style checks would not be the highest. You would mainly want to avoid code style issues. However, I still thought about how we could reach both objectives, i.e., reduce build times and fail fast on code-style issues. The result is the appveyor.yml I shared above. It runs the code style checks on a Windows image first and only in that first job. While this is a little slower than the solution option that I've called "Run on Windows Only and Possibly Fail Just a Little Later While Creating a Much Better Developer Experience", you might like it better if you still wanted to fail fast(er) on code style issues. Since you have not answered my question, let me ask it again: Would you be fine with the first solution that I've called "Run on Windows Only and Possibly Fail Just a Little Later While Creating a Much Better Developer Experience"? This option is in line with your most recent comment. |
|
My latest response was based on new insights. I've come to realize that we should focus our efforts on improving the local development experience, instead of adapting the cibuild to optimize the workflow for individuals. Tomorrow someone may ask us to run the tests first because they don't have docker running locally. Or to build the documentation website first, because it takes so long to build it locally. This is solving the wrong problem, so I don't think it's a good idea to change the cibuild for such purposes. In the exceptional case that someone is debugging the cibuild itself (such as installing the Cosmos Emulator for Linux), I already mentioned that turning off steps temporarily in a PR to get faster feedback is fine, so that's not the discussion here. The cibuild partly validates that the code complies with our principles and quality norms, which typically vary between organizations, teams, projects, and products. The sole fact that "it works" is not good enough, as pointed out in our guidelines:
So instead of changing the cibuild, I'd like to focus our efforts on improving the local tools. Things we can do:
|
|
@ThomasBarnekow I've spent some time trying to improve cleanupcode for local usage as described earlier, and in the process created sethreno/ReGitLint#15 and sethreno/ReGitLint#16. As for inspectcode, apart from a "copy" switch, I thought of something else that may help. While in Visual Studio, you can import the file WarningSeverities.DotSettings as an extra layer, then run Resharper > Inspect > Code Issues in Solution. This applies the same rules as the command-line runner, but faster. Note, however, that the list also includes spelling errors, which are not reported by the command-line running, so they can be ignored. Do you think the proposed changes are going to improve the local development flow for you? |
|
@bart-degreed, sorry for the very late response. I did not see this. I think the answer is "yes". |
Context
This issue is linked to a side discussion in #1109 on the very long build times. Here are some bits and pieces of the discussion in #1109, starting with my comment on a statement made by @bart-degreed:
@bart-degreed responded as follows:
Problem
The two steps in question take 18 to 22 minutes on Linux and 5 to 7 minutes on Windows, i.e., ca. 23 to 29 minutes in total!
The following example highlights how frustrating this long time can be. The build started for my commit e65face related to PR #1103 led to a formatting-related failure after
22 min 8 sec. Here are relevant parts of the output:The situation was that the relevant part of
appsettings.jsonlooked like this:However, it was supposed to look like that:
In this case, the error was reported by
RunCleanupCode(which is relatively fast) rather thanRunInspectCode(which is super-slow at least on Linux).Solution Options
To kick this off, I'd see multiple solutions.
Run on Windows Only and Possibly Fail Just a Little Later While Creating a Much Better Developer Experience
In PR #1103, I changed the relevant part of
Build.psas follows to only run the two checks on Windows:The Linux build took
16 min 13 secwhile the Windows build took14 min 21 sec. Roughly8 min 20 secinto the Windows build,RunInspectCodewas done.9 min 30 secinto the build,RunCleanupCodefinished. Thus, it would have taken approximately24 min 30 secinstead of22 min 8 secto fail the build, e.g., in case of the above formatting issue. However, this way would have led to a much better developer experience. The build on one platform would have confirmed some more material facts ("does it work?") while also pointing out issues that are relatively less material ("does the code look consistent?"). In that case, I would have been more than happy to also fix that formatting issue. However, having to wait 22 minutes to only learn that a JSON file was not laid out as expected was super-frustrating.Consider Changing the Order of the Images
You said you need to run Linux before Windows, because the Windows build publishes the NuGet package and the documentation website while also wanting to fail fast on formatting issues.
Looking at appveyor.yml, my question would be whether this must run on Windows. If it were possible to publish the NuGet package and the documentation website on Linux, we could:
RunInspectCodeandRunCleanupCodesteps, where those two steps only take 5 to 7 minutes in total as opposed to 18 to 22 minutes on Linux; andRunInspectCodeandRunCleanupCodesteps but with the NuGet package and documentation website publishing steps.We would fail much faster on formatting issues, i.e., after 5 to 7 rather than 18 to 22 minutes, while at the same time reducing the total build time by 18 to 22 minutes (as we also would in the first solution option).
The text was updated successfully, but these errors were encountered: