Fix rustls transport error / use nix releases #1153
Conversation
…nsipx/downgrade-ssl
insipx
changed the title
insipx
changed the title
insipx
changed the title
WalkthroughThis pull request introduces multiple new GitHub Actions workflows for automating the release processes of Kotlin and Swift bindings. It modifies existing workflows to streamline installation methods and update dependencies. Several Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (19)
bindings_ffi/run_swift_local.sh (3)
1-3: Add shebang and LGTM for color and path resolution.The color variables and dynamic path resolution for
XMTP_SWIFTare well-implemented. However, it's recommended to add a shebang at the beginning of the script for better portability and explicit interpreter specification.Add the following shebang as the first line of the script:
+#!/usr/bin/env bash RED='\033[0;31m' NC='\033[0m' # No Color XMTP_SWIFT="${1:-$(realpath ../../libxmtp-swift)}"🧰 Tools
🪛 Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
12-15: Enhance build and copy commands with comments and error handling.While the build and copy commands are correctly using the
XMTP_SWIFTvariable, there are a few improvements that could be made:
- Add a comment explaining what
make swiftdoes.- Implement error handling for the
makeandcpcommands.Consider updating the section as follows:
+# Build Swift bindings -make swift +if ! make swift; then + echo "${RED}Failed to build Swift bindings${NC}" + exit 1 +fi +# Copy generated files to Swift project -cp build/swift/xmtpv3.swift $XMTP_SWIFT/Sources/LibXMTP/xmtpv3.swift -cp build/swift/libxmtp-version.txt $XMTP_SWIFT/Sources/LibXMTP/libxmtp-version.txt +if ! cp build/swift/xmtpv3.swift $XMTP_SWIFT/Sources/LibXMTP/xmtpv3.swift && \ + cp build/swift/libxmtp-version.txt $XMTP_SWIFT/Sources/LibXMTP/libxmtp-version.txt; then + echo "${RED}Failed to copy generated files to Swift project${NC}" + exit 1 +fi + +echo "${GREEN}Swift bindings built and copied successfully${NC}"This change adds comments, error handling, and a success message, improving the script's robustness and user feedback.
1-15: LGTM: Well-structured script. Consider adding a completion message.The overall structure of the script is logical and covers the necessary steps for running Swift locally. To further improve user experience, consider adding a completion message at the end of the script.
Add the following line at the end of the script:
cp build/swift/libxmtp-version.txt $XMTP_SWIFT/Sources/LibXMTP/libxmtp-version.txt +echo "${GREEN}Swift setup completed successfully${NC}"This will provide clear feedback to the user that the script has finished executing without errors.
🧰 Tools
🪛 Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
dev/release-swift (2)
6-10: Consider adding error checking forcargo locate-project.The path setup looks good, but it's worth adding a check to ensure the
cargo locate-projectcommand succeeds. This will help catch errors early if the command fails.Here's a suggested improvement:
WORKSPACE_MANIFEST=$(cargo locate-project --workspace --message-format=plain) if [ $? -ne 0 ]; then echo "Error: Failed to locate workspace manifest" >&2 exit 1 fi
16-19: Add error handling for build and copy operations.The build and copy operations look correct, but they lack error handling. Consider adding checks to ensure each step completes successfully.
Here's a suggested improvement:
cd "$BINDINGS_PATH" || { echo "Error: Failed to change directory to $BINDINGS_PATH" >&2; exit 1; } if ! nix develop ../ --command make swift; then echo "Error: Failed to build Swift bindings" >&2 exit 1 fi if ! cp build/swift/xmtpv3.swift "$XMTP_SWIFT/Sources/LibXMTP/xmtpv3.swift"; then echo "Error: Failed to copy xmtpv3.swift" >&2 exit 1 fi if ! cp build/swift/libxmtp-version.txt "$XMTP_SWIFT/Sources/LibXMTP/libxmtp-version.txt"; then echo "Error: Failed to copy libxmtp-version.txt" >&2 exit 1 fiAlso, consider using variables for the destination paths to improve maintainability.
dev/release-kotlin (2)
6-8: Consider adding Nix check for non-macOS systems.The script correctly installs Nix on macOS if it's not present. However, it doesn't verify the presence of Nix on other operating systems.
Consider adding a check for Nix on non-macOS systems to ensure it's available everywhere:
if [[ "${OSTYPE}" == "darwin"* ]]; then if ! which nix &>/dev/null; then curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- install --determinate; fi +else + if ! which nix &>/dev/null; then echo "Nix is required but not found. Please install Nix."; exit 1; fi fi
13-18: LGTM: Comprehensive build process for multiple architectures.The Nix and Cargo commands effectively build JNI libraries for all relevant Android architectures in a consistent environment.
For improved readability, consider aligning the target architectures:
nix develop . --command cargo ndk -o bindings_ffi/jniLibs/ --manifest-path ./bindings_ffi/Cargo.toml \ - -t aarch64-linux-android \ - -t x86_64-linux-android \ - -t i686-linux-android \ - -t armv7-linux-androideabi \ + -t aarch64-linux-android \ + -t x86_64-linux-android \ + -t i686-linux-android \ + -t armv7-linux-androideabi \ -- build --releasebindings_ffi/gen_kotlin.sh (1)
Line range hint
1-38: Suggestions for future enhancementsWhile the script is functional and the main change improves its reliability, here are some suggestions for future enhancements:
- Add error handling for the build and copy operations to make the script more robust.
- Implement checks for required tools (e.g., cargo, realpath) at the beginning of the script.
- Consider adding a verbose mode for easier debugging.
- Include a help/usage message to improve user guidance.
These improvements could further enhance the script's reliability and user-friendliness.
.github/workflows/release-kotlin-bindings.yml (1)
38-38: Approve the change with a minor suggestion.The switch to using the
taiki-e/install-action@crossaction for installing thecrosstool is a good improvement. It simplifies the installation process and likely provides better consistency across different environments.Consider pinning the action to a specific version (e.g.,
taiki-e/install-action@v2) to ensure reproducibility and avoid potential breaking changes in future updates of the action..github/workflows/release-swift-bindings-nix.yml (1)
41-61: Update runner and improve job clarity.The
swiftjob effectively generates Swift bindings, but there are a couple of points to address:
- As mentioned earlier, replace the custom runner label with a standard one:
runs-on: macos-latest
- To improve clarity, consider adding a comment or updating the job name to better describe its purpose. For example:
swift-bindings-generation: name: Generate Swift BindingsThese changes will enhance the job's portability and readability.
🧰 Tools
🪛 actionlint
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
xmtp_id/src/scw_verifier/mod.rs (2)
157-157: LGTM: Consistent environment variable accessThe change from
env::vartostd::env::varis correct and consistent with the updated imports. This standardizes environment variable access throughout the code.For consistency, consider updating all other occurrences of
env::varin this file (if any) to usestd::env::var.
Line range hint
157-163: LGTM: Added test utility featureThe addition of the
test-utilsfeature is a good improvement for testing purposes. It allows for easy integration of an Anvil verifier when needed, which can significantly aid in local testing and development.Consider making the Anvil chain ID configurable, either through an environment variable or a constant. This would provide more flexibility if the chain ID needs to change in the future.
Example:
const ANVIL_CHAIN_ID: &str = "eip155:31337"; // ... self.verifiers.insert( ANVIL_CHAIN_ID.to_string(), Box::new(RpcSmartContractWalletVerifier::new(url)?), );xmtp_mls/src/retry.rs (1)
188-188: Improved error logging for better debugging.The change enhances the error logging by including both the debug and display representations of the error. This provides more comprehensive information, which can be valuable for debugging and troubleshooting.
Consider using the
%formatting specifier instead of{}for the error's display representation to ensure consistent formatting across different error types:tracing::info!("error is not retryable. {:?}:%s", e, e);xmtp_proto/src/api_client.rs (1)
Line range hint
203-216: Improved error handling and formatting.The changes to the
fmt::Displayimplementation for theErrorstruct are well-structured and improve both readability and maintainability. The use of amatchexpression provides a clear mapping betweenErrorKindvariants and their corresponding error messages.For consistency, consider using the
write!macro instead of multiplewrite_strcalls. This can make the code more concise and easier to read. Here's a suggested improvement:write!(f, "{}", s)?; if let Some(source) = self.source() { write!(f, ": {}", source)?; }This change maintains the same functionality while slightly improving code consistency.
xmtp_mls/src/subscriptions.rs (4)
101-109: Approve change to use reference forwelcome_v1The modification to use a reference for
welcome_v1instead of cloning it is a good optimization that reduces unnecessary memory allocation. This change improves the efficiency of theprocess_streamed_welcomemethod.Consider adding a comment explaining why
welcome_v1is now passed as a reference, to make the intention clear for future maintainers:- let welcome_v1 = &welcome_v1; + // Use a reference to avoid unnecessary cloning + let welcome_v1 = &welcome_v1;
Line range hint
153-180: Approve enhancements tostream_conversationsmethodThe changes to the
stream_conversationsmethod are well-implemented:
- The new filtering mechanism for conversation types adds flexibility to the method.
- The clearer return type specification improves code readability and type safety.
These modifications enhance the overall functionality and usability of the method.
For improved readability, consider extracting the filter logic into a separate function:
fn filter_group_by_type( group: MlsGroup<Self>, conversation_type: &Option<ConversationType>, ) -> impl Future<Output = Option<MlsGroup<Self>>> { async move { let provider = crate::optify!(group.client.context().mls_provider())?; let metadata = crate::optify!(group.metadata(provider), "error processing group metadata"); metadata .filter(|m| conversation_type.map_or(true, |ct| ct == m.conversation_type)) .map(|_| group) } } // Then in stream_conversations: Ok(futures::stream::select(stream, event_queue) .filter_map(|group| filter_group_by_type(group, &conversation_type)))This separation of concerns makes the main method more concise and easier to understand.
Line range hint
516-755: Approve additions and updates to test casesThe new test cases and updates to existing ones are well-implemented:
- New tests for DM streaming provide good coverage for the added functionality.
- Updates to existing tests ensure the new conversation type filtering is properly tested.
- The tests cover various scenarios, including streaming all messages, DM-only streams, and group-only streams.
These changes improve the overall test coverage and reliability of the code.
Consider adding a test case that checks the behavior when switching between different conversation types mid-stream. This would ensure that the filtering mechanism works correctly when changing conversation types dynamically:
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] #[cfg_attr(not(target_arch = "wasm32"), tokio::test(flavor = "multi_thread"))] async fn test_switch_conversation_type_mid_stream() { // Setup clients and groups // ... let messages = Arc::new(Mutex::new(Vec::new())); let notify = Delivery::new(None); let (notify_pointer, messages_pointer) = (notify.clone(), messages.clone()); let mut stream = client.stream_all_messages(Some(ConversationType::Group)).await.unwrap(); // Send and receive a group message // ... // Switch to DM conversation type stream = client.stream_all_messages(Some(ConversationType::Dm)).await.unwrap(); // Send and receive a DM // ... // Assert that only appropriate messages were received based on the current conversation type // ... }This test would help ensure that the conversation type filtering works correctly even when changed dynamically.
Line range hint
1-755: Overall improvements to subscription handlingThe changes in this file significantly enhance the XMTP client's subscription handling:
- Memory efficiency: Using references instead of cloning in
process_streamed_welcome.- Filtering flexibility: New conversation type filtering in
stream_conversations.- Comprehensive testing: Added and updated tests for new features, especially DM streaming.
These improvements contribute to a more efficient, flexible, and reliable implementation of the subscription system.
As the codebase grows, consider splitting the test cases into a separate file to improve maintainability. This separation would make it easier to manage and extend the test suite in the future.
flake.nix (1)
78-81: Clarify the usage of deprecatedANDROID_SDK_ROOTYou've set
ANDROID_SDK_ROOT(line 80) even though it's deprecated, mentioning that some tools may still use it. Consider specifying which tools require it or verify if all tools can useANDROID_HOMEinstead, to keep the environment variables up to date.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
- .github/workflows/release-kotlin-bindings-nix.yml (1 hunks)
- .github/workflows/release-kotlin-bindings.yml (1 hunks)
- .github/workflows/release-swift-bindings-nix.yml (1 hunks)
- .github/workflows/release-swift-bindings.yml (1 hunks)
- Cargo.toml (4 hunks)
- bindings_ffi/Cargo.toml (2 hunks)
- bindings_ffi/Makefile (1 hunks)
- bindings_ffi/gen_kotlin.sh (1 hunks)
- bindings_ffi/run_swift_local.sh (1 hunks)
- dev/release-kotlin (1 hunks)
- dev/release-swift (1 hunks)
- flake.nix (1 hunks)
- rust-toolchain (1 hunks)
- xmtp_api_grpc/Cargo.toml (1 hunks)
- xmtp_id/Cargo.toml (1 hunks)
- xmtp_id/src/scw_verifier/mod.rs (2 hunks)
- xmtp_mls/src/client.rs (2 hunks)
- xmtp_mls/src/groups/mod.rs (1 hunks)
- xmtp_mls/src/groups/subscriptions.rs (2 hunks)
- xmtp_mls/src/retry.rs (1 hunks)
- xmtp_mls/src/subscriptions.rs (1 hunks)
- xmtp_proto/src/api_client.rs (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/release-kotlin-bindings-nix.yml
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
70-70: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release-swift-bindings-nix.yml
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
83-83: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
87-87: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release-swift-bindings.yml
29-29: key "uses" is duplicated in element of "steps" section. previously defined at line:24,col:9
(syntax-check)
🪛 yamllint
.github/workflows/release-swift-bindings.yml
[error] 29-29: duplication of key "uses" in mapping
(key-duplicates)
🪛 Shellcheck
bindings_ffi/run_swift_local.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (41)
rust-toolchain (1)
4-14: LGTM! The changes align well with the PR objectives.The additional target architectures in the
rust-toolchainfile support the following improvements:
- Address Android-related issues by including various Android targets.
- Support Swift bindings with the addition of iOS targets.
- Enable cross-compilation for the new workflows using native dependencies.
- Maintain backward compatibility by retaining existing targets.
These changes are crucial for the new GitHub Actions workflows introduced in this PR, particularly for releasing Kotlin and Swift bindings across multiple platforms.
bindings_ffi/run_swift_local.sh (1)
5-10: LGTM: Robust directory check and error handling.The implementation of the directory existence check and error handling is well done. The use of colored error messages and early exit improves user experience and prevents potential issues downstream.
xmtp_api_grpc/Cargo.toml (1)
13-13: LGTM! Verify tonic version in workspace root.The addition of the "tls" feature to the tonic dependency is in line with the PR objectives of addressing rustls-related issues. This change enhances the TLS capabilities and aligns with the project's move towards standardizing TLS usage.
To ensure full compatibility, please verify that the tonic version is correctly pinned in the workspace root Cargo.toml as mentioned in the PR objectives. Run the following script to check:
✅ Verification successful
Verified! The tonic version in the workspace root Cargo.toml is correctly set to "^0.12.2".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify tonic version and features in workspace root Cargo.toml # Test: Check tonic dependency in root Cargo.toml rg -A 5 'tonic.*=.*version.*=' Cargo.tomlLength of output: 417
dev/release-swift (2)
1-5: LGTM: Robust error handling and clear purpose.The script starts with proper shebang and robust error handling using
set -eou pipefail. The comment clearly explains the script's purpose.
1-19: Overall, the script is functional but could benefit from improved error handling and security practices.The script successfully sets up the environment, builds Swift bindings, and copies the generated files. However, there are several areas where it could be improved:
- Add error checking for the
cargo locate-projectcommand.- Enhance security for the Nix installation process on macOS.
- Implement error handling for the build and copy operations.
- Consider using variables for destination paths to improve maintainability.
These improvements will make the script more robust and easier to maintain in the long run.
To ensure the script is being used correctly in the project, let's check for any references to it:
dev/release-kotlin (3)
1-4: LGTM: Robust error handling and clear purpose.The script starts with proper shebang and comprehensive error handling options. The comment clearly explains the script's purpose.
10-11: LGTM: Clear and maintainable variable declarations.The variables for library names are well-defined and make the script more maintainable.
1-23: Overall: Well-structured script aligning with PR objectives.This script effectively automates the process of building and renaming Android JNI libraries for multiple architectures. It aligns well with the PR objectives of simplifying local testing and speeding up CI builds by using Nix to ensure a consistent build environment. The script addresses the need for quick building of JNI artifacts locally, as mentioned in the PR summary.
While the script is generally well-structured, consider implementing the suggested improvements to enhance its robustness and maintainability.
bindings_ffi/gen_kotlin.sh (1)
11-11: Improved path handling withrealpathThe use of
realpathto convert theXMTP_ANDROIDpath to an absolute path is a good improvement. This change enhances the script's reliability by ensuring it works correctly regardless of the current working directory when executed. It also aligns well with CI environments where absolute paths are often preferred.bindings_ffi/Cargo.toml (3)
24-24: Addition of tonic as a workspace dependencyThe addition of tonic as a workspace dependency aligns with the PR objectives. This change ensures consistent versioning of tonic across the workspace and is part of the effort to address the rustls and tonic-related issues mentioned in the PR summary.
34-34: Update of ethers dependency to use rustlsChanging the ethers dependency to use the rustls feature instead of openssl is in line with the PR objectives. This modification is part of the broader effort to remove OpenSSL pins and address the dependency upgrade issues mentioned in the PR summary. It also aligns with the project's shift towards using Rustls across multiple packages.
Line range hint
1-42: Verify workspace version updateThe AI summary mentions a version update from 0.0.1 to 0.1.0, but this change is not visible in the provided code. Since the version is set to use the workspace version (
version.workspace = true), please verify that the workspace-level version has been updated as mentioned in the summary.To confirm the version update, please run the following command from the repository root:
✅ Verification successful
Workspace version updated successfully
The workspace version has been updated to
0.1.0as required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the workspace version update grep -n "^version" Cargo.tomlLength of output: 52
.github/workflows/release-kotlin-bindings-nix.yml (7)
1-3: LGTM: Workflow name and trigger are appropriate.The workflow name "Release Kotlin Bindings" is clear and descriptive. Using
workflow_dispatchfor manual triggering is suitable for release workflows, allowing controlled execution.
5-6: Verify the custom runner label.The runner label
warp-ubuntu-latest-x64-16xis not a standard GitHub-hosted runner. Ensure this custom runner is properly configured in your GitHub organization or consider using a standard GitHub-hosted runner likeubuntu-latestif appropriate.🧰 Tools
🪛 actionlint
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
7-44: LGTM: Well-structured build job with Nix integration.The
build-linuxjob is well-configured:
- Matrix strategy covers all necessary Android architectures.
- Dependency caching is implemented for improved performance.
- Nix installation with GitHub token configuration helps avoid rate limiting.
- The use of
cargo ndkfor building JNI libraries aligns with the PR objectives.This setup should provide a consistent and efficient build process across different Android architectures.
45-53: LGTM: Proper artifact handling.The steps for preparing and uploading artifacts are well-implemented:
- Renaming the library ensures consistency with expected naming conventions.
- Using a 1-day retention period for artifacts is appropriate for temporary build outputs in the release process.
This approach facilitates efficient management of build artifacts during the release workflow.
54-70: LGTM: Well-structured packaging job.The
package-kotlinjob is well-configured:
- Proper dependency on the
build-linuxjob ensures correct workflow sequence.- Artifact download and ZIP creation steps are straightforward and effective.
This setup should reliably package the built JNI libraries for release.
🧰 Tools
🪛 actionlint
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
70-70: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
71-89: LGTM: Proper release creation and asset upload.The release creation and asset upload process is well-implemented:
- Creating the release as a pre-release is appropriate for automated workflows.
- The use of standard GitHub Actions for release creation and asset upload ensures reliability.
- The release naming convention using the short SHA provides clear version identification.
This setup should result in a properly created GitHub release with the packaged JNI libraries attached as an asset.
1-89: Overall assessment: Well-implemented release workflow for Kotlin bindings.This new workflow successfully implements the PR objectives:
- It introduces the use of Nix for building JNI libraries, which should speed up CI builds.
- The workflow covers multiple Android architectures, ensuring comprehensive artifact generation.
- The release process is automated, creating pre-releases with properly packaged artifacts.
Minor improvements have been suggested for the runner label and short SHA generation. Once these are addressed, this workflow should significantly enhance the release process for Kotlin bindings.
🧰 Tools
🪛 actionlint
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
70-70: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release-swift-bindings-nix.yml (3)
1-3: LGTM: Workflow name and trigger are appropriate.The workflow name "Release Swift Bindings" clearly describes its purpose, and the manual trigger (workflow_dispatch) is suitable for a release process that requires human oversight.
1-107: Overall assessment: Well-structured workflow with minor improvements needed.This GitHub Actions workflow effectively automates the release process for Swift bindings. It's well-organized into logical jobs for building, generating bindings, and packaging. The use of matrix strategy for multiple targets is commendable.
Key strengths:
- Clear separation of concerns across jobs.
- Effective use of caching and artifacts.
- Comprehensive release process including checksum generation and GitHub release creation.
Areas for improvement:
- Replace custom runner labels with standard GitHub-hosted runners for better portability.
- Update deprecated
set-outputcommands to the current syntax.- Fix potential shell scripting issues by properly quoting variables.
Implementing these suggestions will significantly enhance the workflow's reliability, maintainability, and compatibility across different environments.
🧰 Tools
🪛 actionlint
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
83-83: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
87-87: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
5-40: Consider using a standard GitHub-hosted runner for better portability.The
build-macosjob is well-structured and uses a matrix strategy effectively. However, the custom runner labelwarp-macos-13-arm64-6xmay cause issues:
- It's not a standard GitHub-hosted runner label, which could lead to workflow failures if the custom runner is not available.
- Using a custom runner may impact the portability and reproducibility of the workflow across different environments.
To ensure the workflow's portability, consider using a standard GitHub-hosted macOS runner. You can verify available runners with this script:
Replace the custom runner with a standard one, for example:
runs-on: macos-latestThis change will improve the workflow's compatibility and ease of maintenance.
🧰 Tools
🪛 actionlint
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
bindings_ffi/Makefile (3)
53-55: LGTM! Consistent with ARCHS_IOS changes.The changes here mirror those made for the iOS architectures, maintaining consistency across different targets. This is good for maintainability.
Please refer to the previous comment regarding the verification of the
--no-default-featuresflag's impact.
48-50: LGTM! Verify the impact of--no-default-features.The change from
cross buildtocargo buildsimplifies the build process by leveraging Cargo's native capabilities. This is a good improvement.However, please ensure that the addition of
--no-default-featuresdoesn't unintentionally exclude any necessary functionality for these iOS targets.To verify the impact, you can run:
58-60: Verify the intentional omission of--no-default-features.The change to
cargo buildis consistent with the other architecture targets, which is good. However, unlike the iOS and Mac targets, the--no-default-featuresflag is not used here.Is this omission intentional? If not, consider adding the flag for consistency. If it is intentional, it might be helpful to add a comment explaining why this target is treated differently.
To verify the difference, you can run:
Cargo.toml (6)
25-25: Verify the intentionality of the version bump.The workspace package version has been updated from "0.0.1" to "0.1.0". Please confirm that this minor version bump is intentional and aligns with the project's versioning strategy, especially considering the changes introduced in this PR.
60-63: Acknowledge tonic dependency update and testing requirements.The tonic dependency has been updated to version "^0.12.2" with specific TLS-related features. This change aligns with the PR objectives of addressing rustls transport errors. However, as noted in the comments:
- These changes may potentially affect the Android build.
- Testing with Android and Swift is recommended before merging.
- Consider exploring the use of https://github.com/rustls/rustls-native-certs in the future for potential improvements.
Ensure thorough testing is conducted, especially for Android and Swift platforms, before merging this change.
64-64: Acknowledge rustls dependency addition and version pinning.The addition of rustls with a specific version (=0.23.7) and the "ring" feature aligns with the PR objectives. This precise version pinning suggests a need for compatibility with other components.
Consider setting up dependency update checks to ensure you're notified of future rustls updates that might be compatible and bring improvements or security fixes.
77-77: Acknowledge openssl-sys version generalization.The openssl-sys dependency version has been generalized from "0.9.104" to "0.9". This change allows for minor version updates, which can include bug fixes and non-breaking changes. It aligns with the PR description mentioning the removal of OpenSSL pins.
Monitor for any potential issues that may arise from this more flexible version specification, especially in relation to the rustls changes.
78-78: Acknowledge openssl dependency update with "vendored" feature.The openssl dependency has been updated to include the "vendored" feature. This change ensures that OpenSSL is compiled from source and statically linked, which can:
- Avoid system-level OpenSSL version conflicts.
- Ensure consistency across different build environments.
- Simplify the build process, aligning with the PR objectives.
This update is a positive change that should improve build reliability and consistency.
Line range hint
25-78: Summary of Cargo.toml changes and their impact.The changes in this file address the PR objectives by:
- Updating the workspace version to reflect the changes.
- Modifying tonic and rustls dependencies to resolve transport errors.
- Adjusting OpenSSL-related dependencies to simplify builds and ensure consistency.
These modifications should improve build reliability and address the reported issues. However, it's crucial to:
- Thoroughly test these changes, especially on Android and Swift platforms.
- Monitor for any potential issues arising from the more flexible version specifications.
- Consider setting up dependency update checks for future improvements and security updates.
Overall, these changes appear to be well-considered and aligned with the project's goals.
xmtp_id/src/scw_verifier/mod.rs (1)
4-4: LGTM: Improved import organizationThe changes to the imports enhance code organization by removing the unnecessary
envmodule import and directly importingHashMapandfsfromstd. This approach is more explicit and aligns with Rust best practices.xmtp_mls/src/groups/subscriptions.rs (4)
38-39: Optimization: Improved memory usage inprocess_stream_entryGood job on optimizing memory usage by passing
client_idandmsgv1by reference. This change reduces unnecessary cloning and improves performance. The error handling refinement forprocess_messageis also a nice touch.Also applies to: 53-53
Line range hint
108-116: Improved error handling inprocess_streamed_group_messageExcellent update to the
process_streamed_group_messagemethod. The new return typeResult<StoredGroupMessage, GroupError>and the use ofok_orto convertOptiontoResultimprove error handling and make the method's behavior more explicit. This change aligns well with Rust's idiomatic error handling practices.
Line range hint
131-140: Enhanced type safety instream_with_callbackGreat improvement to the
stream_with_callbackfunction. The updated return typeimpl crate::StreamHandle<StreamOutput = Result<(), crate::groups::ClientError>>provides more information about the stream's output type. This change enhances type safety and makes the function's behavior more explicit, which will help prevent potential runtime errors and improve code maintainability.
38-39: Summary: Excellent optimizations and improvementsThe changes in this file demonstrate a commendable effort to enhance the codebase:
- Memory optimization by passing variables by reference.
- Improved error handling, particularly in the
process_streamed_group_messagemethod.- Enhanced type safety in the
stream_with_callbackfunction.These refinements align well with the PR objectives and contribute to better performance, reliability, and maintainability of the code. Great work on these improvements!
Also applies to: 53-53, 108-116, 131-140
xmtp_mls/src/client.rs (2)
Line range hint
803-813: Improved performance by passingwelcome_v1by referenceThe change from cloning
welcome_v1to passing it by reference is a good optimization. This reduces unnecessary memory allocation and copying, potentially improving performance, especially when processing multiple welcome messages.
Line range hint
819-833: Enhanced error handling for duplicate welcome IDsThe error handling has been refined to provide more specific logging for duplicate welcome IDs. This improvement helps in distinguishing between different types of errors during group creation from welcome messages, making debugging and troubleshooting easier.
flake.nix (2)
47-49: Ensure the correctness of thesha256hash forrust-toolchainVerify that the
sha256hash provided (line 48) matches the actual hash of therust-toolchainfile. A mismatch could lead to build failures or security issues.
4-4: Confirm the Nixpkgs version specifiedYou are specifying
nixos-24.05as the Nixpkgs version (line 4). Ensure that this is the intended version and that it's stable for your use case.xmtp_mls/src/groups/mod.rs (1)
484-488: Improved performance by passing a borrowed sliceChanging
encrypted_welcome_bytesfromVec<u8>to&[u8]avoids unnecessary cloning and enhances performance.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nsipx/downgrade-ssl
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
dev/release-kotlin (2)
6-19: LGTM: Well-implemented Nix installation check and prompt.The script correctly checks for macOS and Nix installation, providing a user-friendly prompt for installation. The installation source is trusted, and users have the option to abort.
Consider adding a check for Linux systems as well, as the script might be used on Linux-based CI systems:
if [[ "${OSTYPE}" == "darwin"* ]] || [[ "${OSTYPE}" == "linux-gnu"* ]]; then # Existing Nix check and installation logic fi
24-29: LGTM: Comprehensive build command for multiple architectures.The Nix and Cargo commands are well-structured to build JNI libraries for multiple Android architectures in a consistent environment. The build is correctly set to release mode for distribution.
Consider adding a progress indicator or echo statements to provide feedback during the build process, as it might take some time:
echo "Building JNI libraries for multiple architectures..." nix develop . --command cargo ndk -o bindings_ffi/jniLibs/ --manifest-path ./bindings_ffi/Cargo.toml \ -t aarch64-linux-android \ -t x86_64-linux-android \ -t i686-linux-android \ -t armv7-linux-androideabi \ -- build --release echo "Build completed successfully."dev/release-swift (2)
12-25: Great improvements on Nix installation security!The script now addresses the security concerns raised in the previous review:
- It uses HTTPS for the download.
- It provides information about the installation script.
- It gives users the option to review and abort the installation.
These changes significantly improve the security and user control of the Nix installation process.
Consider adding a comment explaining why Nix is necessary for the script, to provide context for users who may be unfamiliar with the project setup.
27-28: LGTM: Proper Swift binding build process.The script correctly changes to the bindings directory and uses
nix developto ensure a consistent build environment. This approach is robust and maintainable.Consider adding error handling for the
make swiftcommand, in case it fails:if ! nix develop ../ --command make swift; then echo "Failed to build Swift bindings" exit 1 fi.github/workflows/release-kotlin-bindings-nix.yml (1)
58-89: LGTM with suggestions: Update deprecated actions.The
package-kotlinjob is well-structured for packaging and releasing the Kotlin bindings. However, two actions used in this job are deprecated:
actions/create-release@v1actions/upload-release-asset@v1Consider updating these to their recommended replacements:
- name: Create release env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} uses: softprops/action-gh-release@v1 with: files: ./bindings_ffi/LibXMTPKotlinFFI.zip tag_name: kotlin-bindings-${{ steps.slug.outputs.sha7 }} name: Kotlin-Bindings-${{ steps.slug.outputs.sha7 }} draft: false prerelease: trueThis single action replaces both the create release and upload asset steps, simplifying the workflow and using up-to-date actions.
🧰 Tools
🪛 actionlint
70-70: shellcheck reported issue in this script: SC2086:info:1:19: Double quote to prevent globbing and word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2086:info:1:50: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- .github/workflows/release-kotlin-bindings-nix.yml (1 hunks)
- .github/workflows/release-swift-bindings-nix.yml (1 hunks)
- .github/workflows/release-swift-bindings.yml (1 hunks)
- dev/release-kotlin (1 hunks)
- dev/release-swift (1 hunks)
- flake.nix (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/release-swift-bindings.yml
- flake.nix
🧰 Additional context used
🪛 actionlint
.github/workflows/release-kotlin-bindings-nix.yml
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
70-70: shellcheck reported issue in this script: SC2086:info:1:19: Double quote to prevent globbing and word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2086:info:1:50: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release-swift-bindings-nix.yml
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
83-83: shellcheck reported issue in this script: SC2086:info:1:78: Double quote to prevent globbing and word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (11)
dev/release-kotlin (3)
1-5: LGTM: Robust script setup with clear purpose.The script begins with proper shebang and comprehensive error handling options. The comment clearly states the script's purpose, which is to release Android JNI libraries in a CI-consistent environment.
21-22: LGTM: Clear variable declarations.The LIBRARY_NAME and TARGET_NAME variables are well-defined and their purpose is clear. These will be used later for library renaming.
31-33: LGTM: Efficient library renaming implemented.The loop for renaming libraries is concise and efficient, addressing the suggestion from the previous review. This implementation ensures correct library names for all Android architectures.
dev/release-swift (1)
1-10: LGTM: Solid script setup and error handling.The script starts with proper shebang, strict error handling, and clear variable setup using
cargo locate-project. This approach ensures robustness and clarity..github/workflows/release-kotlin-bindings-nix.yml (4)
1-3: LGTM: Workflow name and trigger are appropriate.The workflow name "Release Kotlin Bindings" is clear and descriptive. Using
workflow_dispatchfor manual triggering is suitable for a release workflow, allowing controlled execution.
5-23: Verify custom runner and LGTM for matrix strategy.The matrix strategy for building multiple Android architectures is well-structured and comprehensive. However, the custom runner label
warp-ubuntu-latest-x64-16xis not recognized by the static analysis tool.Please confirm that
warp-ubuntu-latest-x64-16xis a valid custom runner in your GitHub Actions environment. If it's intentional, consider documenting it in the repository's README or in a comment within the workflow file. Alternatively, if it's not needed, you may want to use a standard GitHub-hosted runner likeubuntu-latest.🧰 Tools
🪛 actionlint
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
24-53: LGTM: Well-structured build process with appropriate actions.The
build-linuxjob is well-organized, using up-to-date versions of actions for checkout, caching, and artifact upload. The use of Nix and cargo-ndk for cross-compilation is appropriate for building Android libraries.
54-57: LGTM: Correct job dependency and configuration.The
package-kotlinjob correctly depends on thebuild-linuxjob, ensuring proper sequencing of the workflow. The job configuration is appropriate for its purpose.Regarding the custom runner
warp-ubuntu-latest-x64-16x, please refer to the previous comment about verifying its validity in your GitHub Actions environment.🧰 Tools
🪛 actionlint
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/release-swift-bindings-nix.yml (3)
41-61: Apply consistent runner configurationFor consistency with the
build-macosjob, consider using the same fallback runner option for theswiftjob:runs-on: ${{ fromJson(vars.USE_CUSTOM_RUNNER && '["warp-macos-13-arm64-6x"]' || '["macos-latest"]') }}This ensures that both jobs can run on standard GitHub-hosted runners if the custom runner is unavailable.
🧰 Tools
🪛 actionlint
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
62-64: Apply consistent runner configurationFor consistency with the other jobs, use the same fallback runner option for the
package-swiftjob:runs-on: ${{ fromJson(vars.USE_CUSTOM_RUNNER && '["warp-macos-13-arm64-6x"]' || '["macos-latest"]') }}This ensures that all jobs in the workflow can run on standard GitHub-hosted runners if the custom runner is unavailable.
🧰 Tools
🪛 actionlint
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
1-107: Overall workflow review: Effective automation for Swift bindings releaseThis workflow effectively automates the release process for Swift bindings. It covers building for multiple Apple platforms, generating Swift bindings, packaging, and creating a GitHub release. The use of Nix ensures consistent development environments across different stages of the workflow.
Key benefits:
- Matrix build strategy for various Apple platforms and architectures.
- Consistent environment using Nix.
- Automated artifact handling and release creation.
With the suggested improvements for runner configuration and shell scripting, this workflow will be more robust and portable. Great job on setting up this comprehensive release automation!
🧰 Tools
🪛 actionlint
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
83-83: shellcheck reported issue in this script: SC2086:info:1:78: Double quote to prevent globbing and word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
dev/release-swift (5)
1-11: LGTM! Consider enhancingXMTP_SWIFTvariable handling.The script setup and environment variable initialization look good. The use of
set -eou pipefailfor strict error handling is a great practice. The way you're locating the workspace and bindings paths using Cargo is robust.Consider adding a check for the
XMTP_SWIFTvariable to ensure it's not empty and the directory exists:if [ -z "${XMTP_SWIFT}" ] || [ ! -d "${XMTP_SWIFT}" ]; then echo "Error: XMTP_SWIFT is not set or directory does not exist" exit 1 fi
13-26: Good improvements, consider further security enhancements.The Nix installation check and user prompt are good improvements. The script now uses HTTPS and allows users to review the installation script before running it, addressing some of the security concerns raised in the past review.
Consider adding a checksum verification step for the Nix installer script to further enhance security:
if [[ "${OSTYPE}" == "darwin"* ]]; then if ! which nix &>/dev/null; then echo "Nix is not installed. Please review the installation script at:" echo "https://install.determinate.systems/nix" read -p "Do you want to proceed with the installation? (y/n) " -n 1 -r echo if [[ $REPLY =~ ^[Yy]$ ]]; then INSTALLER_URL="https://install.determinate.systems/nix" EXPECTED_CHECKSUM="<insert_expected_checksum_here>" curl -sSL "$INSTALLER_URL" > nix_installer.sh ACTUAL_CHECKSUM=$(shasum -a 256 nix_installer.sh | cut -d ' ' -f 1) if [ "$ACTUAL_CHECKSUM" = "$EXPECTED_CHECKSUM" ]; then sh nix_installer.sh install --determinate rm nix_installer.sh else echo "Checksum verification failed. Aborting installation." rm nix_installer.sh exit 1 fi else echo "Nix installation aborted. Please install Nix manually or run commands in local environment to proceed." exit 1 fi fi fiThis enhancement adds a layer of security by verifying the integrity of the installer script before execution.
28-29: LGTM! Consider adding error handling.The directory change and Nix command execution look good. Using Nix ensures a consistent environment for building Swift bindings.
Consider adding error handling for the
cdandnix developcommands:if ! cd "$BINDINGS_PATH"; then echo "Error: Failed to change directory to $BINDINGS_PATH" exit 1 fi if ! nix develop ../ --command make swift; then echo "Error: Failed to build Swift bindings" exit 1 fiThis will provide more informative error messages if something goes wrong during these steps.
30-36: Good improvements, consider further enhancements.The file copying process has been improved with checks for file existence and warnings for missing files. This addresses some of the concerns raised in the past review.
Consider these additional enhancements:
- Ensure the destination directory exists before copying:
- Use
cp -vfor verbose output of the copying process.- Add an overall success/failure message.
Here's a suggested implementation:
DEST_DIR="$XMTP_SWIFT/Sources/LibXMTP" mkdir -p "$DEST_DIR" copy_success=true for file in "build/swift/xmtpv3.swift" "build/swift/libxmtp-version.txt"; do if [ -f "$file" ]; then if cp -v "$file" "$DEST_DIR/$(basename "$file")"; then echo "Successfully copied $file" else echo "Error: Failed to copy $file" copy_success=false fi else echo "Warning: $file not found" copy_success=false fi done if $copy_success; then echo "All files were successfully copied." else echo "Warning: Some files were not copied. Check the output above for details." fiThis implementation ensures the destination directory exists, provides verbose output for successful copies, and gives an overall status of the copying process.
1-36: Overall, a well-structured script with room for enhancements.This script provides a solid foundation for releasing Swift bindings. It covers the necessary steps including environment setup, Nix installation check, building Swift bindings, and file copying. The error handling and user prompts are good practices.
Consider these additional enhancements to further improve the script:
Add logging throughout the script:
log() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] $1" } # Use it like this: log "Starting Swift bindings release process"Implement a cleanup function to handle any temporary files or states:
cleanup() { # Add cleanup logic here log "Cleanup completed" } trap cleanup EXITAdd a version check for Nix to ensure compatibility:
check_nix_version() { local required_version="2.3.0" local current_version=$(nix --version | cut -d' ' -f3) if [ "$(printf '%s\n' "$required_version" "$current_version" | sort -V | head -n1)" != "$required_version" ]; then log "Error: Nix version $required_version or higher is required" exit 1 fi } # Call this function after Nix installation check check_nix_versionConsider adding a dry-run option for testing:
DRY_RUN=false while getopts ":d" opt; do case ${opt} in d ) DRY_RUN=true ;; \? ) echo "Usage: $0 [-d]" ;; esac done # Use it like this: if $DRY_RUN; then log "Dry run: Would copy $file to $DEST_DIR" else cp -v "$file" "$DEST_DIR/$(basename "$file")" fiThese enhancements will improve logging, error handling, version compatibility, and testing capabilities of the script.
.github/workflows/release-kotlin-bindings-nix.yml (1)
41-44: Consider adding error handling to thecargo ndkcommand.The
cargo ndkcommand is crucial for building the JNI libraries. To improve robustness, consider adding error handling and logging.Here's a suggested improvement:
- name: Build jniLibs run: | set -eo pipefail nix develop --command \ cargo ndk -o bindings_ffi/jniLibs --manifest-path ./bindings_ffi/Cargo.toml \ -t ${{ matrix.target }} -- build --release shell: bashThis change adds
set -eo pipefailto exit immediately if any command fails andshell: bashto ensure consistent behavior across different operating systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/release-kotlin-bindings-nix.yml (1 hunks)
- .github/workflows/release-swift-bindings-nix.yml (1 hunks)
- dev/release-swift (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/release-kotlin-bindings-nix.yml
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
56-56: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
70-70: shellcheck reported issue in this script: SC2086:info:1:19: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release-swift-bindings-nix.yml
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (6)
.github/workflows/release-kotlin-bindings-nix.yml (2)
1-3: LGTM: Workflow name and trigger are appropriate.The workflow name "Release Kotlin Bindings" is clear and descriptive. Using
workflow_dispatchas the trigger is suitable for a release workflow, providing manual control over when releases are initiated.
5-23: Verify the custom runner label.The
build-linuxjob is well-structured with a matrix strategy covering all major Android architectures. However, the custom runner labelwarp-ubuntu-latest-x64-16xis not a standard GitHub-hosted runner.Ensure that this custom runner is properly configured in your GitHub Actions environment. If it's not intended to be a custom runner, consider using a standard GitHub-hosted runner like
ubuntu-latest.To verify the available runners in your repository, you can run the following GitHub CLI command:
Replace
{owner}and{repo}with your repository details.🧰 Tools
🪛 actionlint
6-6: label "warp-ubuntu-latest-x64-16x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/release-swift-bindings-nix.yml (4)
1-3: LGTM: Workflow name and trigger are appropriate.The workflow name "Release Swift Bindings" clearly describes its purpose, and the manual trigger (
workflow_dispatch) allows for controlled releases.
41-61: Ensure consistency withbuild-macosjob runnerThe
swiftjob is also using the custom runner labelwarp-macos-13-arm64-6x. For consistency and to address potential availability issues, apply the same fallback strategy suggested for thebuild-macosjob:runs-on: ${{ fromJson(vars.USE_CUSTOM_RUNNER && '["warp-macos-13-arm64-6x"]' || '["macos-latest"]') }}This ensures that both jobs use the same runner selection logic, improving maintainability and reliability of the workflow.
🧰 Tools
🪛 actionlint
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
62-107: Approve release process and suggest runner consistencyThe
package-swiftjob effectively packages and releases the Swift bindings. The process of downloading artifacts, building the archive, and creating a GitHub release with the checksum is well-structured.However, for consistency with the previous jobs and to address potential availability issues, apply the same fallback strategy for the runner:
runs-on: ${{ fromJson(vars.USE_CUSTOM_RUNNER && '["warp-macos-13-arm64-6x"]' || '["macos-latest"]') }}This ensures that all jobs in the workflow use the same runner selection logic, improving overall reliability.
🧰 Tools
🪛 actionlint
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
1-107: Overall, well-structured workflow with minor improvements neededThis workflow effectively automates the release process for Swift bindings. It's well-organized with clear job dependencies and consistent use of Nix for environment setup. The release process, including checksum calculation and artifact management, is robust.
Key strengths:
- Clear separation of concerns between building, generating bindings, and packaging.
- Effective use of caching to optimize performance.
- Comprehensive matrix strategy for building multiple targets.
Suggestions for improvement:
- Implement the fallback runner strategy for all jobs to ensure workflow portability.
- Consider adding a step to validate the generated bindings before creating the release.
- Add comments to explain any complex steps or decisions in the workflow for better maintainability.
Once the runner label issue is addressed, this workflow will be a solid foundation for your Swift bindings release process.
🧰 Tools
🪛 actionlint
6-6: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
42-42: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
64-64: label "warp-macos-13-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
A rustls dependency upgrade that broke android + swift tests. Originally thought it was an openssl issue, but openssl was completely unrelated so i removed all the openssl pins (the patch we had was never being applied). The only dep using openssl is diesel (and it is vendored).
I pinned rustls and tonic to the versions present in main before the wasm PR and it works again. Probably, android is very fragile about the webpki version we use. We should consider moving over to rustls-platform-verifier at some point
CI builds should be alot faster using native dependencies + use the same rust cache (
/targetfolder), and so much easier to test locally. It was invaluable being able to quickly build the jniLibs and copy them over/add logging/repeat. I'm leaving the old cross builds just in case, but pushing new workflows with a -nix ending. These have the benefit of being able to run./dev/release-kotlin/&./dev/release-swiftin libxmtp and create the jni/swift artifacts locally & quickly for testing, but do require installing nix which is included in the script (the nix version curated by determinate systems).If you'd rather not use nix, you can also
cargo install cargo-ndkand run the command locally, but there's no guarantee of the same environment as CI. In my experience this hasn't mattered much, unless there are non-standard/unexpected sqlite/openssl versions on the system.swiftartifacts were already possible to build without any magic &cargo, as long as a mac machine is used. A nix build is still used there just for consistency and assurance of envSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Cargo.tomlfiles to improve compatibility and functionality.