Add support for osx-arm64 #50991
Add support for osx-arm64 #50991
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/panacus/meta.yaml (3 hunks)
🔇 Additional comments (4)
recipes/panacus/meta.yaml (4)
13-13: Build number increment looks good, but clarification needed.The build number has been incremented from 0 to 2, which is good practice when updating a package. However, could you please clarify why it was incremented by 2 instead of 1? Was there an intermediate build that's not reflected in this PR?
21-24: Build requirements for Rust look good.The addition of the Rust compiler as a build requirement and specifying a minimum Rust version (>=1.68) are appropriate changes. These additions will ensure that the package can be built correctly on the osx-arm64 platform and maintain consistency across different build environments.
48-49: Addition of osx-arm64 to additional-platforms is appropriate.The inclusion of 'osx-arm64' in the 'additional-platforms' list is consistent with the PR objective of adding support for the osx-arm64 platform. This change will ensure that the package is built and available for macOS systems using ARM64 architecture.
Line range hint
1-50: Overall, changes align well with PR objectives, with one point needing clarification.The modifications made to the
meta.yamlfile are generally consistent with the PR objective of adding support for the osx-arm64 platform. Key changes include:
- Incrementing the build number (clarification requested on the jump from 0 to 2).
- Adding Rust compiler requirements, which is appropriate for the target platform.
- Including osx-arm64 in the list of additional platforms.
These changes should enable the compilation and execution of Panacus on the osx-arm64 platform as intended. However, clarification is needed regarding the 'native_and_emulated' addition in the test section to ensure it's correctly implemented.
Once the points raised in the previous comments are addressed, this PR should be ready for approval.
recipes/panacus/meta.yaml
Outdated
| @@ -31,6 +32,7 @@ requirements: | |||
| - seaborn | |||
|
|
|||
| test: | |||
| native_and_emulated | |||
There was a problem hiding this comment.
💡 Codebase verification
Unrecognized 'native_and_emulated' test command in recipes/panacus/meta.yaml.
The native_and_emulated command in the test section does not appear to be a valid conda build test command based on existing documentation and usage in other recipes. Please verify if this is a custom command and ensure it is correctly implemented.
🔗 Analysis chain
Clarification needed on the 'native_and_emulated' test command.
The addition of 'native_and_emulated' in the test section is likely related to testing the package on both native and emulated environments, which is relevant for osx-arm64 support. However, the syntax of this line is unusual for a conda recipe test section. Could you please clarify:
- What does the 'native_and_emulated' command do?
- Is this the correct syntax for adding this test in a conda recipe?
- Should this be part of the 'commands:' subsection instead?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'native_and_emulated' is a valid conda build option or command
# Test 1: Search for 'native_and_emulated' in conda build documentation
echo "Searching for 'native_and_emulated' in conda build documentation:"
rg --type md "native_and_emulated" $(fd -t d "conda-build" .)
# Test 2: Check if 'native_and_emulated' is used in other recipe files
echo "Checking usage of 'native_and_emulated' in other recipe files:"
rg --type yaml "native_and_emulated" recipes/
Length of output: 441
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
recipes/panacus/build.sh (1)
Line range hint
1-7: Consider optimizing build settings and clarify environment variable usageWhile reviewing the entire script, I noticed a couple of points that might benefit from clarification or optimization:
The script uses
RUST_BACKTRACE=1. While this is helpful for debugging, it might produce unnecessary verbose output in a production build.
CARGO_HOMEis set to a path withinBUILD_PREFIX. This seems unusual and might be a workaround for some build system limitations.Consider the following suggestions:
Use
RUST_BACKTRACE=1only for debug builds:if [ "${DEBUG:-}" = 1 ]; then export RUST_BACKTRACE=1 fiClarify why
CARGO_HOMEis set to a path withinBUILD_PREFIX. If it's not necessary for all builds, consider making it conditional.Could you provide more context on these choices, especially if they're related to the osx-arm64 support?
recipes/panacus/build.sh
Outdated
| cp target/release/panacus $PREFIX/bin | ||
| cp scripts/panacus-visualize.py $PREFIX/bin/panacus-visualize | ||
| cp $PREFIX/target/release/panacus $PREFIX/bin | ||
| cp $PREFIX/scripts/panacus-visualize.py $PREFIX/bin/panacus-visualize |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: panacus-visualize.py is missing in the expected source paths.
- The source file
panacus-visualize.pywas not found at/scripts/panacus-visualize.pynor atscripts/panacus-visualize.py. This may indicate that the file has been deleted or moved without updating the build script accordingly.
🔗 Analysis chain
Verify the changes to panacus-visualize.py copying
Two changes have been made to the copying of panacus-visualize.py:
- The source path now includes
$PREFIX. - The destination filename has been changed from
panacus-visualize.pytopanacus-visualize.
Please confirm:
- Is the change in the source path necessary for all platforms or specific to osx-arm64?
- Is the removal of the
.pyextension intentional? If so, ensure that:- The script has the correct shebang line.
- All documentation and user instructions are updated to reflect this change.
To verify the existence of the file and its contents, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and contents of panacus-visualize.py
if [ -f "$PREFIX/scripts/panacus-visualize.py" ]; then
echo "panacus-visualize.py exists at $PREFIX/scripts/panacus-visualize.py"
echo "First line of the script:"
head -n 1 "$PREFIX/scripts/panacus-visualize.py"
else
echo "panacus-visualize.py not found at $PREFIX/scripts/panacus-visualize.py"
fi
# Check if the old path still exists
if [ -f "scripts/panacus-visualize.py" ]; then
echo "panacus-visualize.py also exists at scripts/panacus-visualize.py"
fi
Length of output: 221
recipes/panacus/build.sh
Outdated
| @@ -3,5 +3,5 @@ | |||
| RUST_BACKTRACE=1 CARGO_HOME="${BUILD_PREFIX}/.cargo" cargo build --release | |||
|
|
|||
| mkdir -p $PREFIX/bin | |||
| cp target/release/panacus $PREFIX/bin | |||
| cp scripts/panacus-visualize.py $PREFIX/bin/panacus-visualize | |||
| cp $PREFIX/target/release/panacus $PREFIX/bin | |||
There was a problem hiding this comment.
💡 Codebase verification
panacus executable not found at the specified source path
The cp command is attempting to copy the panacus executable from $PREFIX/target/release/panacus, but the file does not exist at this location. This will prevent the installation from succeeding.
- Ensure that the build process correctly generates the
panacusexecutable in$PREFIX/target/release/. - Verify that the build steps for the osx-arm64 platform are correctly configured to output the executable to the expected directory.
🔗 Analysis chain
Verify the necessity of changing the source path for panacus
The source path for copying the panacus executable has been changed to include $PREFIX. While this change might be related to supporting osx-arm64, it's not immediately clear why this is necessary.
Please confirm:
- Is this change specific to osx-arm64 or does it apply to all platforms?
- Does
$PREFIX/target/release/panacusalways exist after the build process?
To verify the existence of the file, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of panacus executable
if [ -f "$PREFIX/target/release/panacus" ]; then
echo "panacus executable exists at $PREFIX/target/release/panacus"
else
echo "panacus executable not found at $PREFIX/target/release/panacus"
fi
# Check if the old path still exists
if [ -f "target/release/panacus" ]; then
echo "panacus executable also exists at target/release/panacus"
fi
Length of output: 193
|
Thanks a lot, @mencian! |
Panacus compiles and runs on osx-arm64. This update brings panacus also to this plattform.
Summary by CodeRabbit
New Features
panacustool to include macOS systems with ARM64 architecture and Linux systems with ARM64 architecture.Chores
panacusrecipe.seaborndependency withseaborn-base.panacustool.panacusexecutable.