Fixed CI errors on x86 (hopefully)#64
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses and aims to resolve continuous integration errors on x86 platforms by standardizing the architecture naming convention across various configuration files and documentation. The changes ensure consistency in how x86-based systems are referenced, particularly in build environments and installation instructions, which should lead to more reliable CI outcomes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request updates the CI workflow to use Python 3.13.12, changes build architecture targets from x86_64 to x86 across multiple CI and Docker configurations, and updates corresponding documentation and wheel filename references to reflect these changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix CI errors on x86 platforms but introduces critical issues that will likely break builds and user installations. The changes in both Dockerfiles point to a non-existent Android NDK toolchain directory, which will cause the Docker builds to fail. Additionally, the README file has been updated with a URL to a Linux wheel with an invalid platform tag, which will prevent users from installing the package. These changes seem to originate from a misunderstanding of the x86 vs x86_64 architecture identifiers and should be reverted or corrected.
| ENV ANDROID_SDK_ROOT=/opt/android-sdk | ||
| ENV ANDROID_NDK_HOME=$ANDROID_SDK_ROOT/ndk/$ANDROID_NDK_VERSION | ||
| ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86_64/bin:$PATH" | ||
| ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86/bin:$PATH" |
There was a problem hiding this comment.
The path to the Android NDK toolchain is incorrect. The Android NDK for Linux is distributed for 64-bit hosts, and the prebuilt toolchain directory is named linux-x86_64. The path .../linux-x86/bin does not exist in the NDK r26d distribution. This will cause the Docker build to fail when it cannot find the toolchain binaries.
ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86_64/bin:$PATH"
| ENV ANDROID_SDK_ROOT=/opt/android-sdk | ||
| ENV ANDROID_NDK_HOME=$ANDROID_SDK_ROOT/ndk/$ANDROID_NDK_VERSION | ||
| ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86_64/bin:$PATH" | ||
| ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86/bin:$PATH" |
There was a problem hiding this comment.
The path to the Android NDK toolchain is incorrect. The Android NDK for Linux is distributed for 64-bit hosts, and the prebuilt toolchain directory is named linux-x86_64. The path .../linux-x86/bin does not exist in the NDK r26d distribution. This will cause the Docker build to fail when it cannot find the toolchain binaries.
ENV PATH="$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86_64/bin:$PATH"
|
|
||
| ```bash | ||
| pip install "https://github.com/ChipaDevTeam/BinaryOptionsTools-v2/releases/download/BinaryOptionsToolsV2-0.2.8/BinaryOptionsToolsV2-0.2.8-cp38-abi3-manylinux_2_34_x86_64.whl" | ||
| pip install "https://github.com/ChipaDevTeam/BinaryOptionsTools-v2/releases/download/BinaryOptionsToolsV2-0.2.8/BinaryOptionsToolsV2-0.2.8-cp38-abi3-manylinux_2_34_x86.whl" |
There was a problem hiding this comment.
The updated wheel filename for Linux uses an invalid platform tag. The tag manylinux_2_34_x86 is not a standard Python wheel tag. For 64-bit Linux, the tag should be manylinux_..._x86_64. This incorrect URL will lead to a 404 error for users trying to install the package on Linux by following the documentation.
| pip install "https://github.com/ChipaDevTeam/BinaryOptionsTools-v2/releases/download/BinaryOptionsToolsV2-0.2.8/BinaryOptionsToolsV2-0.2.8-cp38-abi3-manylinux_2_34_x86.whl" | |
| pip install "https://github.com/ChipaDevTeam/BinaryOptionsTools-v2/releases/download/BinaryOptionsToolsV2-0.2.8/BinaryOptionsToolsV2-0.2.8-cp38-abi3-manylinux_2_34_x86_64.whl" |
| - Tests often skip without live credentials; use mocking where possible | ||
| - Python tests use fixtures from `conftest.py`; module-scoped fixtures for connection reuse | ||
| - CI builds wheels for Linux (manylinux, musllinux), Windows, macOS; runs pytest on x86_64 | ||
| - CI builds wheels for Linux (manylinux, musllinux), Windows, macOS; runs pytest on x86 |
There was a problem hiding this comment.
The term x86 is ambiguous and can lead to confusion. It can refer to 32-bit architecture or the entire family of architectures including 64-bit. For clarity, it's best practice to use specific identifiers like x86_64 for 64-bit systems. Given that other parts of the project target x86_64, this change introduces inconsistency and could mislead developers about the CI environment.
| - CI builds wheels for Linux (manylinux, musllinux), Windows, macOS; runs pytest on x86 | |
| - CI builds wheels for Linux (manylinux, musllinux), Windows, macOS; runs pytest on x86_64 |
1 participant
Fixed CI errors on x86
Summary by CodeRabbit