Release M263 #1872
Merged
Release M263 #1872
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Git simply has so strong an opinion about line endings that it is not worth fighting. Git will passive-aggressively color the Carriage Return in the diff output until you give up, and that's what I'm doing here. Der Klügere gibt nach. Technically, this is far from the only file that has this issue. But I do not want to interfere with other ongoing work in the CI area, so I'll refrain from touching files other than the one I am about to modify. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Yes, yes, CodeQL. It is totally _possible_ that someone with admin privileges will change the default back to the unsafe `write` permission. Even if unlikely, let's make it explicit that the `build` workflow only requires `read` permission. Even in a PR that is only modifying that workflow, and whose purpose is a totally different one. Let's conflate separations of concerns. Whatever. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Most of the failures we encounter in VFSforGit's CI runs are flakes. In those instances, it is not helpful but rather annoying when long-running jobs are canceled after more than half an hour when they could have run to completion and succeeded instead. Because when those runs fail, the first thing I do is to hit "Re-run" to see whether it _is_ a flake. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
GitHub Actions sadly lacks the feature where you can reuse prior runs that were successful on the same `head_sha`. Similar to what I did in git/git@99fe06cbfd6 (ci: avoid building from the same commit in parallel, 2023-08-23), let's simply skip the CI run if the same commit has been tested successfully before. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As far as VFSforGit's CI is concerned, there is no difference between commits when their _files_ have not changed (it's not like the tip commit's SHA is persisted in some version number or some such). Therefore, if there is already a successful run for another commit whose top-level tree is identical to the one that is about to be tested, skip the current run in favor of that one. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To increase confidence in Pull Requests, a couple of Checks are required, for example the PR build must pass. However, due to one of GitHub Actions' limitations that seem to stay with us forever, it is impossible to require a specific _workflow_ to succeed for a PR before it can be merged. You have to specify workflow _jobs_ that are required. Therefore, the changes I just made to skip runs when there already exists a successful run for the same commit (or at least a tree-same one), would eternally prevent affected PRs from being merged. While the _workflow_ succeeds in such instances, the (required) workflow _jobs_ would be skipped (and therefore the required checks would not be fulfilled). Let's bite the bullet and patch through the information whether to skip re-running the tests _to the individual jobs_, so that they do run, and the do get marked as successful, and the Pull Requests can proceed. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It is always nice when one is given a good reason for something. For example, when a CI run is skipped, it is nice to not only to know that there _has_ been a previous successful run, but also _which one_. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The Functional Test jobs takes long enough (35 minutes for x86_64, 1 hour 15 minutes for ARM64) that it is worth waiting for them to finish before kicking off a matrix build that tests the exact same code. So let's do that, wait for any in-progress run that might result in some saved time. Granted, this spends cycles of GitHub runners waiting idly, but GitHub does not provide for a better way. Note: The `concurrency` attribute _might_ look as helpful in this context at first sight. But is comically limited: It can only let a _single_ job wait in a queue, subsequent jobs get canceled (and if you have notifications turned on, in a loud manner!). And even letting the job wait would not help here because we also want to handle _tree-same_ commits, not only identical commits, and GitHub does not provide for a way to identify them in GitHub workflow expressions, either. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
There might be runs for the very same commit, those should be looked at first. Then there might be runs that have not yet finished; Let's first look at the finished ones. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In `workflow_dispatch`-triggered runs, the user can specify a Git version to use for testing, which means that something different is actually tested than in regular, non-`workflow_dispatch` runs. So let's neither skip the build/test when triggered thusly, nor reuse any results from thusly-triggered runs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…jobs ci: avoid unnecessary builds and test runs Sadly, VFSforGit's CI and PR builds are not exactly cheap. Not only in terms of combined runtime (where VFSforGit's 3 hours 45 minutes is not even close runner-up to [`git/git`'s 8.5 hours (!!!)](https://github.com/git/git/actions/runs/17628515153/usage). A Functional Test job typically runs for 35 minutes to test on x86_64, and an even worse 1 hour and 15 minutes on arm64. The wall-clock time for a CI or PR build of VFSforGit therefore weighs around 1 hour and 20 minutes. This hurts a lot when trying to fast-track a change from a PR targeting `master` to a full VFSforGit release because not only does the entire build run _again_ on the (tree-same) `master` after merging, the next step (thanks to the original VFSforGit maintainer's decision to go for a Git Flow-like setup) requires the build to run _a third-time_ on the (also tree-same) PR merge commit. Let's try to reuse build and test results from previous, successful runs on what is essentially the same code.
This teaches a new command-line option to `GVFS.FunctionalTests.exe` that allows running only a slice of all tests at a time, via `--slice=<m>,<n>` where `m` is the zero-based index of the slice to run, and `n` is the total number of slices to split the tests into. The idea is to distribute the test cases evenly across the slices, so that running all slices in parallel will finish in about the same time as running all tests in a single slice. This is slightly tricky because the test cases of classes within the `EnlistmentPerFixture` namespace are known to rely on side effects of previous test cases within the same class. Therefore, all test cases of such a class must be run in the same slice. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In #1866 I introduced logic to skip builds and tests when a previous workflow run already succeeded for the same commit. However, the original revision of that Pull Request tried something _even more_ elaborate, and when I dropped that elaborate logic (because it is a bit fragile), I made a mistake in the replacement logic. The `run.status` can never be `success`, it can be `completed` and _`run.conclusion`_ can be `success`. So let's test for that instead ;-) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Following the example of git/git, which runs the Windows tests in parallel to compensate for a very long run time, we split the Functional Tests into 10 buckets and run them in parallel. This is particularly useful in light of some flaky tests that frequently need to be re-run. That way, the cost of re-running a failed test is reduced by virtue of only having to re-run a _slice_ of the tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
ci: fix skipping logic
In the GitHub UI, it is possible to specify Checks that need to be successful before a PR can be merged. These "Checks" are actually the names of jobs in the workflow. It would be much better to specify a required _workflow_, but only workflow _jobs_ can be specified. The Required Checks are currently: - Functional Tests (Debug, arm64) - Functional Tests (Debug, x86_64) - Functional Tests (Release, arm64) - Functional Tests (Release, x86_64) So let's make sure that there are job names that match the recorded names and that indicate a successful workflow run. An alternative would be to add a single job with a defined name that depends on all the other jobs, and that can compensate for the missing GitHub feature to require a whole workflow instead of individual jobs to have run successfully. But that would interfere with other PRs that are currently in flight, and will therefore have to wait its turn in a separate PR. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
GitHub has a feature called "Require status checks before merging": https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging What they actually mean by "status checks" is "successful workflow job runs". And what they _really_ actually mean is "_display labels_ of successful workflow job runs". That is, workflow job runs are identified by the label they display. Example: In VFSforGit, there is currently a "required status check" that reads "Functional Test (Debug, arm64)". This is the label that is displayed for the matrix job with vector {configuration: Debug, architecture: arm64} in the "functional_test" workflow job. This is quite restrictive! In VFSforGit, specifying those four matrix job labels is a _work-around_ for the _actual_ requirement, namely that the workflow defined in `build.yaml` succeeds. It just so happens that those four matrix jobs are the leaf jobs, i.e. when they all succeed, the workflow _run_ has succeeded. And vice versa, if the workflow run failed, at least one of those four matrix jobs must have failed or not even run. Now that I multiplied the matrix jobs even further by running the Functional Tests in parallel, the _display labels_ (and the number) of the matrix jobs has changed. As a consequence, to appease the "Require status checks before merging" rule, I had to add _another_ set of matrix jobs just to guarantee that the same four matrix job labels exist. This is silly, because those four matrix jobs are not needed at all for actually testing the code. They are just there to make GitHub happy. This commit prepares to change that. It adds a new workflow job that fits the bill "if the job succeeded, the workflow run must have succeeded as a hole, and vice versa". This new job will be made the only "required status check", once this here PR has been merged, and then we can remove the silly "duplicate" matrix jobs again. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Run the Functional Tests in parallel From time to time, the Functional Tests fail, and re-running "fixes" the failures, i.e. those tests are flaky. When that happens, we have to invest a lot of time into the re-run: currently 35 minutes for the x86_64 jobs and 1 hour 15 minutes for the ARM64 ones. That's unwelcome friction. Let's follow the playbook of git/git which runs its test suite (which, due to the heavy over-use of Bash scripting, is very, very slow on Windows) in parallel on Windows, slicing up the tests so that the individual test slices take roughly the same amount of time to run. In practice, this reduces the wall-clock time of the `build` workflow runs from around 1 hour 15 minutes to around 20 minutes, at the expense of increasing the total time of all jobs from about 3 hours 35 minutes to about 5 hours 15 minutes.
Since .NET SDK 8, the Informational Version attribute includes the commit ID of the current HEAD at build time. The product code relies on this value to be System.Version-compliant, so we should opt out of this new behaviour. https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Directory.Build.props: don't include commit ID in version
It is not _any_ version that starts with `0.` that is a development version, instead it is the version hard-coded in `Version.props` when no `GVFSVersion` has been specified. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In .NET SDK 8, the Informational Version attribute of the assembly includes the commit ID of the current HEAD at build time: https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link This kind of version might be compliant with SemVer v2.0, but it is _not_ System.Version-compliant. Therefore, we opted out of this new behavior _really quickly_ after upgrading from SDK 3 to SDK 9, in order to fix cloning from repositories where `gvfs/config` requires certain minimal VFSforGit versions. In this here commit, we adapt the logic to strip off the commit ID if present. That way, the version check would continue to work even if we opted back in to this new behavior that includes the commit SHA in `gvfs version`'s output. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…when-revision-is-included Fix `gvfs clone` version check when revision is included in the version string As reported by Tyrie Vella, `gvfs clone` would fail with https://github.com/microsoft/VFSForGit/releases/tag/v1.0.25260.1 when the repository to clone specifies a minimal VFSforGit version in its `gvfs/config` endpoint. The reason is that with the version bump from .NET SDK v3 to v9, the version string included in the assembly now contains the suffix `+<commit-SHA>`, which cannot be parsed by `System.Version`: ``` Cannot clone @ C:\path: System.FormatException: Input string was not in a correct format. at System.Version.VersionResult.SetFailure(ParseFailureKind failure, String argument) at System.Version.TryParseComponent(String component, String componentName, VersionResult& result, Int32& parsedComponent) at System.Version.TryParseVersion(String version, VersionResult& result) at System.Version.Parse(String input) at System.Version..ctor(String version) at GVFS.CommandLine.GVFSVerb.TryValidateGVFSVersion(GVFSEnlistment enlistment, ITracer tracer, ServerGVFSConfig config, String& errorMessage, Boolean& errorIsFatal) at GVFS.CommandLine.GVFSVerb.ValidateClientVersions(ITracer tracer, GVFSEnlistment enlistment, ServerGVFSConfig gvfsConfig, Boolean showWarnings) at GVFS.CommandLine.CloneVerb.Execute() ``` Matthew Cheetham provided a quick work-around in #1870 to opt out of the new behavior and report the version _without_ that suffix again; Here is a PR that prepares the code to handle such versions gracefully, to open up the door to opt back in to the new behavior (and let `gvfs version` report an even more informative output than right now).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
gvfs cloneversion check when revision is included in the version string #1871)