Modernize build infrastructure, CI and analyzer compliance - #7
Merged
Merged
Conversation
Build - target net10.0, net8.0 and netstandard2.0 (drop EOL net6.0) - add Directory.Build.props and Directory.Packages.props (central package management) - treat compiler warnings as errors, enable analyzers (CA* stay warnings for now) - replace mkver with MinVer (tag prefix "v"), drop SourceLink package (built into the SDK) - migrate CoreLib.sln to CoreLib.slnx, add global.json and .editorconfig - update test stack (xunit 2.9.3, Test.Sdk 18, coverlet) CI - build and test on Linux, Windows and macOS, pack on every change - tests that need the internet are marked Category=Network and run non-blocking - publish via NuGet trusted publishing, falling back to the API key secret - verify that the package version matches the release tag - add dependabot for NuGet and GitHub Actions Fixes - resolve macOS release names for macOS 14, 15 and 26 and fall back to the product name - fix wrong expectation in LoremIpsumGeneratorTest that DefaultTempUtil.UseFile swallowed, which also made TestLog flaky through the process-wide log event - make TestLog collect only its own events - fix xunit analyzer findings in MaskedTextFormatterTest and DownloaderTest
Enable CodeAnalysisTreatWarningsAsErrors. All 131 CA findings in Core are either fixed or explicitly accepted; nothing about the public API changes. Fixed - culture: use ordinal comparisons and ToLowerInvariant; format and parse machine-readable values (URLs, hex, HTTP headers, OS versions) with the invariant culture - HttpHeader parsed Date/Last-Modified with the current culture, which fails for e.g. ar-SA - GeoLocation threw ArgumentException instead of ArgumentNullException for longitudeDMS - pass parameter names and messages to ArgumentException/ArgumentOutOfRangeException - seal internal types, call GC.SuppressFinalize in Dispose, use static readonly separator arrays - AssertionPredicate uses TextWriter.Null instead of an undisposed StreamWriter - narrow private field and parameter types (CA1859) - add the Polyfill source generator for netstandard2.0 (no runtime dependency) Accepted (Core/.editorconfig, Core/Extensions/SecurityRelated/HashAlgorithmExt.cs) - rules that can only be fixed by breaking the public API are downgraded to suggestions and revisited for 13.0: naming (CA1711, CA1716, CA1720, CA1725), public fields (CA1051, CA2211), static members (CA1000, CA1822), CA2201 - MD5/SHA1 (CA5350, CA5351) are suppressed locally until they are deprecated in 13.0 - CA1307 and CA1510 are off: the affected overloads are ordinal by default, and ArgumentNullException.ThrowIfNull does not exist on netstandard2.0 Tests: add CultureIndependenceTest (de-DE, tr-TR, sv-SE, ar-SA).
DefaultServiceUrlTest asserted that every service returns the same IPv4 address. That fails on dual-stack hosts (a service answers with IPv6) and behind NAT pools such as GitHub-hosted runners, where consecutive requests leave via different addresses (seen on macos-latest). Each service must now answer with a valid IP address (IPv4 or IPv6); Resolve() is still expected to return an IPv4 address.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Phases 0-3 of the modernization plan. The public API is unchanged (verified with package validation against the published
Cap.Core 12.0.0).Build (213c4dd)
net10.0;net8.0;netstandard2.0(drops the EOLnet6.0); tests run onnet10.0andnet8.0Directory.Build.props/Directory.Packages.props(central package management),global.json,.editorconfigCoreLib.sln->CoreLib.slnxmkverreplaced by MinVer (tag prefixv); the SourceLink package is dropped (built into the SDK)CI (213c4dd)
Category=Networkand run non-blockingNUGET_API_KEYsecret, and a check that the package version matches the release tagAnalyzer compliance (a30e4bf)
CA*findings are fixed or explicitly accepted; analyzer findings now fail the buildCore/.editorconfigfor 13.0; MD5/SHA1 are suppressed locally until deprecationHttpHeaderparsed dates with the current culture (fails e.g. forar-SA),GeoLocationthrew the wrong exception type for a nulllongitudeDMS, the macOS name mapping ended at macOS 13, and a wrong expectation inLoremIpsumGeneratorTestwas swallowed byDefaultTempUtil.UseFile(which also madeTestLogflaky)CultureIndependenceTestTest plan
dotnet buildon the whole solution: 0 warnings, 0 errorsnet10.0andnet8.0(local, macOS), 25 repeated runs without a failuredotnet packproducesnet10.0,net8.0andnetstandard2.0assetsCap.Core 12.0.0: no removed types or members (onlyISpanFormattablenoise from dropping thenet6.0asset)Capjan, repocapjan/CoreLib, workflownuget_deploy.yml); until then the release workflow uses the API key secretNotes
PublicIpResolverTest.DefaultServiceUrlTeststill fails on hosts that answer with an IPv6 address (it asserts an IPv4 regex). It carries theNetworktrait, so CI treats it as informational.HttpWebRequest, sync-over-async, MD5/SHA1 deprecation, async overloads).