add musl target to kos-go#212
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.github/workflows/kos-go.yaml (2)
44-49: Broaden commit pattern to include regenerated bindingsIf uniffi regenerates Go sources or headers, they won’t be committed with a single-file pattern.
Apply this diff:
- file_pattern: "packages/kos-go/kos_mobile/lib/linux-amd64/libkos_mobile.so" + file_pattern: | + packages/kos-go/kos_mobile/lib/linux-amd64/libkos_mobile.so + packages/kos-go/**/*.go + packages/kos-go/kos_mobile/*.hRepeat similarly for macOS/musl jobs if desired.
1-1: Avoid push conflicts from parallel auto-commitsAdd concurrency to serialize runs on a ref, or commit only in one job after aggregating artifacts.
Add this at the top-level of the workflow:
concurrency: group: kos-go-${{ github.ref }} cancel-in-progress: falseMakefile (1)
46-49: Make uniffi-bindgen-go install idempotent; crate-type for musl verified
- Avoid reinstalling uniffi-bindgen-go on every build — move install to CI setup or make it idempotent (use
cargo install -q --locked ... || truein build-go and build-go-musl).- Verified packages/kos-mobile/Cargo.toml:12 contains crate-type = ["cdylib", "staticlib", "rlib"] — both staticlib and cdylib present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
packages/kos-go/go.sumis excluded by!**/*.sumpackages/kos-go/kos_mobile/lib/linux-musl-amd64/libkos_mobile.sois excluded by!**/*.so
📒 Files selected for processing (6)
.github/workflows/kos-go.yaml(1 hunks).github/workflows/pull-develop.yaml(1 hunks)Makefile(1 hunks)packages/kos-go/.gitignore(1 hunks)packages/kos-go/kos_mobile/.gitignore(1 hunks)packages/kos-go/kos_mobile/cgo.go(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/kos-go.yaml
9-9: both "paths" and "paths-ignore" filters cannot be used for the same event "push". note: use '!' to negate patterns
(events)
22-22: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
59-59: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
64-64: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
98-98: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
103-103: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (1)
- GitHub Check: format
🔇 Additional comments (3)
packages/kos-go/kos_mobile/.gitignore (1)
1-16: LGTM – sensible ignores for tests/coverage/workspace/IDE.github/workflows/pull-develop.yaml (1)
20-20: LGTM, but confirm intent to not trigger checks on lib updatesIgnoring
packages/kos-go/kos_mobile/lib/**will prevent PR checks when only the prebuilt libs change (your new workflow will commit them). Confirm this is desired.packages/kos-go/.gitignore (1)
1-1: Change packages/kos-go/.gitignore to use vendor/ and verify vendor-mode usageReplace
vendorwithvendor/in packages/kos-go/.gitignore and confirm no build/test/CI tooling relies on Go's vendor mode (-mod=vendor).-vendor +vendor/Run to verify vendor usage:
rg -nP --type=go -C2 '(-mod=vendor|go\s+mod\s+vendor|GOMODCACHE=vendor|go\s+build\s+-mod=vendor|go\s+test\s+-mod=vendor)'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/kos-go.yaml (2)
26-30: Enable Go module caching for faster CIactions/setup-go supports caching.
Apply:
- name: Setup Go - uses: actions/setup-go@v4 + uses: actions/setup-go@v5 with: go-version: '1.21' + cache: trueRepeat at the other Setup Go steps.
Also applies to: 63-67, 102-106
1-16: Overall: solid structure and platform coverageMatrix could reduce duplication later; current explicit jobs are fine for now.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
.github/workflows/kos-go.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/kos-go.yaml
9-9: both "paths" and "paths-ignore" filters cannot be used for the same event "push". note: use '!' to negate patterns
(events)
22-22: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
59-59: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
64-64: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
98-98: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
103-103: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (7)
.github/workflows/kos-go.yaml (7)
120-124: Verify darwin folder namingConsumer conventions often use darwin-arm64; you’re using darwin-aarch64. Confirm downstream expectations.
Would you like me to search the repo for imports/CGO flags expecting this exact path?
4-13: Fix event filters: cannot combine paths and paths-ignore on pushUse only paths; convert exclusions to negations.
Apply:
on: push: - paths: - - 'packages/kos-go/kos_mobile/**' branches: - develop - paths-ignore: - - '**/*.md' - - '.gitignore' - - '.github/**' + paths: + - 'packages/kos-go/kos_mobile/**' + - '!**/*.md' + - '!.gitignore' + - '!.github/**' pull_request: branches: - develop
22-29: Upgrade core actions: checkout→v4, setup-go→v5Older versions are deprecated on GH Actions runners.
Apply:
- uses: actions/checkout@v3 + uses: actions/checkout@v4 @@ - uses: actions/setup-go@v4 + uses: actions/setup-go@v5Repeat the same replacements at Lines 59 and 64, and Lines 98 and 103.
Also applies to: 59-66, 98-105
74-76: Invalid multi-line run syntaxUse a block scalar for multi-line commands.
Apply:
- - name: Build Linux Go bindings - run: - rustup target add x86_64-unknown-linux-musl && make build-go-musl + - name: Build Linux Go bindings + run: | + rustup target add x86_64-unknown-linux-musl + make build-go-musl
79-81: Fix musl artifact dir nameCreate linux-musl-amd64 (not linux-amd64) to match cp target.
Apply:
- mkdir -p packages/kos-go/kos_mobile/lib/linux-amd64 + mkdir -p packages/kos-go/kos_mobile/lib/linux-musl-amd64 cp target/x86_64-unknown-linux-musl/release/libkos_mobile.so packages/kos-go/kos_mobile/lib/linux-musl-amd64/
85-86: Correct file_pattern path for musl commitUse dashed folder, not slash.
Apply:
- file_pattern: "packages/kos-go/kos_mobile/lib/linux-musl/amd64/libkos_mobile.so" + file_pattern: "packages/kos-go/kos_mobile/lib/linux-musl-amd64/libkos_mobile.so"
88-92: Run tests with musl build tagEnsure musl-specific cgo flags are used.
Apply:
- name: Run Go tests run: | cd packages/kos-go - make test + GOFLAGS="-tags=musl" make test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kos-go/Makefile (1)
151-151: Help text consistency (mention arch or align with directory naming).Minor nit: other entries include arch. Consider aligning wording to match directory naming.
- @echo " build-linux-musl - Build for Linux with musl" + @echo " build-linux-musl - Build for Linux musl AMD64" @@ - @echo " test-linux-musl - Test Linux with musl" + @echo " test-linux-musl - Test Linux musl AMD64"Also applies to: 155-155
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
packages/kos-go/.gitignore(1 hunks)packages/kos-go/Makefile(2 hunks)packages/kos-go/kos_mobile/cgo.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kos-go/kos_mobile/cgo.go
- packages/kos-go/.gitignore
🔇 Additional comments (1)
packages/kos-go/Makefile (1)
107-116: Use a musl-targeting CC, point LD_LIBRARY_PATH at the musl lib, and pass -tags=musl on go build (don’t use GOFLAGS)Linking a musl-built staticlib with glibc’s cc is error-prone — set CC to a musl-targeting compiler (e.g., x86_64-linux-musl-gcc or
zig cc -target x86_64-linux-musl), update LD_LIBRARY_PATH to the musl lib dir, and pass tags directly togo build.File: packages/kos-go/Makefile (lines ~107-116)
.PHONY: build-linux-musl build-linux-musl: - LD_LIBRARY_PATH=$(PWD)/kos_mobile/lib/linux-amd64 \ - CGO_ENABLED=1 \ - CGO_LDFLAGS="-lm" \ - GOOS=linux \ - GOARCH=amd64 \ - GOFLAGS="-tags=musl" \ - go build $(BUILD_FLAGS) -o bin/kos-linux-musl ./... + LD_LIBRARY_PATH=$(PWD)/kos_mobile/lib/linux-musl-amd64 \ + CGO_ENABLED=1 \ + CC=x86_64-linux-musl-gcc \ + CGO_LDFLAGS="-lm" \ + GOOS=linux \ + GOARCH=amd64 \ + go build -tags=musl $(BUILD_FLAGS) -o bin/kos-linux-musl ./...Confirm kos_mobile/lib/linux-musl-amd64 exists and CI/tooling provides a musl cc (or use
zig cc) before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.github/actions/rustup/action.yaml (1)
92-92: Trim install footprint; consider making musl-tools opt-in
- Add --no-install-recommends to reduce packages/time (already in the diff above).
- Optional: add a follow-up cleanup to free apt lists if you hit disk limits:
sudo rm -rf /var/lib/apt/lists/*- Consider an input (e.g., inputs.install_musl: boolean) to install musl-tools only when needed; or move musl setup to the kos-go workflow to keep this “WasmPack toolchain” action focused.
If you want, I can draft the input/plumbing changes to gate musl-tools installation.
.github/workflows/kos-go.yaml (5)
24-28: Drive Go version from go.mod and enable cachingReduces drift and speeds builds.
- name: Setup Go uses: actions/setup-go@v5 with: - go-version: '1.21' + go-version-file: packages/kos-go/go.mod + cache: true
64-68: Apply same go.mod-driven version and cache here- name: Setup Go uses: actions/setup-go@v5 with: - go-version: '1.21' + go-version-file: packages/kos-go/go.mod + cache: true
106-110: Apply same go.mod-driven version and cache here- name: Setup Go uses: actions/setup-go@v5 with: - go-version: '1.21' + go-version-file: packages/kos-go/go.mod + cache: true
3-11: Optional: Add PR path filters to reduce unnecessary runsMirror the push paths under pull_request to only run when kos-go assets change.
pull_request: branches: - develop + paths: + - 'packages/kos-go/kos_mobile/**'
13-18: Optional: Avoid concurrent auto-commits across jobsMultiple jobs committing to the same ref can race. Use workflow concurrency to cancel in-flight runs on the same branch.
Add at top-level:
concurrency: group: kos-go-${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
packages/kos-go/kos_mobile/lib/darwin-aarch64/libkos_mobile.dylibis excluded by!**/*.dylibpackages/kos-go/kos_mobile/lib/linux-amd64/libkos_mobile.sois excluded by!**/*.so
📒 Files selected for processing (2)
.github/actions/rustup/action.yaml(1 hunks).github/workflows/kos-go.yaml(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). (4)
- GitHub Check: build-go-linux
- GitHub Check: build-go-linux-musl
- GitHub Check: build-go-mac
- GitHub Check: format
🔇 Additional comments (6)
.github/actions/rustup/action.yaml (1)
90-92: Guard apt-get to Linux runnersGate the protobuf setup step to Linux to avoid breaking macOS/Windows runners; use --no-install-recommends.
- - name: Setup protobuf - shell: bash - run: sudo apt-get update && sudo apt-get -y install protobuf-compiler musl-tools + - name: Setup protobuf + if: runner.os == 'Linux' + shell: bash + run: sudo apt-get update && sudo apt-get install -y --no-install-recommends protobuf-compiler musl-toolsVerify no workflows call .github/actions/rustup on non-Linux runners.
.github/workflows/kos-go.yaml (5)
42-48: Auto-commit guard runs on PRs and can loop CI; restrict to push and add [skip ci]Current condition executes on pull_request and on push without “[skip ci]”, causing PR failures and retrigger loops. Limit to push and include “[skip ci]” in the commit message.
- - name: Commit and push changes - if: github.event_name != 'push' || !contains(github.event.head_commit.message, '[skip ci]') + - name: Commit and push changes + if: github.event_name == 'push' && !contains(github.event.head_commit.message, '[skip ci]') uses: stefanzweifel/git-auto-commit-action@v4 with: - commit_message: "Update Linux Go bindings library" + commit_message: "[skip ci] Update Linux Go bindings library" file_pattern: "packages/kos-go/kos_mobile/lib/linux-amd64/libkos_mobile.so"
79-83: mkdir targets wrong directory for musl; cp may failYou create linux-amd64 but copy into linux-musl-amd64. Create the correct directory.
- name: Copy .so to Go package run: | - mkdir -p packages/kos-go/kos_mobile/lib/linux-amd64 + mkdir -p packages/kos-go/kos_mobile/lib/linux-musl-amd64 cp target/x86_64-unknown-linux-musl/release/libkos_mobile.so packages/kos-go/kos_mobile/lib/linux-musl-amd64/
84-90: Same auto-commit guard issue on musl job; fix and add “[skip ci]”- name: Commit and push changes - if: github.event_name != 'push' || !contains(github.event.head_commit.message, '[skip ci]') + if: github.event_name == 'push' && !contains(github.event.head_commit.message, '[skip ci]') uses: stefanzweifel/git-auto-commit-action@v4 with: - commit_message: "Update Linux musl Go bindings library" + commit_message: "[skip ci] Update Linux musl Go bindings library" file_pattern: "packages/kos-go/kos_mobile/lib/linux-musl-amd64/libkos_mobile.so"
129-135: Same auto-commit guard issue on mac job; fix and add “[skip ci]”- name: Commit and push changes - if: github.event_name != 'push' || !contains(github.event.head_commit.message, '[skip ci]') + if: github.event_name == 'push' && !contains(github.event.head_commit.message, '[skip ci]') uses: stefanzweifel/git-auto-commit-action@v4 with: - commit_message: "Update macOS Go bindings library" + commit_message: "[skip ci] Update macOS Go bindings library" file_pattern: "packages/kos-go/kos_mobile/lib/darwin-aarch64/libkos_mobile.dylib"
111-117: Install rustup on macOS runner before callingrustup component addThe workflow (.github/workflows/kos-go.yaml lines 111–117) assumes rustup is present; add an install fallback (run the rustup-init installer or use a GitHub Action to set up Rust) before
rustup component add --toolchain stable rustfmt clippy.
3 participants
Summary by CodeRabbit
New Features
Build
Tests
Chores