Fix OSX15 build - #182
Conversation
…as the Expander fix has been merged
There was a problem hiding this comment.
Summary of Changes
Hello @alxkzmn, 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 primarily focuses on resolving build failures on OSX15 by updating the project's Rust dependencies. The most significant change involves re-sourcing the mpi crate from a specific Git repository, which necessitated a corresponding type update in the codebase. Numerous other dependencies have also been upgraded, and the internal crate references have been synchronized.
Highlights
- Dependency Updates: This PR updates various Rust crate dependencies in
Cargo.lockto resolve potential build and compatibility issues, particularly on OSX15. - MPI Crate Source Change: The
mpicrate and its dependencies have been switched fromcrates.ioto a specific commit in thersmpi/rsmpiGit repository to address platform-specific build problems. - MPI Type Alignment: A code adjustment was made in
server_ctrl.rsto update an MPI-related type fromMPI_Wintoompi_win_t, ensuring compatibility with the newly sourcedmpicrate. - Internal Crate Synchronization: The commit hashes for several internal crates have been updated, reflecting a synchronization with the latest state of the
Expanderrepository's main branch. - Dependency Graph Refinement: Several transitive dependencies have been removed, contributing to a potentially leaner and more stable dependency graph.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix the build on macOS 15 by updating dependencies, most notably pointing the mpi crate to a specific git revision containing a fix. The changes look reasonable to address the build issue.
I have a couple of suggestions to improve long-term maintainability:
- In
Cargo.toml, I recommend adding aTODOto track reverting thempidependency back to acrates.ioversion once the fix is released. - In
server_ctrl.rs, the MPI window type has been changed to an implementation-specific one (ompi_win_t). This harms portability. I've suggested adding a comment to document this workaround and its implications.
Overall, good job on fixing the build!
| ] } | ||
| hex = "0.4" | ||
| mpi = "0.8.0" | ||
| mpi = { git = "https://github.com/rsmpi/rsmpi", rev = "61796831954b679cbe267c1b704ddbcb7fef3715" } |
There was a problem hiding this comment.
Using a git dependency is a good way to get an immediate fix, but it can be a maintenance challenge long-term. It's best to switch back to a version from crates.io once the fix for macOS 15 is released in a new version of the rsmpi crate.
I recommend adding a TODO comment here to track this. This will help ensure we don't forget to update it.
mpi = { git = "https://github.com/rsmpi/rsmpi", rev = "61796831954b679cbe267c1b704ddbcb7fef3715" } # TODO: Revert to a crates.io version once a release with the macOS 15 fix is available.
| pub static mut LOCAL_COMMUNICATOR: Option<SimpleCommunicator> = None; | ||
| pub struct SharedMemoryWINWrapper { | ||
| pub win: MPI_Win, | ||
| pub win: *mut ompi_win_t, |
There was a problem hiding this comment.
This change from MPI_Win to *mut ompi_win_t seems necessary to fix the build on macOS 15. However, ompi_win_t is specific to OpenMPI, which could cause portability issues if this code is ever built against other MPI implementations like MPICH.
If MPI_Win cannot be used, could you add a comment explaining why this implementation-specific type is necessary here? This would help future maintainers understand the context and potential limitations.
| pub win: *mut ompi_win_t, | |
| pub win: *mut ompi_win_t, // TODO: Revert to generic `MPI_Win` for portability once build issues are resolved. |
Related to PolyhedraZK/Expander#307 and PolyhedraZK/Expander#308
Original issue description: PolyhedraZK/Expander#307