C#: Pin integration tests to a specific .NET version. #14878
Conversation
michaelnebel
force-pushed
the
csharp/pindotnetinintegrationtests
branch
from
77ad874
to
3d34bb5
Compare
michaelnebel
added
the
no-change-note-required
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "sdk": { | |||
| "version": "7.0.102" | |||
There was a problem hiding this comment.
Should we add "rollForward": "latestFeature", this would mean that any newer 7.0 variant would be accepted too.
There was a problem hiding this comment.
I would prefer that we concretely pin the version number and not even accept another version with the same feature band to avoid switching to a newer version, when it is installed on the runner. At least for .NET 7 we have around 5 different versions installed and I am uncertain that it is the same across all platforms - so I will prefer that it is completely under our control when a newer version is used even with the same feature band.
There was a problem hiding this comment.
I'm okay with this decision as I don't fully understand the problem that we're solving. Is it that previously the latest 7.0.x version of the SDK was used, while now it's the latest 8.0.x version? (Unless we somehow force the usage of a 7.0.x variant, which this PR is doing.)
I think version 7.0.102 is the version that we're installing to the runners. The default installed variants seem to depend on the OS image. So if we were to upgrade our jobs to use 7.0.404, then we would have to update all the tests too, wouldn't we?
There was a problem hiding this comment.
Answering for @michaelnebel, but I think the problem we are solving with this is the general case of what you are describing. When the GHA images get updated to a new .NET version, but we do not yet support that .NET version, then this may cause widespread test failures throughout not just the main branch, but also rc branches etc. (as we have seen with the recent .NET 8 release). Pinning the version guarantees that there's a match between the version that the tests are designed for in a given branch and the version of .NET that is used there.
I think your point about not necessarily having the right version installed, depending on the runner image, is a valid one. Perhaps we should add setup-dotnet steps with the same version, just to be sure.
For Go, we do allow the patch version to differ (i.e. we have it pinned to 1.21.x right now for any x). I think it is probably OK to similarly pin .NET to 7.0.x since we wouldn't expect patch-level releases to introduce any breaking changes.
There was a problem hiding this comment.
@tamasvajk : Yes, our CI has used the most recent installed SDK (which meant 7.0.404) until 8.0.100 was installed - then this was used by default.
The reason why I picked 7.0.102 is because that is the one we are installing on the runners as a part of setup-dotnet - so to make it consistent I picked that (I could also have forced the installation of the newer version). The .NET version has just changed gradually over time during the last year as newer versions of .NET became available on the runners.
Also, I don't think we should be to concerned whether we pick 7.0.102 or 7.0.404 for this change as the version number will change again as part of the .NET 8 update PR(s) and be pinned to a .NET 8 versions.
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
Can't we put a single global.json file to csharp/ql/integration-tests/?
The docs say:
The .NET SDK looks for a global.json file in the current working directory (which isn't necessarily the same as the project directory) or one of its parent directories.
There was a problem hiding this comment.
Seems like that would be worth trying to see if it works as expected. That would mean we'd only need to update the version in one place since we can point setup-dotnet at that file as well (see https://github.com/actions/setup-dotnet#using-the-global-json-file-input) if we choose to add an explicit step for that.
There was a problem hiding this comment.
I think our workflow generator already adds such a step: https://github.com/github/semmle-code/actions/runs/6967032126/job/18958211361#step:20:1
There was a problem hiding this comment.
I will try and add it directly to the integration test directory and see if it works (there might be some issues with relying on the upwards search as the integration tests are copied before executed and I don't know what happens to the content of the parent folder (the . There are some folders where a different .NET version is hardcoded (eg. for msbuild). Will keep it on test level.integration-tests directory itself))
Will also take a look at the workflow generation script and see.
There was a problem hiding this comment.
We will not be able to use a global.json file for all workflow descriptions. Should we put in an effort to use the global.json files in some cases? If we should do so it will be a PR in semmle-code and not anything we can do as a part of this PR.
There was a problem hiding this comment.
We will not be able to use a global.json file for all workflow descriptions.
What's the reason for that? A limitation of the workflow generator or something else?
michaelnebel
force-pushed
the
csharp/pindotnetinintegrationtests
branch
from
3d34bb5
to
d1c4e77
Compare
This pins most of the integration tests to a specific version of .NET (not the
msbuildas this for some reason fails on win2019, when pinned to this specific .NET version - but I will try and pin this when doing the .NET upgrade).The motivation for doing this are the issues we saw when the runners we updated to also include a newer version of .NET, which caused integration test failures in main and the release branch - we would like to avoid this in the future.
We should also consider to pin the extractor to a specific version of .NET.
Please note that it is not enough to just install a specific dotnet version using the dotnet setup action.
This is from the documentation: