-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rebuild regenie recipe #52074
Rebuild regenie recipe #52074
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Possibly related PRs
Suggested labels
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/regenie/build.sh (1)
20-21
: Consider consolidating include path settingsThe include path is set in multiple variables (INCLUDES, CXXFLAGS, CFLAGS, and CPATH). While this works, it might be cleaner to use a single variable that others reference.
-export INCLUDES="-I${PREFIX}/include" -export CFLAGS="${CFLAGS} -O3 -I$PREFIX/include" -export CPATH="${PREFIX}/include" +export INCLUDES="-I${PREFIX}/include" +export CFLAGS="${CFLAGS} -O3 ${INCLUDES}" +export CPATH="${PREFIX}/include"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
recipes/regenie/build.sh
(1 hunks)recipes/regenie/build_failure.osx-64.yaml
(0 hunks)recipes/regenie/conda_build_config.yaml
(0 hunks)recipes/regenie/meta.yaml
(4 hunks)recipes/regenie/patches/0001-use-shared-cxxopts.patch
(0 hunks)recipes/regenie/patches/0002-import-shared-Eigen3.patch
(0 hunks)recipes/regenie/patches/0003-use-conda-cmakelists.patch
(0 hunks)recipes/regenie/patches/0004-fix-fail-exit-on-help.patch
(0 hunks)recipes/regenie/patches/0005-use-conda-cmakelist-file.patch
(0 hunks)recipes/regenie/patches/0006-fix-cmakelist-file.patch
(0 hunks)recipes/regenie/patches/0007-fix-cmakelist-file-for-osx.patch
(0 hunks)recipes/regenie/patches/0008-fix-cmakelist-file-for-osx-eigen4.patch
(0 hunks)recipes/regenie/patches/0009-update-cmake-file-for-conda.patch
(0 hunks)recipes/regenie/patches/0010-update-cmakelist-file-for-v3.2.7.patch
(0 hunks)recipes/regenie/patches/0011-update-cmakelist-file-for-v3.2.9.patch
(0 hunks)recipes/regenie/patches/0012-update-cmakelist-file-for-v3.4.patch
(0 hunks)recipes/regenie/patches/0013-update-cmakelist-file-for-v3.4.1.patch
(0 hunks)recipes/regenie/patches/0014-update-cmakelist.txt-for-v3.5.patch
(0 hunks)
💤 Files with no reviewable changes (16)
- recipes/regenie/build_failure.osx-64.yaml
- recipes/regenie/conda_build_config.yaml
- recipes/regenie/patches/0001-use-shared-cxxopts.patch
- recipes/regenie/patches/0002-import-shared-Eigen3.patch
- recipes/regenie/patches/0003-use-conda-cmakelists.patch
- recipes/regenie/patches/0004-fix-fail-exit-on-help.patch
- recipes/regenie/patches/0005-use-conda-cmakelist-file.patch
- recipes/regenie/patches/0006-fix-cmakelist-file.patch
- recipes/regenie/patches/0007-fix-cmakelist-file-for-osx.patch
- recipes/regenie/patches/0008-fix-cmakelist-file-for-osx-eigen4.patch
- recipes/regenie/patches/0009-update-cmake-file-for-conda.patch
- recipes/regenie/patches/0010-update-cmakelist-file-for-v3.2.7.patch
- recipes/regenie/patches/0011-update-cmakelist-file-for-v3.2.9.patch
- recipes/regenie/patches/0012-update-cmakelist-file-for-v3.4.patch
- recipes/regenie/patches/0013-update-cmakelist-file-for-v3.4.1.patch
- recipes/regenie/patches/0014-update-cmakelist.txt-for-v3.5.patch
🔇 Additional comments (8)
recipes/regenie/build.sh (3)
6-8
: LGTM: Environment variable setup follows conda-build best practices
The environment variables are correctly configured with proper prefix paths and optimization flags.
24-31
: LGTM: CMake configuration follows best practices
The CMake configuration is well-structured with:
- Modern CMake syntax
- Proper compiler and flag handling
- Parallel build support
- Correct Boost configuration
10-17
: Verify MKL configuration for non-Darwin platforms
The MKL threading layer is set to "GNU" for non-Darwin platforms. This is generally correct for conda-forge builds, but we should verify that regenie actually uses MKL.
✅ Verification successful
MKL configuration for non-Darwin platforms verified
The package uses MKL, and the MKL_THREADING_LAYER is appropriately set to "GNU".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the package actually uses MKL
rg -l "mkl" --type cmake
rg -l "MKL" --type cmake
# Check for MKL-related dependencies in meta.yaml
if [ -f meta.yaml ]; then
echo "Checking meta.yaml for MKL dependencies:"
grep -i "mkl" meta.yaml
fi
Length of output: 72
Script:
#!/bin/bash
# Search for 'MKL' in all source and configuration files
rg -l "MKL" --type cpp --type h --type py --type sh --type yaml --type cmake
# Additionally, search for 'intel' which might be related to MKL
rg -l "intel" --type cpp --type h --type py --type sh --type yaml --type cmake
Length of output: 771
recipes/regenie/meta.yaml (5)
16-18
: LGTM: Build configuration changes look good
The build number increment and addition of run_exports
with appropriate version pinning will help maintain compatibility with downstream recipes, as mentioned in the PR objectives.
39-43
: Platform-specific OpenMP dependencies look good
The addition of platform-specific OpenMP libraries (llvm-openmp
for macOS, libgomp
for Linux) is correctly configured and matches the host requirements.
52-65
: Documentation and metadata improvements look good
The addition of the DOI identifier and cleaned-up documentation URLs enhance the package metadata, making it more discoverable and citable in academic contexts.
Line range hint 1-65
: Verify bioconda guidelines compliance
Please ensure this recipe update aligns with bioconda guidelines, particularly:
- Package versioning and compatibility with existing bioconda packages
- Dependency availability in the bioconda or conda-forge channels
#!/bin/bash
# Check if dependencies are available in bioconda/conda-forge
# and if there are any potential conflicts
# Check for boost-cpp availability in conda-forge
curl -s https://conda.anaconda.org/conda-forge/noarch/repodata.json | jq '.packages | with_entries(select(.key | contains("boost-cpp-1.74")))'
# Check for potential conflicts in other bioconda recipes
rg -g "meta.yaml" "boost-cpp.*1\.74"
Line range hint 28-38
: Verify MKL version constraints and dependencies
The host requirements use MKL-specific configurations:
liblapack * *mkl
uses MKL variantmkl >=2020.4
specifies minimum version
Please verify if these version constraints are sufficient for the current regenie version's requirements.
✅ Verification successful
MKL version constraints for regenie are appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any other bioconda recipes use different MKL version constraints with regenie
rg -g "meta.yaml" "regenie.*mkl"
Length of output: 380
Script:
#!/bin/bash
# Search the regenie meta.yaml for explicit MKL version requirements
rg -g "meta.yaml" "mkl\s*>=?\s*"
Length of output: 254
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add label
command.@bioconda/core
in 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_exports
section 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 eithername
if aname|lower
variable 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 label
please review & merge
label.@BiocondaBot please fetch artifacts
You can use this to test packages locally.
Note that the
@BiocondaBot please merge
command 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>
.