Build strelka on osx #52208
Build strelka on osx #52208
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the configuration and build process of 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
recipes/strelka/meta.yaml (1)
21-31: Consider tightening Python version constraintWhile
python <3works, it might be better to specify a more precise version range likepython >=2.7,<3to ensure compatibility with older Python 2.7 versions.run: - - python <3 + - python >=2.7,<3 - boost-cpprecipes/strelka/build.sh (3)
8-11: Initialize compiler flags to prevent leading spacesIf
LDFLAGSorCXXFLAGSare not set in the environment, the exported variables may begin with a leading space, which could cause issues in some build systems. Initialize them to empty strings if they're unset to avoid potential problems.Apply the following changes:
-export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" -export CXXFLAGS="${CXXFLAGS} -O3 -I${PREFIX}/include" +export LDFLAGS="${LDFLAGS:-} -L${PREFIX}/lib" +export CXXFLAGS="${CXXFLAGS:-} -O3 -I${PREFIX}/include"
13-13: Use$(...)instead of backticks for command substitutionFor improved readability and to follow modern Bash best practices, consider using
$(...)instead of backticks`...`for command substitution.Apply this change:
-if [[ `uname` == "Darwin" ]]; then +if [[ $(uname) == "Darwin" ]]; then
26-33: Remove obsolete commented-out code to enhance maintainabilityThe code between lines 26 and 33 is commented out and no longer in use. Removing obsolete code helps keep the script clean and maintainable.
Apply this change:
-#cp -r bin lib libexec share $outdir -#sed -i.bak \ -# -e "s~scriptDir=.*~scriptDir='$outdir/bin'~" \ -# -e "s~workflowDir=.*~workflowDir='$outdir/lib/python'~" \ -# $outdir/bin/configure*.py -#sed -i.bak \ -# -e "s~scriptDir=.*~scriptDir='$outdir/bin'~" \ -# $outdir/bin/run*.bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
recipes/strelka/boost.cmake.patch(1 hunks)recipes/strelka/build.sh(1 hunks)recipes/strelka/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/strelka/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/strelka/boost.cmake.patch (1)
1-24: Verify OSX compatibility with these changes
While disabling static linking (Boost_USE_STATIC_LIBS OFF) is a good step for OSX compatibility, we should verify that these CMake changes work correctly on OSX.
✅ Verification successful
Boost configuration changes are appropriate for OSX compatibility
The changes in the patch are well-aligned with OSX build requirements:
- Disabling static linking (
Boost_USE_STATIC_LIBS OFF) is the correct approach for OSX compatibility - Adding explicit include and link directory handling via
include_directories(${Boost_INCLUDE_DIRS})andLINK_DIRECTORIES(${Boost_LIBRARY_DIRS})provides proper library resolution - The multithreaded setting (
Boost_USE_MULTITHREADED TRUE) is compatible with OSX's threading model
The lack of conflicting OSX-specific Boost configurations in the codebase suggests these changes won't interfere with existing platform-specific handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any OSX-specific Boost configurations or known issues
# that might affect the build.
# Search for any OSX-specific Boost configurations
rg -l "darwin|apple|osx" --type cmake
# Look for existing Boost configurations that might conflict
ast-grep --pattern 'set.*Boost_USE_STATIC_LIBS'
Length of output: 89
Script:
#!/bin/bash
# Let's try a broader search to understand the build system configuration
# Check for platform-specific handling in CMake files
rg -A 5 -B 5 "IF.*APPLE|IF.*Darwin|APPLE|Darwin" --type cmake
# Look for any Boost-related configurations
rg -A 3 "Boost_" --type cmake
# Check if there are any other patches or configuration files for OSX
fd -e patch -e cmake -x grep -l "darwin|apple|osx|boost" {}
Length of output: 833
recipes/strelka/meta.yaml (5)
1-3: LGTM! Good use of Jinja2 variables
The package metadata is well-structured using Jinja2 variables, making it easier to maintain and update.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
16-18: LGTM! Good ABI compatibility practice
The run_exports section with pin_subpackage helps prevent ABI breakage, which aligns with the PR objectives.
39-45: LGTM! Well-documented metadata
The metadata section is complete with proper license information and helpful URLs for documentation and development.
49-55: LGTM! Good platform coverage and identifiers
The addition of arm64 platforms and comprehensive identifiers enhances the package's discoverability and platform support. The inclusion of osx-arm64 aligns well with the PR's objective of OSX support.
10-13: Verify the boost.cmake patch file
The patch file is crucial for OSX support. Let's verify its existence and contents.
✅ Verification successful
Patch file verified and contains appropriate Boost configuration changes
The patch file exists and modifies src/cmake/boost.cmake with necessary changes for OSX compatibility:
- Disables static linking (
Boost_USE_STATIC_LIBS OFF) - Enables multi-threading support
- Adds debug mode
- Includes proper directory linking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the patch file exists and examine its contents
# Expected: Find the patch file and see Boost-related configurations
# Check if patch file exists
if [ -f "recipes/strelka/boost.cmake.patch" ]; then
echo "Patch file found"
cat recipes/strelka/boost.cmake.patch
else
echo "ERROR: Patch file missing"
fi
Length of output: 1199
| - set (Boost_USE_STATIC_LIBS ON) | ||
| + set( Boost_USE_STATIC_LIBS OFF ) | ||
| + set( Boost_USE_MULTITHREADED TRUE ) | ||
| + set( Boost_DEBUG ON ) |
There was a problem hiding this comment.
Review multithreaded configuration and debug settings
There are several concerns with the current configuration:
-
There's a conflict in multithreaded settings:
- Line 11 sets
Boost_USE_MULTITHREADED TRUEglobally - Line 15 sets it
OFFfor non-Windows platforms
This contradiction needs to be resolved.
- Line 11 sets
-
Boost_DEBUG ONshould typically be disabled in production builds as it may impact build performance.
Consider the following changes:
- set( Boost_USE_STATIC_LIBS OFF )
- set( Boost_USE_MULTITHREADED TRUE )
- set( Boost_DEBUG ON )
+ set(Boost_USE_STATIC_LIBS OFF)
+ # Only enable multithreading for Windows
+ if (WIN32)
+ set(Boost_USE_MULTITHREADED TRUE)
+ else()
+ set(Boost_USE_MULTITHREADED OFF)
+ endif()
+ # Enable debug only for development/testing
+ if (CMAKE_BUILD_TYPE STREQUAL "Debug")
+ set(Boost_DEBUG ON)
+ endif()Also applies to: 15-15
| + include_directories(${Boost_INCLUDE_DIRS}) | ||
| + LINK_DIRECTORIES(${Boost_LIBRARY_DIRS}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use modern CMake target-based approach
The current global include and link directory modifications don't follow modern CMake best practices. These commands affect all targets in scope and may cause unnecessary dependencies.
Consider using target-specific commands instead:
- include_directories(${Boost_INCLUDE_DIRS})
- LINK_DIRECTORIES(${Boost_LIBRARY_DIRS})
+ # Create an interface library for Boost dependencies
+ add_library(boost_deps INTERFACE)
+ target_include_directories(boost_deps INTERFACE ${Boost_INCLUDE_DIRS})
+ target_link_libraries(boost_deps INTERFACE ${Boost_LIBRARIES})
+
+ # Link against specific targets that need Boost
+ target_link_libraries(your_target PRIVATE boost_deps)Committable suggestion skipped: line range outside the PR's diff.
| #sed -i.bak \ | ||
| # -e "s~scriptDir=.*~scriptDir='$outdir/bin'~" \ | ||
| # $outdir/bin/run*.bash | ||
| ln -sf $outdir/bin/config*.py $outdir/bin/run*.bash $PREFIX/bin |
There was a problem hiding this comment.
Ensure glob patterns match existing files to prevent errors
If the glob patterns config*.py or run*.bash do not match any files, the ln command may create unintended symlinks or fail. To prevent this, check if the files exist before creating the symlinks.
Modify the command as follows:
# Create symlinks only if matching files exist
for file in $outdir/bin/config*.py $outdir/bin/run*.bash; do
if [[ -e "$file" ]]; then
ln -sf "$file" "$PREFIX/bin"
fi
done
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>.