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

Update build tooling to dotnet8; tests to xUnit #122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

borland
Copy link
Contributor

@borland borland commented Nov 5, 2024

Background

I have some upcoming planned enhancements to ShellFish to improve the usability of the API, and add support for async.

Before embarking on these changes, I would like to set a decent baseline by upgrading the tooling and third party dependencies

.NET 6 goes end of life very soon, and all Octopus repositories need to be updated to a supported .NET version, so we want to get that out of the way too.

Results

  • Updates ShellFish to build with the .NET 8 SDK and C# 12 language version

    • Note, it still targets netstandard2.0 so is usable by other code all the way back to .NET 4.x
  • Updates the nuke build script to use nuke 8.1.3, which is the current version, appropriate for .NET 8

  • Updates tests to run against net8 and net472

  • Updates test framework to use xUnit (it is our preferred standard now)

  • Updates other dependencies to the latest versions (System.DirectoryServices.AccountManagement, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes to this file, as well as the build.cmd/ps1/sh were entirely performed by nuke tooling, not me. It owns them.

dir.DeleteDirectory();
}
ArtifactsDirectory.CreateOrCleanDirectory();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The static EnsureCleanDirectory and DeleteDirectory methods were deprecated, these are the replacements.

@@ -1,23 +1,72 @@
{
"$schema": "http://json-schema.org/draft-04/schema#",

Choose a reason for hiding this comment

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

I assume this is auto generated in some way?

@@ -4,4 +4,4 @@
:; exit $?

@ECHO OFF
powershell -ExecutionPolicy ByPass -NoProfile "%~dp0build.ps1" %*
powershell -ExecutionPolicy ByPass -NoProfile -File "%~dp0build.ps1" %*

Choose a reason for hiding this comment

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

nit: why did we add -File

@@ -18,11 +18,10 @@ $TempDirectory = "$PSScriptRoot\\.nuke\temp"

$DotNetGlobalFile = "$PSScriptRoot\\global.json"
$DotNetInstallUrl = "https://dot.net/v1/dotnet-install.ps1"
$DotNetChannel = "Current"
$DotNetChannel = "STS"

Choose a reason for hiding this comment

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

This i assume is standard nuke stuff

[Test]
[Retry(3)]
[Fact]
// [Retry(3)] TODO retry with polly or something
public void CancellationToken_ShouldForceKillTheProcess()

Choose a reason for hiding this comment

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

I wonder if we will see it be flaky, in any-case Retry(3) is a workaround to some real underlying problem. I for one would rather deal with that problem over hide it in tests.

@@ -5,28 +5,31 @@
<RootNamespace>Tests</RootNamespace>
<Nullable>enable</Nullable>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<LangVersion>8</LangVersion>
<LangVersion>latest</LangVersion>
Copy link

@LukeButters LukeButters Nov 6, 2024

Choose a reason for hiding this comment

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

Does this mean if latest changes to a version that is not compatible with the code in our tests, our test build will break? Can actual version latest points to change because of something outside of our control?

I am a sucker for not using relative terms in build files, and instead being explicit about the build e.g. depending on a particular version rather than the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants