Refactor build scripts and github workflow#1308
Conversation
WalkthroughCI runners and .NET SDK pins updated; Cake-specific script names replaced by unified Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Pinned Runner
participant Script as Build/Package Script
note right of Runner `#eef6ff`: Runners pinned (ubuntu-22.04 / macos-13 / windows-2022)
rect rgb(250,253,255)
GH->>Runner: checkout + setup dotnet (e.g., 7.0.410 / 3.1.426)
Runner->>Script: run `./build.sh` / `./build.cmd` or `package.cmd` / `package-push.cmd`
Script->>Runner: dotnet tool restore -> invoke Cake targets or direct steps
Runner-->>GH: report status (success / failure)
end
sequenceDiagram
autonumber
participant Cake as Cake task
participant Helper as ILRepack helper (MergeDLLs)
participant FS as File system / DLLs
note right of Helper `#fff7e6`: Consolidated merge flow (primary + additional DLLs)
rect rgb(255,252,240)
Cake->>Helper: call MergeDLLs(primaryDll, dllsToMerge, outputDll)
Helper->>FS: validate primaryDll exists and dlls provided
Helper->>FS: build input list and ensure output dir
Helper->>Helper: construct ILRepack args (/lib path, keyfile, inputs, output)
Helper->>FS: execute ILRepack once with args
alt success
Helper-->>Cake: return merged output path
else failure
Helper-->>Cake: throw error / fail task
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package-push.cmd (2)
10-10: Minor: Usage example could be clearer.Consider adding quotes around the version number in the usage example for consistency with how it's used in line 13:
package-push.cmd "1.2.8"or document that quotes are optional.
1-14: Migrate packaging to the new build wrappersThe
package-push.cmdscript still callsdotnet cake cake-build/build.cakefor thePushPackagetarget, but neitherbuild.cmdnorbuild.shsupport that target. Update the wrappers to include aPushPackageoption or refactorpackage-push.cmdto invoke the unified build scripts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/package.yml(4 hunks).github/workflows/run-tests-linux.yml(1 hunks).github/workflows/run-tests-macos-mono.yml(2 hunks).github/workflows/run-tests-macos.yml(1 hunks).github/workflows/run-tests-windows-netframework.yml(1 hunks).github/workflows/run-tests-windows.yml(1 hunks)cake-build/README.md(4 hunks)package-cake.sh(0 hunks)package-push.cmd(1 hunks)package-push.sh(1 hunks)package-unity.sh(1 hunks)package.cmd(1 hunks)
💤 Files with no reviewable changes (1)
- package-cake.sh
🧰 Additional context used
🪛 LanguageTool
cake-build/README.md
[grammar] ~101-~101: There might be a mistake here.
Context: ... ### 1. package.cmd - Responsible for creating core ably.io NuGet package. - Works o...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...r creating core ably.io NuGet package. - Works only on Windows due to a dependenc...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...d creates ably.io.1.2.3.nupkg package at root. During release process, this pac...
(QB_NEW_EN)
[style] ~116-~116: To make your writing clearer, consider a more direct alternative.
Context: ...h packages for Android and iOS. Please take a look at [Push Notification Documentatio...
(TAKE_A_LOOK)
🔇 Additional comments (11)
.github/workflows/run-tests-windows.yml (2)
40-40: Verify integration test target compatibility.Similarly, ensure that
build.cmdsupports theTest.NetStandard.Integration.WithRetrytarget with the--frameworkparameter.
37-37: No action required: build.cmd wrapper supports test targets
build.cmd exists at the repo root and delegates to Cake; the Test.NetStandard.Unit.WithRetry target is defined in cake-build/tasks/test.cake..github/workflows/run-tests-windows-netframework.yml (2)
29-32: Verify NetFramework test targets in build.cmd.Ensure that
build.cmdsupports the NetFramework-specific test targets:Test.NetFramework.Unit.WithRetryandTest.NetFramework.Integration.WithRetry.
23-23: .NET SDK 7.0.410 is the latest patch and includes all security fixes (CVE-2024-21409, CVE-2024-30045, etc.)..github/workflows/run-tests-macos.yml (1)
37-40: LGTM! Consistent with Linux workflow.The migration from
build-cake.shtobuild.shis consistent with the Linux workflow changes. The target names and framework matrix remain unchanged, ensuring compatibility..github/workflows/run-tests-linux.yml (1)
37-40: LGTM! Script migration is consistent.The changes correctly update the test execution to use
build.shinstead ofbuild-cake.sh, maintaining the same target names and framework parameters.package-push.sh (1)
10-10: LGTM! Usage message correctly updated.The usage message now correctly references
package-push.shinstead of the oldpackage-push-cake.shscript name.package-unity.sh (1)
7-7: LGTM! Usage message correctly updated.The usage message now correctly references
package-unity.shinstead of the oldpackage-unity-cake.shscript name, consistent with the naming convention updates across the PR..github/workflows/run-tests-macos-mono.yml (2)
23-24: Confirm .NET SDK versions availability and security fixes
Both .NET SDK 3.1.426 (3.1.32) and 7.0.410 (7.0.20) are available and include patches for CVE-2022-41089, CVE-2024-20672 (and earlier 7.0 fixes such as CVE-2023-36792). Review the official Microsoft release notes and security advisories for full details.
34-37: build.sh and Cake tasks are correctly configured for the macOS targets.build.shis present, executable, and forwards--targettodotnet cake, and bothTest.NetFramework.Unit.WithRetryandTest.NetFramework.Integration.WithRetrytasks are defined incake-build/tasks/test.cake..github/workflows/package.yml (1)
34-38: .NET SDK 7.0.410 is publishedVerified that SDK version 7.0.410 appears in the official 7.0 channel releases metadata.
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.devcontainer/devcontainer.json(1 hunks).github/workflows/package.yml(6 hunks).github/workflows/run-tests-linux.yml(2 hunks).github/workflows/run-tests-macos-mono.yml(3 hunks).github/workflows/run-tests-macos.yml(2 hunks).github/workflows/run-tests-windows-netframework.yml(2 hunks).github/workflows/run-tests-windows.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/run-tests-windows-netframework.yml
- .github/workflows/run-tests-windows.yml
- .github/workflows/package.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
sacOO7
force-pushed
the
fix/github-workflow
branch
from
4b6e643 to
794fcbf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/run-tests-macos-mono.yml (1)
10-24: .NET 3.1 cannot be installed on macos-13 (arm64) runners
macos-13GitHub runners are Apple Silicon (arm64). Whenactions/setup-dotnet@v4tries to provision3.1.426, it requests anosx-arm64build that was never produced—.NET 3.1 only ships asosx-x64. The step will therefore fail with “No asset found for osx-arm64” and all Mono tests break. Please either keep using an Intel-based runner (macos-12), or pin the setup step toarchitecture: x64and ensure Rosetta is available before installing. Otherwise the workflow won’t succeed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.devcontainer/devcontainer.json(1 hunks).github/workflows/package.yml(6 hunks).github/workflows/run-tests-linux.yml(2 hunks).github/workflows/run-tests-macos-mono.yml(3 hunks).github/workflows/run-tests-macos.yml(2 hunks).github/workflows/run-tests-windows-netframework.yml(2 hunks).github/workflows/run-tests-windows.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/run-tests-macos.yml
- .github/workflows/run-tests-windows.yml
- .github/workflows/run-tests-linux.yml
- .github/workflows/package.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
- GitHub Check: check
sacOO7
force-pushed
the
fix/github-workflow
branch
from
794fcbf to
45687a0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.devcontainer/devcontainer.json (1)
4-7: Invalid onCreateCommand type — must be string or string arrayThis object form is rejected by the devcontainer spec; commands won’t run. Convert to an array.
Apply:
- "onCreateCommand": { - "submodules": "git submodule update --init --recursive", - "install-dependencies": "bash .devcontainer/install-dependencies.sh" - }, + "onCreateCommand": [ + "git submodule update --init --recursive", + "bash .devcontainer/install-dependencies.sh" + ],
🧹 Nitpick comments (6)
.github/workflows/run-tests-macos.yml (2)
10-10: Runner pin to macos-13Pinning is OK for stability, but macos-13 may be retired sooner. Consider macos-14 for longevity and M1/M2 perf.
19-31: Speed up with NuGet cacheAdd actions/cache for ~/.nuget/packages to cut restore time across matrix builds.
Example:
- name: Download dotnet build-script tools run: dotnet tool restore + + - name: Cache NuGet packages + uses: actions/cache@v4 + with: + path: ~/.nuget/packages + key: nuget-${{ runner.os }}-${{ hashFiles('**/*.sln', '**/*.csproj', '**/global.json', '**/nuget.config') }} + restore-keys: | + nuget-${{ runner.os }}-.github/workflows/run-tests-macos-mono.yml (2)
23-24: .NET 3.1 is EOL — validate necessity or remove3.1.426 is end-of-life and downloads may be unstable. If not strictly required, drop it; otherwise pinning is OK but please confirm tests actually need it.
10-10: Runner pin to macos-13Same note as the other workflow: consider macos-14 for longevity/Apple Silicon.
.devcontainer/install-dependencies.sh (2)
1-11: Harden script and minimize installs
- Add set -euo pipefail.
- Use DEBIAN_FRONTEND=noninteractive and --no-install-recommends to slim image.
Example:
-#!/bin/bash -set -e +#!/bin/bash +set -euo pipefail @@ -sudo apt-get install -y gnupg ca-certificates wget +export DEBIAN_FRONTEND=noninteractive +sudo apt-get install -y --no-install-recommends gnupg ca-certificates wget
28-33: Consider aligning with targeted TFMsDevcontainer base has .NET 8 SDK; workflows test net9. If local dev needs net9, install dotnet-sdk-9.0 or add runtime 8.0/9.0 as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.devcontainer/devcontainer.json(1 hunks).devcontainer/install-dependencies.sh(1 hunks).github/workflows/package.yml(6 hunks).github/workflows/run-tests-linux.yml(2 hunks).github/workflows/run-tests-macos-mono.yml(3 hunks).github/workflows/run-tests-macos.yml(2 hunks).github/workflows/run-tests-windows-netframework.yml(2 hunks).github/workflows/run-tests-windows.yml(2 hunks).gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/run-tests-windows-netframework.yml
- .github/workflows/package.yml
- .github/workflows/run-tests-windows.yml
- .github/workflows/run-tests-linux.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
🔇 Additional comments (4)
.github/workflows/run-tests-macos.yml (2)
40-40: Same as above for Integration target via build.shValidate the Integration target exists in build.sh as per the verification script above.
37-37: Build infrastructure verified—changes are correct.build.sh exists, is executable, and both
Test.NetStandard.Unit.WithRetryandTest.NetStandard.Integration.WithRetrytargets are properly defined incake-build/tasks/test.cakeand documented. The workflow changes are ready..github/workflows/run-tests-macos-mono.yml (1)
34-37: Confirm build.sh targets for .NET Framework (Mono)Ensure build.sh defines Test.NetFramework.Unit.WithRetry and Test.NetFramework.Integration.WithRetry targets.
Use the same script from the macOS workflow comment (adjust the regex to NetFramework).
.devcontainer/devcontainer.json (1)
41-45: NuGet cache path and mount are consistent — LGTMNUGET_PACKAGES matches the mounted volume path; good for caching across rebuilds.
sacOO7
force-pushed
the
fix/github-workflow
branch
from
45687a0 to
3d3c07d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.devcontainer/install-dependencies.sh (1)
13-14: Mono apt repository configuration is invalid on Debian 12.
apt-key advis deprecated and thestable-busterchannel mismatches the bookworm base image, soapt-get updatewill fail or skip the Mono repo. Switch to the bookworm feed and install the key into/usr/share/keyrings, e.g.:-# Add Mono repository -sudo apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys 3FA7E0328081BFF6A14DA29AA6A19B38D3D831EF -echo "deb https://download.mono-project.com/repo/debian stable-buster main" | sudo tee /etc/apt/sources.list.d/mono-official-stable.list +# Add Mono repository (bookworm) with keyring +wget -qO- https://download.mono-project.com/repo/xamarin.gpg | sudo gpg --dearmor -o /usr/share/keyrings/mono-official.gpg +echo "deb [signed-by=/usr/share/keyrings/mono-official.gpg] https://download.mono-project.com/repo/debian stable-bookworm main" | sudo tee /etc/apt/sources.list.d/mono-official-stable.list.devcontainer/devcontainer.json (1)
4-7:onCreateCommandmust be a string or array, not an object.As written the lifecycle commands won’t run, so submodules and dependency setup never happen. Collapse into an array:
- "onCreateCommand": { - "submodules": "git submodule update --init --recursive", - "install-dependencies": "bash .devcontainer/install-dependencies.sh" - }, + "onCreateCommand": [ + "git submodule update --init --recursive", + "bash .devcontainer/install-dependencies.sh" + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.devcontainer/devcontainer.json(1 hunks).devcontainer/install-dependencies.sh(1 hunks).github/workflows/package.yml(6 hunks).github/workflows/run-tests-linux.yml(2 hunks).github/workflows/run-tests-macos-mono.yml(3 hunks).github/workflows/run-tests-macos.yml(2 hunks).github/workflows/run-tests-windows-netframework.yml(2 hunks).github/workflows/run-tests-windows.yml(2 hunks).gitignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/run-tests-windows-netframework.yml
- .github/workflows/run-tests-macos.yml
- .github/workflows/run-tests-windows.yml
- .github/workflows/package.yml
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
sacOO7
force-pushed
the
fix/github-workflow
branch
from
3d3c07d to
4075ece
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
4075ece to
5ada145
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
5ada145 to
08d27aa
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
08d27aa to
82b64ec
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
82b64ec to
5bfb646
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
4f47cd4 to
d3227ad
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
sacOO7
force-pushed
the
fix/github-workflow
branch
from
d3227ad to
518f365
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.devcontainer/install-dependencies.sh (1)
45-54: Add .NET 9 SDK to match “latest” targets (SDK also brings runtime)If the repo targets net9.0, you need the 9.0 SDK inside the devcontainer; currently only 6/7 runtimes are installed.
Proposed change:
# Install .NET 6.0 runtime echo "🔧 Installing .NET 6.0 runtime..." sudo apt-get install -y dotnet-runtime-6.0 echo "✓ .NET 6.0 runtime installed" # Install .NET 7.0 runtime echo "🔧 Installing .NET 7.0 runtime..." sudo apt-get install -y dotnet-runtime-7.0 echo "✓ .NET 7.0 runtime installed" + +# Install .NET 9.0 SDK (includes 9.0 runtime) +echo "🔧 Installing .NET 9.0 SDK..." +sudo apt-get update -qq +sudo apt-get install -y dotnet-sdk-9.0 +echo "✓ .NET 9.0 SDK installed"Verify TFMs in the repo:
#!/bin/bash # Lists any project targeting net9.0 rg -nP -g '!**/bin/**' -g '!**/obj/**' '(?i)<TargetFrameworks?>\s*[^<]*net9\.0' -C2
🧹 Nitpick comments (13)
.github/workflows/run-tests-windows.yml (4)
10-10: Pinned runner: good; add a deprecation watch.windows-2022 pin is fine for stability. Consider tracking runner-image deprecations to avoid surprise breaks or add a periodic update task.
11-12: Disable telemetry in CI.Add DOTNET_CLI_TELEMETRY_OPTOUT to reduce noise and tiny perf hit.
env: DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: true
25-31: Enable NuGet cache for faster runs.actions/setup-dotnet@v4 supports caching. If you have lock files, enable it.
Example:
- uses: actions/setup-dotnet@v4 with: dotnet-version: | 6.0.428 7.0.410 8.0.404 9.0.100 cache: true # Optionally narrow cache with lock files if present: # cache-dependency-path: | # **/packages.lock.json # **/global.json
37-40: Use Windows path for .cmd and confirm flags support.PowerShell usually runs .cmd fine, but .\build.cmd is safer. Also confirm build.cmd accepts --target and --framework like the old script.
- run: ./build.cmd --target=Test.NetStandard.Unit.WithRetry --framework=${{ matrix.targetframework }} + run: .\build.cmd --target=Test.NetStandard.Unit.WithRetry --framework=${{ matrix.targetframework }} ... - run: ./build.cmd --target=Test.NetStandard.Integration.WithRetry --framework=${{ matrix.targetframework }} + run: .\build.cmd --target=Test.NetStandard.Integration.WithRetry --framework=${{ matrix.targetframework }}.github/workflows/run-tests-linux.yml (3)
27-31: Add NuGet caching and review EOL SDKs
- Enable NuGet cache to speed up CI.
- .NET 6 and 7 SDKs are EOL; if you still support TFMs net6.0/net7.0, keep them, otherwise drop to reduce surface.
Proposed change:
- name: Download dotnet framework uses: actions/setup-dotnet@v4 with: dotnet-version: | 6.0.428 7.0.410 8.0.404 9.0.100 + cache: true + cache-dependency-path: | + **/packages.lock.json + **/Directory.Packages.propsIf you plan to deprecate net6.0/net7.0, confirm against your published support matrix and adjust the matrix and SDK list accordingly.
8-16: Harden the job: timeout, minimal permissions, and telemetry opt-out
- Add job timeout to prevent hangs.
- Use least-privilege GITHUB_TOKEN.
- Opt-out of CLI telemetry.
jobs: check: - runs-on: ubuntu-22.04 + runs-on: ubuntu-22.04 + timeout-minutes: 60 env: DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: 1And at the workflow root (top-level):
name: "Unit and Integration Test: Linux" on: pull_request: push: branches: - main +permissions: + contents: read + +concurrency: + group: run-tests-linux-${{ github.ref }} + cancel-in-progress: true
10-10: Pinned runner is fine; consider 24.04 when feasibleubuntu-22.04 is stable and supported for .NET 9. When convenient, plan migration to ubuntu-24.04 to extend the support window for the runner image.
.devcontainer/install-dependencies.sh (3)
2-2: Harden shell execution flagsUse nounset and pipefail for safer provisioning.
-set -e +set -euo pipefail +# Avoid interactive prompts in apt +export DEBIAN_FRONTEND=noninteractive
39-43: Trim image bloat for apt installsInstall without recommendations to reduce size and speed up installs.
-sudo apt-get install -y mono-complete nuget +sudo apt-get install -y --no-install-recommends mono-complete nuget
1-3: Make provisioning idempotent; avoid re-running on every attachThis script runs on every attach. Add a sentinel to skip repeated apt work.
#!/bin/bash -set -e +set -euo pipefail +export DEBIAN_FRONTEND=noninteractive + +# Fast‑exit if already provisioned +if [ -f /usr/local/.ably-devcontainer-deps-installed ]; then + echo "✅ Dependencies already installed. Skipping." + exit 0 +fi @@ echo "🎉 Setup complete! You can now build the project." echo "==========================================" + +# Mark as provisioned +sudo touch /usr/local/.ably-devcontainer-deps-installedAlso applies to: 57-78
.devcontainer/devcontainer.json (3)
5-5: Run provisioning once on create, not on every attachSwitch to postCreateCommand to avoid repeated apt installs on each VS Code attach.
- "postAttachCommand": "bash .devcontainer/install-dependencies.sh", + "postCreateCommand": "bash .devcontainer/install-dependencies.sh",
38-38: Prefer local dotnet tools first in PATHPrepend .dotnet/tools so devcontainer respects user tools/shims before system ones.
- "PATH": "${containerEnv:PATH}:${containerWorkspaceFolder}/.dotnet/tools" + "PATH": "${containerWorkspaceFolder}/.dotnet/tools:${containerEnv:PATH}"
3-3: Align base image with target SDKsIf targeting net9.0, consider a 9.0 SDK image (when available) to reduce extra apt steps, or keep 8.0 and install SDK 9.0 as you propose in the script.
Example:
- "image": "mcr.microsoft.com/devcontainers/dotnet:1-8.0-jammy", + "image": "mcr.microsoft.com/devcontainers/dotnet:1-9.0-jammy",Confirm target frameworks first (see the script in my other comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.devcontainer/devcontainer.json(1 hunks).devcontainer/install-dependencies.sh(1 hunks).github/workflows/package.yml(6 hunks).github/workflows/run-tests-linux.yml(2 hunks).github/workflows/run-tests-macos-mono.yml(3 hunks).github/workflows/run-tests-macos.yml(2 hunks).github/workflows/run-tests-windows-netframework.yml(2 hunks).github/workflows/run-tests-windows.yml(2 hunks).gitignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/run-tests-windows-netframework.yml
- .github/workflows/run-tests-macos-mono.yml
- .github/workflows/run-tests-macos.yml
- .github/workflows/package.yml
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check
- GitHub Check: check (net8.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
🔇 Additional comments (2)
.github/workflows/run-tests-windows.yml (1)
17-17: Verify testing net7.0 (EOL) is intentional.net7.0 is out of support. Keep it if you still target it; otherwise drop to reduce matrix time.
.github/workflows/run-tests-linux.yml (1)
37-40: build.sh executable bit and targets confirmed—no issues foundScript verification confirms:
- build.sh is executable (mode 100755 in git) ✓
- Both targets
Test.NetStandard.Unit.WithRetryandTest.NetStandard.Integration.WithRetryexist incake-build/tasks/test.cake(lines 195, 203) ✓The workflow changes are sound.
sacOO7
force-pushed
the
fix/github-workflow
branch
from
518f365 to
c3f180b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
…endencies - Updated github workflows with specific ubuntu, macOS and windows versions
sacOO7
force-pushed
the
fix/github-workflow
branch
from
c3f180b to
54dbc62
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nuget/io.ably.nuspec (1)
35-35: Upgrade Newtonsoft.Json from 9.0.1 to 13.0.1 or later to fix CVE-2024-21907.Version 9.0.1 is affected by a known denial-of-service vulnerability (CVE-2024-21907) that can be triggered by deeply nested/malicious JSON causing StackOverflow or resource exhaustion. Upgrade to version 13.0.1 or later to resolve this.
This applies to all three instances in the file (lines 35, 39, 43).
🧹 Nitpick comments (2)
nuget/io.ably.nuspec (1)
37-44: Consider adding net8.0 target framework.The PR objectives mention updating to "latest dotnet versions," but the package only targets up to net7.0. .NET 8.0 is the current LTS release and should be included for modern compatibility.
nuget/io.ably.push.ios.nuspec (1)
23-23: Update Xamarin.Essentials from 1.6.1 to 1.8.1.The latest stable Xamarin.Essentials version is 1.8.1, while the package currently pins version 1.6.1 (released in 2021). Version 1.8.1 contains no breaking changes and includes improvements relevant to iOS targets. Update lines 23 and 27 in
nuget/io.ably.push.ios.nuspecfromversion="1.6.1"toversion="1.8.1".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
nuget/io.ably.nuspec(2 hunks)nuget/io.ably.push.android.nuspec(1 hunks)nuget/io.ably.push.ios.nuspec(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
🔇 Additional comments (7)
nuget/io.ably.nuspec (2)
7-18: LGTM! Metadata modernization is excellent.The updates align with current NuGet best practices: license expressions, embedded icons/readme, repository metadata, and enhanced discoverability through tags.
55-56: All referenced files exist; no issues found.Verification confirms that both
README.mdandimages/NETSDK-github.pngexist at their specified paths and are correctly referenced in the package manifest.nuget/io.ably.push.ios.nuspec (2)
9-18: LGTM! Metadata improvements are consistent and comprehensive.The iOS push package metadata now matches the modern standards applied across all nuspec files.
33-34: No issues found. Both referenced files exist.Verification confirms that
README.mdandimages/NETSDK-github.pngare present at the paths specified in the nuspec file. The package references are valid and will not cause file resolution failures during package creation.nuget/io.ably.push.android.nuspec (3)
9-18: LGTM! Comprehensive metadata improvements.The Android push package now includes all modern NuGet metadata standards with appropriate platform-specific tagging (android, firebase, fcm).
35-36: Files referenced in the nuspec exist; no action required.Verification confirms both
README.mdandimages/NETSDK-github.pngexist at the specified paths. The package build will not fail due to missing files.
24-24: Xamarin.Essentials 1.6.1 is outdated; upgrade to 1.8.1 requires Android API 34 target.The latest stable version is 1.8.1, but upgrading requires significant changes: 1.8.1 adds Android 14 / API 34 compatibility, requiring TargetFrameworkVersion to be updated to MonoAndroid14.0 and android:targetSdkVersion to 34. Your Android project currently targets v11.0 (API 30). Verify whether this upgrade is desired and, if so, coordinate updates to both the nuspec dependency and the Android project configuration in
./src/IO.Ably.Push.Android/IO.Ably.Push.Android.csprojand related AndroidManifest files.
sacOO7
force-pushed
the
fix/github-workflow
branch
from
b79e40e to
19fd1bd
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nuget/io.ably.nuspec (1)
27-29: Use xamarinios10 TFM and lib\Xamarin.iOS10, not Xamarin.iOS. The dependency group TFM "Xamarin.iOS" and files target "lib\Xamarin.iOS" are non-canonical and may not resolve. Replace with xamarinios10 in dependencies and lib\Xamarin.iOS10 in files.Docs: supported TFM is xamarinios; packaging folder is Xamarin.iOS10. (learn.microsoft.com)
- <group targetFramework="Xamarin.iOS"> + <group targetFramework="xamarinios10"> <dependency id="System.Threading.Channels" version="4.6.0" /> </group> ... - <file src="..\src\IO.Ably.iOS\bin\$configuration$\Packaged\IO.Ably.*" target="lib\Xamarin.iOS" /> + <file src="..\src\IO.Ably.iOS\bin\$configuration$\Packaged\IO.Ably.*" target="lib\Xamarin.iOS10" />Also applies to: 50-51
♻️ Duplicate comments (2)
nuget/io.ably.push.android.nuspec (1)
21-30: Duplicate Android dependency groups; align with packaged assets. The groups for targetFramework="monoandroid9" and "monoandroid" are identical, but you only ship lib\monoandroid. Either remove monoandroid9 or add matching lib\monoandroid9 assets to avoid confusion.Preferred fix (consolidate to monoandroid):
<dependencies> - <group targetFramework="monoandroid9"> - <dependency id="ably.io" version="[1.2.7,2.0.0]" /> - <dependency id="Xamarin.Firebase.Messaging" version="122.0.0" /> - <dependency id="Xamarin.Essentials" version="1.6.1" /> - </group> <group targetFramework="monoandroid"> <dependency id="ably.io" version="[1.2.7,2.0.0]" /> - <dependency id="Xamarin.Firebase.Messaging" version="122.0.0" /> - <dependency id="Xamarin.Essentials" version="1.6.1" /> + <dependency id="Xamarin.Firebase.Messaging" version="122.0.0" /> + <dependency id="Xamarin.Essentials" version="1.6.1" /> </group> </dependencies>Alternatively, add lib\monoandroid9 assets if you truly multi-target. Based on learnings.
nuget/io.ably.push.ios.nuspec (1)
25-28: Remove invalid TFM group "Xamarin.iOS". NuGet’s iOS TFM is xamarinios (often versioned as xamarinios10). Keep the existing xamarinios10 group and delete the Xamarin.iOS group.Docs list xamarinios as the supported Xamarin iOS TFM. (learn.microsoft.com)
Apply:
<dependencies> <group targetFramework="xamarinios10"> <dependency id="ably.io" version="[1.2.7,2.0.0]" /> <dependency id="Xamarin.Essentials" version="1.6.1" /> </group> - <group targetFramework="Xamarin.iOS"> - <dependency id="ably.io" version="[1.2.7,2.0.0]" /> - <dependency id="Xamarin.Essentials" version="1.6.1" /> - </group> </dependencies>
🧹 Nitpick comments (5)
nuget/io.ably.push.android.nuspec (3)
23-23: Update Firebase Messaging to latest stable (125.0.0.1) or justify pin. Current pin 122.0.0 is outdated; 125.0.0.1 is the latest stable on nuget.org (updated August 16, 2025). Validate compatibility with your Xamarin/Android target.Recommended change:
-<dependency id="Xamarin.Firebase.Messaging" version="122.0.0" /> +<dependency id="Xamarin.Firebase.Messaging" version="125.0.0.1" />Refs: nuget.org package page shows 125.0.0.1 as latest. (nuget.org)
Also applies to: 28-28
24-24: Xamarin.Essentials 1.6.1 is old; 1.8.1 is latest (project archived). If you must stay on Xamarin, consider 1.8.1; otherwise prefer MAUI Essentials for .NET.Refs: Essentials archived May 2024; latest NuGet shows 1.8.1. (github.com)
Also applies to: 29-29
34-37: Packaging path check. You only ship lib\monoandroid; ensure dependency groups reflect this (see earlier comment) or add versioned lib folders to match any versioned TFMs you declare.Microsoft docs confirm lib subfolders must use framework names/versions. (learn.microsoft.com)
nuget/io.ably.nuspec (2)
37-45: Drop System.Threading.Channels for net6.0/net7.0. Channels are in the shared framework since .NET Core 3.0; external package is redundant on net6/7.Docs: “if you're using .NET Core 3.0 or later, the package is included as part of the framework.” (learn.microsoft.com)
<group targetFramework="net6.0"> - <dependency id="System.Threading.Channels" version="4.6.0" /> <dependency id="Newtonsoft.Json" version="9.0.1" /> </group> <group targetFramework="net7.0"> - <dependency id="System.Threading.Channels" version="4.6.0" /> <dependency id="Newtonsoft.Json" version="9.0.1" /> </group>
34-40: Consider updating Newtonsoft.Json to current stable (13.0.4). 9.0.1 is from 2016 and has advisories; latest stable is 13.0.4 (September 16, 2025). Verify compatibility across targets before bumping.Refs: nuget.org shows 13.0.4 latest, 9.0.1 flagged with vulnerabilities. (nuget.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
images/logo.pngis excluded by!**/*.pngtools/NuGet.exeis excluded by!**/*.exe
📒 Files selected for processing (3)
nuget/io.ably.nuspec(2 hunks)nuget/io.ably.push.android.nuspec(1 hunks)nuget/io.ably.push.ios.nuspec(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check
🔇 Additional comments (5)
nuget/io.ably.push.android.nuspec (1)
9-19: Metadata modernization looks good. License expression, icon/readme, tags, repository are correct.nuget/io.ably.push.ios.nuspec (2)
9-18: Metadata modernization looks good. License expression, icon/readme, tags, repository are correct.
32-34: Fix iOS lib folder to Xamarin.iOS10 to match TFM. Current target="lib\xamarinios" is not the canonical folder. Use lib\Xamarin.iOS10 when targeting xamarinios10.Official docs and guidance show lib\Xamarin.iOS10; lib\MonoAndroid10 for Android. (learn.microsoft.com)
-<file src="..\src\IO.Ably.Push.iOS\bin\Package\IO.Ably.Push.*" target="lib\xamarinios" /> +<file src="..\src\IO.Ably.Push.iOS\bin\Package\IO.Ably.Push.*" target="lib\Xamarin.iOS10" />Likely an incorrect or invalid review comment.
nuget/io.ably.nuspec (2)
7-18: Metadata modernization looks good. Authors/owners reorder, license expression, icon/readme, tags, repository — all fine.
48-55: Suggested iOS fix contradicts NuGet conventions. The review recommends changing tolib\Xamarin.iOS10, but NuGet canonical forms for Xamarin.iOS arelib\xamarinios10(versioned) orlib\xamarinios(generic)—not mixed-case variants. The currentlib\Xamarin.iOSis a historical but valid alias; if changing for consistency, adopt the lowercase canonical form. Local pack verification for NU5xxx warnings remains a valid check.Likely an incorrect or invalid review comment.
…priate description
sacOO7
force-pushed
the
fix/github-workflow
branch
from
19fd1bd to
7864444
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nuget/io.ably.nuspec (2)
27-29: Fix invalid Xamarin.iOS TFM and lib folder.NuGet doesn’t recognize targetFramework="Xamarin.iOS" or lib\Xamarin.iOS. Use xamarinios/xamarinios10 consistently.
Apply this diff:
- <group targetFramework="Xamarin.iOS"> + <group targetFramework="xamarinios10"> <dependency id="System.Threading.Channels" version="4.6.0" /> </group>- <file src="..\src\IO.Ably.iOS\bin\$configuration$\Packaged\IO.Ably.*" target="lib\Xamarin.iOS" /> + <file src="..\src\IO.Ably.iOS\bin\$configuration$\Packaged\IO.Ably.*" target="lib\xamarinios10" />Also applies to: 50-50
30-32: Remove deprecated MonoTouch support from nuspec file.MonoTouch (renamed to Xamarin.iOS) is legacy and out of support — Microsoft retired it on May 1, 2024. No active projects target this framework. Remove the monotouch dependency group at lines 30–32 and the corresponding file target at line 51 to reduce surface area.
nuget/io.ably.push.android.nuspec (1)
28-32: Update Xamarin.Essentials to 1.8.1; Xamarin.Firebase.Messaging is already current.
- Xamarin.Firebase.Messaging 122.0.0 is the latest stable version—no update needed.
- Xamarin.Essentials 1.8.1 is the latest stable version; update from 1.6.1.
♻️ Duplicate comments (1)
nuget/io.ably.push.android.nuspec (1)
28-34: Note: prior duplication resolved.Previously duplicated monoandroid and monoandroid9 groups were removed; thanks for consolidating.
🧹 Nitpick comments (2)
nuget/io.ably.push.ios.nuspec (1)
28-28: Align TFM group and lib folder naming (optional but clearer).You target xamarinios10 but ship to lib\xamarinios. Consider aligning to reduce ambiguity.
Apply either approach:
- <file src="..\src\IO.Ably.Push.iOS\bin\Package\IO.Ably.Push.*" target="lib\xamarinios" /> + <file src="..\src\IO.Ably.Push.iOS\bin\Package\IO.Ably.Push.*" target="lib\xamarinios10" />or change the group to unversioned:
- <group targetFramework="xamarinios10"> + <group targetFramework="xamarinios">nuget/io.ably.push.android.nuspec (1)
37-37: Align Android TFM group and lib folder (recommended).You declare monoandroid9 dependencies but ship assets to lib\monoandroid. Align to reduce confusion and ensure best match.
Apply:
- <file src="..\src\IO.Ably.Push.Android\bin\Package\IO.Ably.Push.*" target="lib\monoandroid" /> + <file src="..\src\IO.Ably.Push.Android\bin\Package\IO.Ably.Push.*" target="lib\monoandroid9" />Alternatively, change the dependency group to unversioned monoandroid to match the current lib folder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
images/logo.pngis excluded by!**/*.pngtools/NuGet.exeis excluded by!**/*.exe
📒 Files selected for processing (3)
nuget/io.ably.nuspec(2 hunks)nuget/io.ably.push.android.nuspec(2 hunks)nuget/io.ably.push.ios.nuspec(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
🔇 Additional comments (7)
nuget/io.ably.push.ios.nuspec (2)
9-19: Metadata modernization LGTM.License expression, icon/readme, tags, repo, and description updates look correct and NuGet‑friendly.
21-24: Update dependency versions to newer compatible releases.
- Xamarin.Essentials 1.8.1 supports Xamarin.iOS1.0 target framework, making it compatible with xamarinios10. Upgrading from 1.6.1 to 1.8.1 is technically feasible, but Xamarin.Essentials is no longer supported; migrate apps to .NET MAUI instead. Validate whether this package should remain on Xamarin or plan a migration path.
- The ably.io range [1.2.7, 2.0.0) is valid; version 1.2.16 is the latest and falls within it. No changes required unless a narrower range is preferred.
nuget/io.ably.nuspec (3)
7-19: Metadata updates look good.Authors/owners, license expression, icon/readme, tags, repo, and description are consistent with current NuGet guidance.
55-56: README/icon inclusion LGTM.Packaging README at root and icon.png via files mapping is correct.
37-44: Now let me verify if the codebase actually uses System.Threading.Channels:Verify dependencies for net6.0 and net7.0 targets.
Based on verification:
System.Threading.Channels has been included as part of the .NET framework since .NET Core 3.0, making version 4.6.0 redundant for both net6.0 and net7.0 targets. Removing this dependency from those specific framework groups is a valid optimization.
Newtonsoft.Json 9.0.1 was released in June 2016, while the latest version is 13.0.3. Extensive codebase usage of Newtonsoft.Json is confirmed (JsonConvert, JToken, custom converters throughout src/IO.Ably.Shared and tests). However, upgrading from 12.0.3 to 13.0.1 introduced breaking changes affecting deserialization with [JsonExtensionData], indicating testing is required before upgrading from 9.0.1 to 13.x. If compatible, upgrading is recommended for security and performance.
nuget/io.ably.push.android.nuspec (2)
9-19: Metadata modernization LGTM.License expression, icon/readme, tags, repo, and description are consistent and correct.
22-26: Core SDK dependency range is fine.Keeping ably.io as a global group with [1.2.7,2.0.0) matches the iOS push nuspec. No action.
If the core package minimum changes, remember to update both Android and iOS push nuspecs together.
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cake-build/tasks/package.cake (1)
117-117: Fix package name in logs (“io.ably”, not “ably.io”).Log lines invert the package ID and may confuse troubleshooting.
- Information($"✓ Package created: ably.io.{version}.nupkg"); + Information($"✓ Package created: io.ably.{version}.nupkg");- Information($"✓ Package created: ably.io.push.android.{version}.nupkg"); + Information($"✓ Package created: io.ably.push.android.{version}.nupkg");- Information($"✓ Package created: ably.io.push.ios.{version}.nupkg"); + Information($"✓ Package created: io.ably.push.ios.{version}.nupkg");Also applies to: 174-174, 186-186
🧹 Nitpick comments (2)
cake-build/helpers/tools.cake (1)
77-80: Set working directory to the bin folder for stable resolution.ILRepack can probe relative paths; set WorkingDirectory to the primary DLL’s directory to reduce surprises.
- var exitCode = _context.StartProcess(ilRepackPath.FullPath, new ProcessSettings - { - Arguments = args - }); + var exitCode = _context.StartProcess(ilRepackPath.FullPath, new ProcessSettings + { + Arguments = args, + WorkingDirectory = binDir + });cake-build/tasks/package.cake (1)
30-33: Packaging step assumes Windows (ilrepack.exe). Gate by OS or switch to a cross‑platform tool.If this task runs on non‑Windows runners, StartProcess("ilrepack.exe") will fail. Consider gating or resolving a cross‑platform ILRepack.
Do we guarantee Windows runners for “Package”? If not, I can add a WithCriteria gate for Windows or wire in a dotnet‑global‑tool variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cake-build/helpers/tools.cake(1 hunks)cake-build/tasks/package.cake(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check
- GitHub Check: check (net6.0)
🔇 Additional comments (3)
cake-build/helpers/tools.cake (1)
58-58: Verify /targetplatform:v4 suits all target frameworks.Android/iOS/NET Framework outputs may not all align with v4 metadata. ILRepack often works without explicitly setting /targetplatform, or may require netcore/netstandard. Please confirm on CI; consider making this configurable per project/TFM or removing it if unnecessary.
Would you like me to parameterize this based on each project’s TargetFramework?
cake-build/tasks/package.cake (2)
36-41: Include NETStandard/MAUI variants if applicable.If the build now targets newer SDKs, confirm whether additional projects/TFMs should be merged (e.g., netstandard2.x, net6/7/8 for Android/iOS). Otherwise some packages may ship unmerged binaries.
I can extend projectsToMerge based on actual project list after we confirm targets.
63-71: The review comment is incorrect. All three projects explicitly define<AssemblyName>IO.Ably</AssemblyName>in their .csproj files, so they all produceIO.Ably.dllregardless of their folder names (IO.Ably.Android, IO.Ably.iOS, IO.Ably.NETFramework). The hardcodedprimaryDll = binPath.CombineWithFilePath("IO.Ably.dll")in package.cake is correct and will not fail.Likely an incorrect or invalid review comment.
sacOO7
force-pushed
the
fix/github-workflow
branch
from
a7f1548 to
55a9b49
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1308/features
2 participants
Summary by CodeRabbit
Chores
Documentation
Devcontainer