Fix spdlog dependency order error - #2
Conversation
Co-authored-by: izzynochill <izzynochill@gmail.com>
Co-authored-by: izzynochill <izzynochill@gmail.com>
|
Cursor Agent can help with this pull request. Just |
There was a problem hiding this comment.
Summary of Changes
Hello @izzy2lost, 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!
I've addressed a build error related to the spdlog dependency for Android builds. The issue stemmed from spdlog being declared twice with conflicting versions in different CMake files. This change removes the redundant declaration in the main CMakeLists.txt to ensure libultraship correctly manages the spdlog dependency.
Highlights
- Dependency Resolution: I've resolved a build error for Android by removing a duplicate
spdlogdependency declaration. - CMake Refinement: Specifically, I removed the
FetchContentblock forspdlog(version 1.13.0) from the mainCMakeLists.txtfile, which was causing conflicts with another declaration (version 1.14.1) inlibultraship/cmake/dependencies/android.cmake. - Include Path Cleanup: As a result of removing the duplicate declaration, I also removed the corresponding
spdloginclude directory from the globalinclude_directorieslist inCMakeLists.txt.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Co-authored-by: izzynochill <izzynochill@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request correctly removes a duplicate spdlog dependency declaration for Android builds and a related hardcoded include path. This helps resolve build errors caused by conflicting versions. My review includes a comment on a potential remaining versioning issue for spdlog on Android that you might want to look into to ensure the intended version is being used.
|
|
||
| # Fetch spdlog for Android - must be done before torch and libultraship | ||
| # Use the same version as libultraship to avoid conflicts | ||
| FetchContent_Declare( | ||
| spdlog | ||
| GIT_REPOSITORY https://github.com/gabime/spdlog.git | ||
| GIT_TAG v1.13.0 | ||
| GIT_TAG v1.14.1 | ||
| ) | ||
| FetchContent_MakeAvailable(spdlog) | ||
| set(SPDLOG_INCLUDE_DIRS ${spdlog_SOURCE_DIR}/include ${spdlog_BINARY_DIR} CACHE INTERNAL "spdlog include paths") | ||
| include_directories(${SPDLOG_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Removing this duplicate spdlog declaration is the correct approach to fix the build errors on Android.
However, there might be a lingering issue with the spdlog version being used. libultraship/cmake/dependencies/common.cmake declares spdlog v1.13.0, and it's included before libultraship/cmake/dependencies/android.cmake, which declares v1.14.1. Because of the if (NOT ${spdlog_FOUND}) check in android.cmake, the v1.14.1 declaration will be skipped, and Android will end up using v1.13.0 from common.cmake.
If the intention is for Android to use v1.14.1, you might need to adjust the logic in libultraship's CMake files, for example by removing the spdlog handling from common.cmake when building for Android.
There was a problem hiding this comment.
Try to build in GitHub actions with android compile
There was a problem hiding this comment.
@cursoragent try to build in GitHub actions with the android-compile.yml
There was a problem hiding this comment.
I tried to trigger the GitHub Actions workflow for the Android build, but after several attempts using the GitHub CLI (which I had to install and authenticate), I discovered that the access token I was using didn't have the necessary permissions to trigger workflows directly. I was able to list workflows and see their statuses, confirming that recent Android builds had failed.
Since I couldn't trigger the build, I've ensured that the changes I made to fix the spdlog dependency order are committed and pushed. These changes involve:
- Modifying
CMakeLists.txt: I added thespdlogdeclaration for Android beforetorchandlibultrashipare included, using versionv1.14.1to matchlibultraship's version. This ensuresspdlogis available whentorchtries to find it. - Updating
libultraship/cmake/dependencies/android.cmake: I added target checks to prevent duplicatespdlogdeclarations and preserved theSDL2linking forImGui.
Because I can't trigger the workflow myself, you'll need to manually trigger the "Android Build" workflow from your GitHub repository's "Actions" tab, selecting the cursor/fix-spdlog-dependency-order-error-ba4a branch. Alternatively, you can push a small, empty commit to trigger the workflow. The build should now succeed as spdlog is declared correctly, torch will find it, and libultraship will skip its own declaration, preventing version conflicts.
Learn more about Cursor Agents
Remove duplicate spdlog declaration to fix Android dependency ordering issues.
The problem was that spdlog was declared twice with conflicting versions (v1.13.0 in the main CMakeLists.txt and v1.14.1 in libultraship/cmake/dependencies/android.cmake), leading to build errors. This change ensures libultraship correctly manages the spdlog dependency for Android builds.