🔧 Run GitHub Actions CI on Linux with CLR4 coverage#1686
Conversation
Review: CI/build modernization + netstandard2.0/net48 test coverageThis PR is primarily CI/build infrastructure (no new quantities/units, no changes to generated quantity code). Summary below. Breaking changesNone for library consumers. All changes are internal test/CI/build tooling. The CI/workflow changes
Code quality / generated codeThe only generator change is in - var units = Enum.GetValues<{_unitEnumName}>();
+ var units = EnumHelper.GetValues<{_unitEnumName}>();This is needed because The same substitution is applied by hand in Potential bug / correctness (worth calling out explicitly)
Test coverage
Other
Overall: a large but low-risk, well-scoped infra PR. The one substantive behavior change ( Reviewed by Claude |
bfdb66b to
cb9689b
Compare
angularsen
changed the title
Review: #1686 — Cross-platform GitHub Actions CIThis PR migrates CI from Azure Pipelines to GitHub Actions (Linux runners), adds a scheduled .NET Framework 4.8 compatibility workflow, and adds netstandard2.0 asset test coverage. No unit/quantity definitions are touched. Overall solid and well-scoped; a few items below. Breaking changesNone for library consumers — this is CI/build/test infrastructure only. The public API surface is unchanged. The Generated code changes
Potential bug: Azure Artifacts push likely fails silently on missing authIn - name: Configure Azure Artifacts
if: env.AZURE_DEVOPS_PAT != ''
uses: actions/setup-dotnet@v6
with:
source-url: https://pkgs.dev.azure.com/...
env:
NUGET_AUTH_TOKEN: ${{ secrets.AZURE_DEVOPS_PAT }}
- name: Push to Azure Artifacts
if: env.AZURE_DEVOPS_PAT != ''
run: dotnet nuget push "**/*.nupkg" --skip-duplicate --api-key AzureArtifacts
working-directory: nugets
Style/conventions
Bug fix bundled into this PR
Test coverageThe new Minor
No security concerns at medium+ severity, and no new quantities/units in this PR so |
ReviewReviewed the CI/build migration to GitHub Actions plus the incidental code changes it carries along. Overall this is a well-scoped, carefully executed infra PR — no new quantities/units, and the generated-code diff is mechanical and consistent everywhere. Breaking changesNone for library consumers. All changes are to CI, build scripts, test infrastructure, and test-only code. The one production source change ( Possible doc/workflow mismatchThe PR description says the net48 workflow will "run the complete suite weekly (and on demand) on Windows/CLR4", but on:
push:
branches:
- master
workflow_dispatch:There's no Changes to generated code
Incidental bug fix bundled in a CI PR
return Math.Pow(number, 1.0 / n);to a version that special-cases negative Good, non-obvious fixes worth highlighting
Style/consistency nit
Test coverageNice addition: Action version pins
Nothing here blocks merging — mostly a couple of things to double-check (the missing cron trigger, action version pins) and two spots where non-CI behavior changes ride along and would benefit from a one-line callout in the PR description or a code comment. |
Review: #1686 — Cross-platform GitHub Actions CIThis is a CI/infra migration PR (158 files, mostly workflows + mechanical generated-test changes). Here's my review against the usual checklist. Breaking changesNone to the public API. One behavior-relevant fix worth calling out: New quantities/unitsNone added — not applicable here. Generated codeOnly one generator line changed, in
Code quality / style
Potential bugs / things to double-check
SecurityNet positive:
Test coverageGood — the Overall this looks like a well-executed, low-risk infra migration with one nice incidental bug fix. Nothing blocking. |
Review: 🔧ci: Cross-platform GitHub Actions CIThis is a pure CI/build-infra PR — no new quantities/units, no public API surface changes. Overall solid work; the netstandard2.0-asset testing is a genuine improvement that already found (and fixed) a real bug. A few things worth a look before merging. Highlights
Questions / possible issues
Test coverageThe new Style/conventionsNo violations of AGENTS.md conventions spotted — no unit/quantity JSON changes, generated code changes trace back to a single generator source change as expected, file headers preserved on new files. No security concerns beyond the general secrets-handling note above (all secret usage looks conventional — env-var indirection into 🤖 Generated with Claude Code |
551886b to
5f0521f
Compare
Review: #1686 — Cross-platform GitHub Actions CIThis is a CI/build-infrastructure PR with no changes to unit definitions or public API surface. Breaking changesNone. No New quantities or unitsNone added. Generated code changesOnly one generator template line changed, in - var units = Enum.GetValues<{_unitEnumName}>();
+ var units = EnumHelper.GetValues<{_unitEnumName}>();This is mechanically applied to every
Code quality / best practices
Test coverage
Potential issues / nits
Overall this is a well-scoped, carefully validated infra migration — the author reports both Linux |
5f0521f to
42fc5c3
Compare
Review: #1686 — Cross-platform GitHub Actions CIThis is an infra-only PR (no new quantities/units, no changes to the shipped public API), so I focused on the CI/build changes, the generated-code template change, and a couple of incidental fixes bundled in. Breaking changesNone for consumers of the NuGet packages. All production changes are either CI/build tooling or Changes to generated code
Code quality / good practices
Minor suggestions (non-blocking)
Bug fix bundled in (worth calling out)
Test coverageNot reduced. Moving the full SecurityNo concerns at or above medium severity. The removal of the manual GPG/SHA256 verification script in favor of the official Codecov action + OIDC is a net improvement, not a regression. Overall: a well-structured, security-conscious CI migration. Nothing blocking; the suggestions above are all optional polish. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1686 +/- ##
=======================================
- Coverage 96% 96% -1%
=======================================
Files 454 456 +2
Lines 29497 30107 +610
=======================================
+ Hits 28459 29043 +584
- Misses 1038 1064 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review: CI migration to GitHub Actions (#1686)This is a CI/build infrastructure PR — no new quantities/units, and only one line of generated-code-generator change, so I'll focus feedback there. Breaking changesNone for library consumers. This only touches build/CI infra, test projects, and one internal test-generator template. No changes to shipped Generated code / code generator changes
Other correctness fixes bundled inA couple of real fixes travel with the CI migration commit:
CI/build workflow changes
Test coverage
Nits
Overall this is a well-scoped, well-tested infra migration with good validation notes in the PR description (test counts on both Linux and Windows net48). No concerns blocking merge from my side. |
|
Correction to my review above: the "Nits" section had a stray typo ("treatureatment") — meant to say the Benchmark csproj's Windows-only |
Review: 🔧ci: Cross-platform GitHub Actions CIThis is primarily a CI/build infrastructure PR (153 files, but ~128 are a single mechanical line change repeated across generated test files). Overall it's a well-scoped, well-documented migration with good validation evidence in the PR description. Breaking changesNone to the shipped library's public API. Not a new quantity/unit PRNo changes under Changes to generated code
Bug fixes surfaced by the net48 target
CI/workflow review
Test coverage
Nits
Overall: solid, low-risk migration with good self-review evidence in the PR description (actual CI run links, local validation, |
Review: Cross-platform GitHub Actions CIThis is a CI/tooling migration PR — no new quantities/units, and no changes to the production quantity/unit generators. 156 files changed, but ~130 of those are a single mechanical line in generated test files, so the actual reviewable surface is small. Nice work overall; a few notes below. Breaking changesNone to the public API. One behavior-relevant fix: Generated code changesThe only generator touched is Code quality
CI/workflow changes
Performance
Test coverageNo new functional tests added (expected for a CI-focused PR), but the SummarySolid, well-scoped infra PR. The two things I'd actually want addressed before merge are minor: the |
Review: 🔧ci: Cross-platform GitHub Actions CIThis is a CI/build-infrastructure PR (no Breaking changesNone to the public API. There is one behavioral fix in Generated codeOnly one generator template changed: Bugs surfaced by this change (good catches)Enabling real test execution on net48 (previously untested) surfaced two latent bugs, both fixed correctly:
This is exactly the value of actually running the net48 suite rather than just compiling for it — nice validation that the new workflow works. Minor nits
SecurityGood improvements here, nothing new to flag:
Other observations
Overall: well-scoped, mechanical, and the incidental bug fixes are a nice bonus from actually exercising the net48 path. No blocking issues found. |
ReviewThis is a CI/build infrastructure migration (Azure Pipelines → GitHub Actions) with no new quantities or units. Overall it's a well-executed, carefully-scoped migration — nice attention to security and cross-platform detail. A few notes: Breaking changesNone for library consumers. Changes to generated code
Code quality / correctness fixes bundled inA few real bug fixes ride along with the CI migration, since they were required to get the net48 suite green:
These are reasonable to bundle since they're purely enablers for the new CI target, not unrelated scope creep. CI/workflow specifics
Build scripts
Test coverageReasonable trade-off: full suite + coverage only runs on No security concerns beyond the OIDC/secret-handling improvements noted above (which are net positives). |
Review: CI/PR — Cross-platform GitHub Actions CI (#1686)This is a CI/infra migration (no changes under Breaking changesNone to the public API surface. The one behavior change is a bug fix in - return Math.Pow(number, 1.0 / n);
+ bool isNegative = BitConverter.DoubleToInt64Bits(number) < 0;
+ if (isNegative && n % 2 == 0) return double.NaN;
+ double root = Math.Pow(Math.Abs(number), 1.0 / n);
+ return isNegative ? -root : root;Previously New quantities/unitsNone — no JSON unit definitions changed, so nothing to evaluate against Generated codeOnly one generator template changed, - var units = Enum.GetValues<{_unitEnumName}>();
+ var units = EnumHelper.GetValues<{_unitEnumName}>();This accounts for the bulk of the diff (~130 Relatedly, Potential issue: doc/workflow mismatch
on:
push:
branches: [master]
pull_request:
branches: [master]
workflow_dispatch:It does trigger on PRs (and the PR description itself says it runs "for pull requests, every push to master, and manual runs"). Worth fixing the doc, or dropping Code quality / best practices
Test coverageThe SecurityNo medium+ severity concerns. The move to Codecov OIDC ( Nice cleanup overall: the Azure Pipelines files become inert stubs ( |
ReviewThis is a substantial but low-risk PR: it's almost entirely CI/build infrastructure, plus a handful of test/library fixes that were needed to make the Breaking changesNone to the public Generated code / code generator
Other non-CI fixes bundled inA few small fixes surfaced by actually running the full suite on
These all look correct and are small enough to be fine bundled with the CI work, but a one-line callout in the PR description ("also fixes a few net48-only compile issues surfaced by the new compatibility workflow") would help future readers understand why a "CI" PR touches production/test logic. Minor style/maintainability note
CI/workflow design
One inconsistency worth a look: Test coverageMain Overall this looks well thought through — the validation section in the PR description is thorough, the security-sensitive parts (OIDC, workflow_run split) are handled correctly, and the incidental fixes are small and traceable. Nice work. |
angularsen
changed the title
Review: #1686 – Cross-platform GitHub Actions CISolid infra migration overall — Linux-based CI, OIDC-based Codecov auth instead of a stored token, SHA-pinned third-party actions in the write-privileged Breaking changesNone for consumers. This is CI/build-only, plus one behavioral fix in product code (see below). No JSON unit definitions were touched, so no new/changed quantities or units to review against Possible doc/config mismatch
on:
push:
branches: [master]
pull_request:
branches: [master]But
|
|
Review: PR #1686 — Cross-platform GitHub Actions CI This is a CI/build infrastructure migration with no changes to the public quantity/unit API surface. No new quantities or units are introduced. Breaking changes Code generation changes Other bundled non-CI fixes
These are legitimate and necessary for the stated goal, just worth flagging since they're functional (not CI-config) changes inside a PR titled as CI work. CI/workflow design
Test coverage Security Overall: solid, well-reasoned CI migration. The main thing I'd want called out more prominently is the |
angularsen
changed the title
Review: Run GitHub Actions CI on Linux with CLR4 coverageThis is primarily a CI/build infrastructure migration, with a few small but real bug fixes surfaced by the new .NET Framework 4.8 (CLR4) coverage. No new quantities/units are added, so that part of the review checklist doesn't apply. Breaking changesNone for library consumers. One thing worth flagging for anyone extending QuantityInfo IQuantity.QuantityInfo { get { return Info; } }to IQuantityInstanceInfo<HowMuch> IQuantityOfType<HowMuch>.QuantityInfo => Info;to compile under net48. This is just test/sample code, but it suggests any third-party custom CI/CD changes (the bulk of the diff)
Bug fixes surfaced by CLR4 testing
Test coverage
Other
|
ReviewThis is primarily a CI/build-infrastructure migration (Windows Azure Pipelines/Windows GH Actions → Linux GH Actions + a dedicated Windows CLR4/net48 compatibility workflow), plus a handful of compatibility fixes it surfaced. No new quantities or units, and no public API changes — reviewed against Breaking changesNone to the public library API. The one behavior change is in Bug fix:
|
be20de5 to
4f5bcfe
Compare
ReviewThis is a CI/infrastructure migration PR (Azure Pipelines → GitHub Actions on Linux, plus a new Windows-based CLR4/.NET Framework 4.8 compatibility workflow). No unit definitions ( Breaking changesNone for library consumers.
Changes to generated codeThe only generated-code change is mechanical: Other compatibility fixes (non-generated)
CI/workflow design
Test coverageCoverage collection was intentionally narrowed to Nit
Overall this is a well-scoped, carefully-reasoned CI migration with good security hygiene around the fork-PR/ |
4f5bcfe to
6caa1d5
Compare
ReviewThis is a CI/build-infrastructure PR (GitHub Actions migration + CLR4/.NET 4.8 compatibility testing) with no new quantities/units and no functional generator changes beyond a one-line template fix. Overall solid work — the fork-PR security pattern in particular is done correctly. Breaking changesNone to the public API. Internally, the test projects ( New quantities/unitsNone introduced. Generated code changesOnly one generator template line changed, in - var units = Enum.GetValues<{_unitEnumName}>();
+ var units = EnumHelper.GetValues<{_unitEnumName}>();This propagates identically to every generated Code quality / bugs
CI/workflow design
Test coverageNo scope creep — new/changed tests are exactly what's needed to validate the CLR4 fixes (ParamName assertions, the SecurityNo medium+ severity issues. The one workflow with elevated ( |
6caa1d5 to
08facd0
Compare
Review: #1686 — Run GitHub Actions CI on Linux with CLR4 coverageThis is primarily a CI/infra migration (158 files, but ~150 are one mechanical line each in generated test files). Overall this is a well-scoped, carefully documented change. Notes below. Breaking changes
Generated code changesThe only generator change is in Verified this is applied identically and mechanically across quantity kinds with no incidental changes:
All three (and the ~150 other regenerated files) contain the exact same single-line diff, so this is low-risk. Code quality / correctness
Security
Minor / non-blocking
Test coverageGood — the Nothing else stood out. Nice, thorough writeup in the PR description. |
27c2e78 to
f011d80
Compare
Review: 🔧 Run GitHub Actions CI on Linux with CLR4 coverageReviewed the diff against 🐛 Likely bug: Linux build will fail due to a hardcoded Windows path separator
function Start-Build {
write-host -foreground blue "Start-Build (dotnet CLI)...`n---"
# Use dotnet CLI for all main projects - cross-platform compatible
dotnet build --configuration Release /p:ContinuousIntegrationBuild=true "$root\UnitsNet.slnx"
...Every other path in this same file was migrated to Since Positive notes on the workflow design
Minor / non-blocking
Generated codeThe only generated-code changes are in the test generator ( Test coverageGood: the new |
Review: GitHub Actions CI migrationSolid infra PR — no changes to the public API, quantity/unit definitions, or code generators (the Breaking changesNone for consumers. The only functional code change is a hand-written test-only quantity ( Security — nice pattern, one suggestion
CI/test design
Test-only code changes
Test coverageThe net48/CLR4 workflow is a valuable addition — it already surfaced a real Overall: clean, well-reasoned CI migration with good security hygiene. The action-pinning consistency point is the only thing I'd nit on before merging. |
f011d80 to
d1b95cd
Compare
|
Review: CI migration to GitHub Actions with CLR4 coverage This PR is CI/build infrastructure only — no JSON unit definitions, generators, or generated code ( Breaking changes New quantities or units Changes to generated code Code quality / correctness
Security
Performance / CI cost
Test coverage
Style / conventions
Overall this is a well-structured, security-conscious CI migration with good documentation in the PR description and inline comments. The one thing I would want addressed or explicitly acknowledged before merge is the loss of net8.0/net9.0 test execution in CI (build-only now for those TFMs). |
d1b95cd to
db3ae18
Compare
Motivation: Modern test TFMs exercise the same code paths, while CLR4 provides distinct compatibility coverage. Changes: Target net10 on every platform and automatically add net48 on Windows.
Motivation: Collecting coverage for equivalent modern TFMs duplicates work. Changes: Run modern tests and coverage on net10; leave netstandard coverage to the CLR4 workflow.
Motivation: Companion NuGet packages should display the project documentation. Changes: Pack the repository README with the NumberExtensions and JsonNet packages.
Motivation: The v6 build has no Windows-only dependency and can use Linux as its primary runner. Changes: Move CI and PR builds to Linux, modernize actions, use Codecov OIDC, and protect master publishing.
Motivation: Linux net10 tests do not execute the shipped netstandard2.0 assets on CLR4. Changes: Run the full net48 suite with coverage on Windows for PRs, master pushes, and manual runs.
Motivation: Fork PRs cannot safely grant checks:write to workflows executing contributor code. Changes: Publish uploaded CLR4 artifacts through a restricted workflow_run workflow with pinned actions.
Motivation: Azure and GitHub duplicated v6 validation and publishing, while maintenance/v5 still needs Azure. Changes: Disable Azure triggers on master and retain branch-specific v5 pipeline support.
Motivation: The README still referenced Azure CI and a temporary migration guide was obsolete. Changes: Document Linux and CLR4 GitHub Actions, update badges, and remove the migration guide.
db3ae18 to
b430a7f
Compare
Review: CI migration to GitHub Actions + CLR4 coverageThis PR is infrastructure-only (17 files, all workflows/build scripts/csproj/README) — no changes under PR description vs. diff mismatchThe description lists source-level "Compatibility fixes surfaced by CLR4 testing" ( Nice security pattern
Minor: net48 testing duplicated/removed from local dev loop
Not blocking, but a comment in Workflow logic
csproj changesConditionally including Overall: solid, well-documented migration. The main thing worth resolving before merge is the stale-looking "compatibility fixes" section in the description, and optionally a note tying together the two net48 test-project lists. |
Review: CI/build infrastructure migration (Linux + CLR4 coverage)This PR is exclusively CI/build tooling — no Breaking changesNone to the public API. The only behavior change for contributors: local PR description vs. diffThe description's "Compatibility fixes surfaced by CLR4 testing" section ( Code quality / best practices
Test coverageNet new: SecurityNothing at medium+ severity. The Overall this is a well-thought-out, well-documented CI migration — the reasoning comments in the workflow files (concurrency, permission splits, |
|
Split into multiple commits and pushed directly to master instead. |
1 participant
Summary
netstandard2.0,net8.0,net9.0, andnet10.0) on Linux, while running the full modern test suite and coverage onnet10.0.master, and manual runs. The complete suite executes onnet48against the shippednetstandard2.0assets and contributes separately flagged Codecov coverage.workflow_runworkflow with narrowly scoped write permission.masterwhile preserving the branch-specific Azure/nanoFramework pipeline onmaintenance/v5.Why CLR4 testing matters
The library ships
netstandard2.0assets, but the modern Linux suite executes thenet10.0assets. Running the test projects on .NET Framework 4.8 exercises thosenetstandard2.0binaries on CLR4, including different conditional compilation paths and runtime behavior. Introducing this explicit coverage has already surfaced compatibility defects that the modern suite did not detect.Compatibility fixes surfaced by CLR4 testing
EnumHelper.GetValues<T>()in tests that must compile on .NET Framework 4.8.NETRootNfallback so odd roots of negative values return a real negative result instead ofNaN.ArgumentNullException.ParamNameinstead of runtime-specific exception messages.Validation
./Build/build.ps1passed locally: all shipped targets compiled, 46,257 tests passed onnet10.0, 20 skipped, and four coverage reports were generated.netstandard2.0UnitsNet assembly.actionlint;git diff --checkpasses.Deployment notes
master.maintenance/v5.workflow_runcan first execute after its workflow exists on the default branch.