update khmer #49276
update khmer #49276
Conversation
|
oops. looks like i duplicated this in PR #49350. maybe this PR should be closed? |
|
closing in favor of the merged #49350 |
|
reopening, since it looks like we'll need it |
|
tests passed, but it didn't actually build anything. |
This reverts commit d668c95. it failed. reverting
|
i have a hunch that the problem is with the patch. the highest release that the khmer github page lists is 2.1.2. i'm not sure where the 3.0+ versions are coming from other than the patch. |
|
I made PR #49494 just to update the build failure file (recipes/khmer/build_failure.linux-64.yaml) on master so it doesn't try to build it on the nightly in the current broken state, but that build_failure file can be removed on this branch once my PR is merged to master and then this branch updated from master. |
|
not sure why this is failing, but it looks like a certain folder ( |
|
giving up for now. |
📝 WalkthroughWalkthroughThe pull request introduces several changes to the Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (4)
recipes/khmer/build.sh (2)
27-32: Improve installation verification and debuggingThe current debugging approach could be more informative and the installation success isn't properly verified.
Consider these improvements:
-# install khmer -$PYTHON -m pip install . --no-deps --no-build-isolation --no-cache-dir --use-pep517 -vvv +# Install khmer and verify success +if ! $PYTHON -m pip install . --no-deps --no-build-isolation --no-cache-dir --use-pep517 -vvv; then + echo "Error: Failed to install khmer package" + exit 1 +fi -# debugging line to find where khmer was installed -echo "here i am! ⊂◉‿◉つ " -ls -lhR +# Verify installation +echo "Verifying khmer installation..." +if ! $PYTHON -c "import khmer; print(f'Successfully installed khmer {khmer.__version__}')"; then + echo "Error: Failed to import installed khmer package" + exit 1 +fi + +# List only relevant directories +echo "Installation contents:" +ls -lh $PREFIX/{bin,lib,include}
1-32: Consider modernizing the build system approachBased on the PR objectives and the current implementation, there are several architectural considerations:
- The current approach of manually copying files and managing paths could be replaced with modern build system tools like
cmakeormeson- The bundled dependencies strategy (seqan, zlib) should be replaced with proper conda dependencies as mentioned in the PR description
- The build script should be integrated with conda's build system to properly handle
run_exportsConsider these improvements:
- Move to a modern build system tool
- Replace bundled dependencies with conda packages
- Add proper version constraints in meta.yaml
- Implement proper ABI compatibility checks
Would you like assistance in implementing any of these architectural changes?
recipes/khmer/meta.yaml (2)
27-46: Review dependency management strategy.The changes to dependencies look good and address the issues mentioned in the PR:
- Explicit bzip2 and zlib dependencies in build/host/run sections
- Platform-specific OpenMP support (
llvm-openmpfor macOS,libgompfor Linux)- Updated Python package dependencies
Consider adding version constraints for critical dependencies like
bzip2andzlibto ensure ABI compatibility.
Line range hint
1-60: Summary of key points requiring attention.
- The Python 3.12 compatibility goal needs clarification, as it conflicts with the skip condition.
- The dependency management changes look good, moving from bundled to conda-managed dependencies.
- The version downgrade from 3.0.0a3 to 2.1.1 needs verification.
- Consider adding version constraints for system libraries.
After addressing these points, please ensure thorough testing of the package build and runtime behavior, particularly focusing on the zlib and bzip2 integration since these were previously causing issues.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
recipes/khmer/0002-do-not-install-dependencies-on-our-own.patch(1 hunks)recipes/khmer/0003-remove-dependencies-from-config.patch(1 hunks)recipes/khmer/0004-remove-dependencies-from-manifest.patch(1 hunks)recipes/khmer/build.sh(1 hunks)recipes/khmer/meta.yaml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- recipes/khmer/0002-do-not-install-dependencies-on-our-own.patch
- recipes/khmer/0004-remove-dependencies-from-manifest.patch
🧰 Additional context used
🪛 yamllint
recipes/khmer/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/khmer/0003-remove-dependencies-from-config.patch (2)
9-10: LGTM! Removing bundled dependencies in favor of system libraries.
The change to use system libraries instead of bundled ones aligns with the PR objective and should help resolve the undefined symbol issues encountered during builds.
10-10: Verify the new seqan include path.
The path to seqan has changed from third-party/seqan/core/include to include/seqan. Let's verify this path exists and contains the required headers.
recipes/khmer/meta.yaml (3)
55-60: Verify license change.
The license has been updated from "BSD License" to "BSD-3-Clause". This change requires verification against the source repository.
✅ Verification successful
License change is correct
The license in the source repository's LICENSE file explicitly states "License: BSD-3-Clause" and includes the standard BSD-3-Clause license text. This confirms that the license change in meta.yaml is accurate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify license in source repository
# Expected: BSD-3-Clause license
# Check license file in repository
gh api repos/dib-lab/khmer/contents/LICENSE
Length of output: 7075
13-15: Verify patch contents for dependency management.
The patches appear to address the dependency management issues mentioned in the PR objectives:
- Not installing dependencies during build
- Removing dependencies from config
- Removing dependencies from manifest
These changes align with the goal of using conda-managed dependencies instead of bundled ones.
✅ Verification successful
Patches correctly implement dependency management changes
The patches effectively implement the intended changes to use conda-managed dependencies:
0002-do-not-install-dependencies-on-own.patch: Disables bundled zlib and bzip2 compilation by commenting out directory definitions and preventing their build0003-remove-dependencies-from-config.patch: Updates include paths to use system libraries instead of bundled ones0004-remove-dependencies-from-manifest.patch: Removes bundled third-party dependencies from package manifest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check patch contents
# Expected: Patches modify dependency handling
for patch in recipes/khmer/*.patch; do
echo "=== $patch ==="
cat "$patch" || echo "Patch not found: $patch"
done
Length of output: 3832
1-3: Verify version downgrade and checksum.
The version has been downgraded from 3.0.0a3 to 2.1.1. This is a significant change that requires verification.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
| # populate compiling directories | ||
| cp -v lib/* $PREFIX/include | ||
| cp -v scripts/* $PREFIX/bin | ||
|
|
||
| # copies over third-party/seqan/core/include/seqan | ||
| mkdir -pv $PREFIX/include/seqan | ||
| cp -vr third-party/seqan/core/include/seqan/* $PREFIX/include/seqan/ |
There was a problem hiding this comment.
Fix incorrect file copying operations and add error handling
There are several issues with the file copying operations:
- Copying
lib/*toincludedirectory seems incorrect - library files should go tolib - No verification that source files exist before copying
- Missing error handling for failed copies
Consider this improved implementation:
-cp -v lib/* $PREFIX/include
-cp -v scripts/* $PREFIX/bin
+# Verify source directories exist
+for dir in lib scripts third-party/seqan/core/include/seqan; do
+ if [ ! -d "$dir" ]; then
+ echo "Error: Required directory $dir not found"
+ exit 1
+ fi
+done
+
+# Copy files to correct locations
+cp -v lib/* $PREFIX/lib || { echo "Error copying library files"; exit 1; }
+cp -v scripts/* $PREFIX/bin || { echo "Error copying scripts"; exit 1; }Committable suggestion skipped: line range outside the PR's diff.
| #!/bin/bash | ||
|
|
||
| set -x -e | ||
|
|
||
| # export variables for compiling | ||
| export INCLUDE_PATH="${BUILD_PREFIX}/include:${PWD}/third-party/seqan/core/include" | ||
| export LIBRARY_PATH="${BUILD_PREFIX}/lib" | ||
| export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:${PWD}/lib" | ||
|
|
||
| export LDFLAGS="-L${BUILD_PREFIX}/lib -L${PWD}/third-party/seqan/core/include" | ||
| export CCPFLAGS="-I${BUILD_PREFIX}/include -I${PWD}/third-party/seqan/core/include" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review environment variable handling
The environment variable setup has several potential issues:
INCLUDE_PATHandLIBRARY_PATHcompletely override system paths instead of appending- Hardcoded paths to
third-party/seqancould break if the directory structure changes - Modifying
LD_LIBRARY_PATHmight affect runtime behavior unexpectedly
Consider these improvements:
-export INCLUDE_PATH="${BUILD_PREFIX}/include:${PWD}/third-party/seqan/core/include"
-export LIBRARY_PATH="${BUILD_PREFIX}/lib"
-export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:${PWD}/lib"
+export INCLUDE_PATH="${INCLUDE_PATH:+$INCLUDE_PATH:}${BUILD_PREFIX}/include:${PWD}/third-party/seqan/core/include"
+export LIBRARY_PATH="${LIBRARY_PATH:+$LIBRARY_PATH:}${BUILD_PREFIX}/lib"
# Remove LD_LIBRARY_PATH modification as it's typically not needed during build📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| set -x -e | |
| # export variables for compiling | |
| export INCLUDE_PATH="${BUILD_PREFIX}/include:${PWD}/third-party/seqan/core/include" | |
| export LIBRARY_PATH="${BUILD_PREFIX}/lib" | |
| export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:${PWD}/lib" | |
| export LDFLAGS="-L${BUILD_PREFIX}/lib -L${PWD}/third-party/seqan/core/include" | |
| export CCPFLAGS="-I${BUILD_PREFIX}/include -I${PWD}/third-party/seqan/core/include" | |
| #!/bin/bash | |
| set -x -e | |
| # export variables for compiling | |
| export INCLUDE_PATH="${INCLUDE_PATH:+$INCLUDE_PATH:}${BUILD_PREFIX}/include:${PWD}/third-party/seqan/core/include" | |
| export LIBRARY_PATH="${LIBRARY_PATH:+$LIBRARY_PATH:}${BUILD_PREFIX}/lib" | |
| # Remove LD_LIBRARY_PATH modification as it's typically not needed during build | |
| export LDFLAGS="-L${BUILD_PREFIX}/lib -L${PWD}/third-party/seqan/core/include" | |
| export CCPFLAGS="-I${BUILD_PREFIX}/include -I${PWD}/third-party/seqan/core/include" |
| # create directories for compiling | ||
| mkdir -p $PREFIX/bin | ||
| mkdir -p $PREFIX/include | ||
| mkdir -p $PREFIX/lib | ||
| mkdir -p $PREFIX/scripts |
There was a problem hiding this comment.
Add safety check for $PREFIX environment variable
The script assumes $PREFIX is set but doesn't verify it. This could lead to unexpected behavior if the variable is unset.
Add this check at the beginning of the script:
set -x -e
+# Verify required environment variables
+if [ -z "${PREFIX}" ]; then
+ echo "Error: PREFIX environment variable is not set"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
| number: 0 | ||
| skip: True # [py >= 312] | ||
| run_exports: | ||
| - {{ pin_subpackage('khmer', max_pin="x.x") }} | ||
| - {{ pin_subpackage(name|lower, max_pin="x.x") }} |
There was a problem hiding this comment.
Inconsistency with PR objectives regarding Python 3.12 compatibility.
The PR objectives mention addressing Python 3.12 compatibility issues, but the recipe explicitly skips Python >= 3.12. This seems contradictory.
Please clarify:
- If the package should support Python 3.12, remove the skip condition
- If Python 3.12 is not supported, update the PR objectives to reflect this limitation
khmer does not play well with python 3.12 and so I have updated the requirement. More information: dib-lab/khmer#1927
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>.