gnparser: add aarch64/arm64 builds #51376
Conversation
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
added
aarch64
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
LGTM! |
|
@BiocondaBot please add label |
📝 WalkthroughWalkthroughThe pull request introduces modifications to two files: Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
recipes/gnparser/build.sh (2)
3-3: Excellent addition of error handling and verbosity.The addition of
set -xeis a great improvement to the build script. It enhances error detection and provides better visibility into the build process. This change aligns well with best practices for shell scripting in build environments.Consider adding a comment explaining the purpose of these flags for future maintainers. For example:
+# Enable verbose mode and exit on any error set -xeThis small addition would improve the script's readability and maintainability.
Line range hint
1-11: Consider enhancing error handling and portability.While the current script works, there are a few areas where it could be made more robust:
- Add a check for the
gnparserdirectory's existence.- Export the
CGO_ENABLEDvariable to ensure it affects subprocesses.- Add a check for successful build before moving the binary.
- Consider adding a check for the
$PREFIXvariable.Here's a suggested improvement:
#!/bin/bash # Enable verbose mode and exit on any error set -xe +# Ensure PREFIX is set +: "${PREFIX:?PREFIX must be set}" + +# Check if gnparser directory exists +if [ ! -d "gnparser" ]; then + echo "Error: gnparser directory not found" >&2 + exit 1 +fi + cd gnparser -CGO_ENABLED=0 +export CGO_ENABLED=0 go clean go build . + +# Check if build was successful +if [ ! -f "gnparser" ]; then + echo "Error: Build failed, gnparser binary not found" >&2 + exit 1 +fi + mkdir -p $PREFIX/bin mv gnparser $PREFIX/binThese changes will make the script more robust and easier to debug if issues arise.
🧰 Tools
🪛 Shellcheck
[warning] 6-6: CGO_ENABLED appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/gnparser/build.sh (1 hunks)
- recipes/gnparser/meta.yaml (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
recipes/gnparser/meta.yaml (4)
14-14: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 0 to 1. This is the expected practice when making changes to a package without altering its version, which is the case here with the addition of new platforms.
Line range hint
1-42: Summary: All changes look good and align with PR objectives.The modifications to
meta.yamlare well-implemented and consistent with the PR's goal of adding aarch64/arm64 builds. Key changes include:
- Updating the version to 1.10.3 with corresponding SHA256 and URL updates.
- Incrementing the build number.
- Adding
linux-aarch64andosx-arm64to the additional platforms.These changes will expand the compatibility of the
gnparserpackage to ARM-based systems while maintaining the integrity of the package. The review process included verification steps for build artifacts and the SHA256 checksum to ensure the changes are correctly implemented.
37-39: LGTM: New platforms added correctly.The addition of
linux-aarch64andosx-arm64platforms is in line with the PR objective and expands the package's compatibility to ARM-based systems. This is a valuable improvement.To ensure these changes are reflected in the build process, let's verify the build artifacts:
#!/bin/bash # Description: Verify that build artifacts for the new platforms are generated. # Test: Check if build artifacts for linux-aarch64 and osx-arm64 are present gh pr view 51376 --json comments -q '.comments[].body' | grep -E 'linux-aarch64|osx-arm64'
Line range hint
1-11: LGTM: Version update is consistent and complete.The version has been correctly updated to "1.10.3" across all relevant fields, including the version number, SHA256 checksum, and source URL. This ensures that the correct version of the package will be downloaded and built.
To ensure the integrity of the package, let's verify the SHA256 checksum:
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.