Update libmaus2 to v2.0.183 #52154
Update libmaus2 to v2.0.183 #52154
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the In 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: 0
🧹 Outside diff range and nitpick comments (3)
recipes/libmaus2/meta.yaml (1)
29-44: LGTM! Consider documenting new dependenciesThe addition of build tools and new host dependencies aligns well with the build script changes. The comment explaining snappy version pinning is helpful.
Consider adding comments explaining why the new dependencies (libdeflate, gmp, nettle, parasail-python) were added, similar to the helpful snappy version comment.
recipes/libmaus2/build.sh (2)
5-6: Evaluate the necessity of runningautoupdatein the build scriptRunning
autoupdateis not typically required in build scripts and may introduce unintended changes toconfigure.ac. Unless there's a specific reason, consider removing it to prevent potential build issues.Suggested change:
autoreconf -if -autoupdate
16-16: Remove unnecessary quotes around${CPU_COUNT}in themakecommandQuoting
${CPU_COUNT}inmake -j"${CPU_COUNT}"is unnecessary and may causemaketo misinterpret the argument. It's standard practice to usemake -j${CPU_COUNT}without quotes.Suggested change:
-make -j"${CPU_COUNT}" +make -j${CPU_COUNT}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/libmaus2/build.sh(1 hunks)recipes/libmaus2/meta.yaml(3 hunks)
🔇 Additional comments (4)
recipes/libmaus2/meta.yaml (3)
55-61: LGTM! Improved package metadata
The additions of license metadata and dev_url improve package documentation and comply with best practices.
13-15: Verify the impact of relaxed version pinning
The change from max_pin='x.x' to max_pin='x' allows updates within the major version, which could potentially lead to compatibility issues if there are breaking changes in minor versions.
#!/bin/bash
# Description: Check for potential breaking changes in recent versions
# Look for changelog or release notes mentioning breaking changes
rg -i "break|incompatible|migration" --type md
# Check if other packages in the ecosystem use stricter pinning
fd -e yaml -e yml | xargs rg "pin_subpackage.*libmaus2"2-4: Version number differs from PR objective
The PR description mentions updating to v2.0.183, but the file shows v2.0.813. This significant version difference needs verification.
recipes/libmaus2/build.sh (1)
7-12: Ensure all new dependencies are listed in meta.yaml
The ./configure command now includes new dependencies:
libdeflatelibsecrecynettleparasaillzmagmp
According to the PR objectives, only libdeflate, gmp, and nettle have been added to meta.yaml. Please verify that libsecrecy, parasail, and lzma are also included under both build and run dependencies in meta.yaml to prevent build or runtime errors.
To confirm that all dependencies are listed in meta.yaml, you can run the following script:
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/libmaus2/build.sh(1 hunks)recipes/libmaus2/meta.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/libmaus2/meta.yaml
🔇 Additional comments (3)
recipes/libmaus2/build.sh (3)
1-17: Consider ABI compatibility and version constraints
Given that this PR updates libmaus2 to v2.0.183 and introduces new library dependencies, please ensure:
- The
meta.yamlfile includes appropriaterun_exportssettings as mentioned in the PR guidelines - Version constraints are properly set for all new dependencies to prevent ABI/API breakage
- Consider adding a pkg-config file to help downstream packages find and link against libmaus2 correctly
#!/bin/bash
# Check for existing pkg-config usage and run_exports
rg -A 5 "run_exports:"
fd -t f "meta.yaml" | xargs rg "pkg-config.*libmaus2"7-12: Verify new library dependencies and their versions
Several new libraries have been added to the configuration (libdeflate, libsecrecy, nettle, lzma, gmp). This could potentially introduce ABI compatibility issues if not properly versioned in meta.yaml.
#!/bin/bash
# Check if these libraries are properly declared in meta.yaml with version constraints
rg -A 10 "dependencies:" | rg -A 15 "host:"
# Look for any existing version constraints in other recipes using these libraries
fd -t f "meta.yaml" | xargs rg "libdeflate|libsecrecy|nettle|lzma|gmp"3-3: Verify library dependencies and optimize link order
The link order can affect resolution of symbols. Consider reordering the LIBS variable to put system libraries last:
-export LIBS="-lstdc++fs -lcurl -lz -ldeflate"
+export LIBS="-ldeflate -lcurl -lz -lstdc++fs"
recipes/libmaus2/build.sh
Outdated
| autoreconf -if | ||
| autoupdate |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for autotools commands
The autoreconf and autoupdate commands might fail silently. Consider adding error checking:
-autoreconf -if
-autoupdate
+if ! autoreconf -if; then
+ echo "Failed to run autoreconf"
+ exit 1
+fi
+if ! autoupdate; then
+ echo "Failed to run autoupdate"
+ exit 1
+fi📝 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.
| autoreconf -if | |
| autoupdate | |
| if ! autoreconf -if; then | |
| echo "Failed to run autoreconf" | |
| exit 1 | |
| fi | |
| if ! autoupdate; then | |
| echo "Failed to run autoupdate" | |
| exit 1 | |
| fi |
| make -j"${CPU_COUNT}" | ||
| make install |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for make commands
Consider adding error handling and logging for make commands:
-make -j"${CPU_COUNT}"
-make install
+if ! make -j"${CPU_COUNT}"; then
+ echo "Build failed. Last 50 lines of config.log:"
+ tail -n 50 config.log
+ exit 1
+fi
+if ! make install; then
+ echo "Installation failed"
+ exit 1
+fi📝 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.
| make -j"${CPU_COUNT}" | |
| make install | |
| if ! make -j"${CPU_COUNT}"; then | |
| echo "Build failed. Last 50 lines of config.log:" | |
| tail -n 50 config.log | |
| exit 1 | |
| fi | |
| if ! make install; then | |
| echo "Installation failed" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/libmaus2/build.sh (1)
8-8: Review optimization and compatibility flagsThe
-O3optimization flag and-D_LIBCPP_DISABLE_AVAILABILITYmacro raise concerns:
-O3might not be optimal for all architectures and could potentially introduce issues- Disabling availability checks could mask ABI compatibility problems
Consider:
- Using
-O2as a more conservative default- Documenting why availability checks are disabled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/libmaus2/build.sh(1 hunks)recipes/libmaus2/meta.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/libmaus2/meta.yaml
🔇 Additional comments (4)
recipes/libmaus2/build.sh (4)
5-6: Error handling suggestion already noted
17-18: Error handling suggestion already noted
7-13: Verify security implications of new cryptographic dependencies
The addition of libsecrecy and nettle suggests security-sensitive functionality. Please ensure:
- These libraries are maintained and security-patched
- Version constraints are properly specified in meta.yaml
Let's check the security advisories for these dependencies:
#!/bin/bash
# Description: Check for security advisories for cryptographic dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: BIOCONDA, package: "nettle") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'3-3: Consider optimizing library linking order
The linking order can affect resolution of symbol dependencies. Consider reordering the libraries to put system libraries last:
-export LIBS="-lstdc++ -lcurl -lz -ldeflate"
+export LIBS="-ldeflate -lcurl -lz -lstdc++"Let's verify the availability of these libraries in the build environment:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
recipes/libmaus2/build.sh (3)
3-3: Verify library availability before buildWhile the LIBS configuration is correct, it would be beneficial to verify the availability of these libraries before proceeding with the build.
export LIBS="-lstdc++fs -lcurl -lz -ldeflate" +# Verify required libraries +for lib in stdc++fs curl z deflate; do + if ! ldconfig -p | grep -q "lib${lib}"; then + echo "Error: lib${lib} not found" + exit 1 + fi +done
13-13: Consider build memory optimizationFor large builds, unrestricted parallel compilation might consume excessive memory. Consider adding a memory-aware job count calculation.
-make -j"${CPU_COUNT}" +# Calculate optimal job count based on available memory +MEM_GB=$(free -g | awk '/^Mem:/{print $2}') +JOBS=$(( MEM_GB < CPU_COUNT ? MEM_GB : CPU_COUNT )) +make -j"${JOBS}"
1-14: Consider adding build documentationSince this is a complex build with multiple dependencies and configuration options, consider adding a comment block at the top of the script documenting:
- Required system dependencies
- Expected environment variables
- Build options and their purposes
- Common troubleshooting steps
recipes/libmaus2/patch (2)
Line range hint
59-72: Enhance filesystem compatibility testingWhile the tests have been updated for the new namespace, consider adding:
- Cross-platform compatibility tests
- Edge cases for different path formats
- Performance comparison tests between standard and experimental implementations
Consider adding these test cases:
+// Test cross-platform path handling +void test_cross_platform_paths() { + // Windows-style paths + test_path("C:\\path\\to\\file"); + // Unix-style paths + test_path("/path/to/file"); + // Network paths + test_path("//server/share/file"); +}
Line range hint
1-270: Consider a staged migration strategy for filesystem handlingThe wholesale transition to
std::experimental::filesystemraises architectural concerns. Consider:
- Implementing a filesystem abstraction layer to isolate filesystem operations
- Using feature detection to choose between standard and experimental implementations
- Planning a future migration path to
std::filesystemThis would allow for:
- Easier future transitions
- Better platform compatibility
- Reduced maintenance burden
Example abstraction:
namespace libmaus2 { namespace filesystem { class PathHandler { public: static std::string getAbsPath(const std::string& path); static bool exists(const std::string& path); // ... other operations private: // Implementation can choose appropriate filesystem library }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/libmaus2/build.sh(1 hunks)recipes/libmaus2/patch(1 hunks)
🔇 Additional comments (5)
recipes/libmaus2/build.sh (2)
5-5: Previous comment about autoreconf error handling still applies
13-14: Previous comment about make error handling still applies
recipes/libmaus2/patch (3)
244-257: Verify template specialization compatibility
The changes to path handling in templates could affect:
- Template specializations
- Dependent code that relies on path type deduction
- Generic code that works with path objects
Let's check for potential impacts:
Also applies to: 265-267
Line range hint 16-27: Verify filesystem operations compatibility
The core filesystem operations have been modified to use experimental features. While functionality remains the same, these are critical path operations that could affect:
- Path resolution
- Directory operations
- File system traversal
Let's verify the impact on existing functionality:
Also applies to: 74-75
✅ Verification successful
Let me analyze the code changes and the search results to verify the impact of switching from std::filesystem to std::experimental::filesystem.
Based on the code changes and search results, I can now provide a final assessment:
Filesystem namespace changes are consistent and safe
The changes to switch from std::filesystem to std::experimental::filesystem are localized and consistent across the codebase. The modifications affect basic filesystem operations like:
- Directory creation and permissions
- Path manipulation
- File removal
The changes maintain identical functionality while just using the experimental namespace. No error handling or exception cases are impacted, as the filesystem error types remain unchanged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage patterns that might break
# Look for code patterns that depend on filesystem behavior
# Search for direct path manipulation
rg -A 2 "\.string\(\)|\.remove_filename\(\)|\.filename\(\)"
# Search for directory operations
rg -A 2 "create_directory|remove|permissions"
Length of output: 396233
Script:
#!/bin/bash
# Search for other filesystem namespace usages to ensure consistency
ast-grep --pattern 'std::filesystem::$_'
ast-grep --pattern 'std::experimental::filesystem::$_'
# Search for any filesystem-related error handling or exception cases
rg -A 2 "filesystem.*error|filesystem.*exception"
Length of output: 3193
Line range hint 21-24: Consider using standard filesystem library instead of experimental
The transition to std::experimental::filesystem raises concerns:
std::experimental::filesystemis deprecated since C++17 in favor ofstd::filesystem- This change could lead to future compatibility issues
- The experimental namespace is typically used for features under development
Let's verify the compiler support for standard filesystem:
Also applies to: 72-74, 303-305
| ./configure --prefix="${PREFIX}" CXX="${CXX}" CC="${CC}" \ | ||
| LDFLAGS="${LDFLAGS} -L${PREFIX}/lib" \ | ||
| CPPFLAGS="${CPPFLAGS} -I${PREFIX}/include" \ | ||
| --with-snappy --with-io_lib --with-libdeflate \ | ||
| --with-libsecrecy --with-nettle \ | ||
| --with-lzma --with-gmp |
There was a problem hiding this comment.
💡 Codebase verification
Dependencies mismatch found in configure options and meta.yaml
libsecrecyis specified in configure options but missing from meta.yaml requirementslzmais specified in configure options but not explicitly listed in meta.yamlio_libin configure refers tostaden_io_libin meta.yaml- Other dependencies (snappy, libdeflate, nettle, gmp) are properly specified
Recommendations:
- Remove
--with-libsecrecyfrom configure options as it's not a dependency - Either remove
--with-lzmafrom configure or add it to meta.yaml if required - Consider renaming
--with-io_libto--with-staden_io_libfor consistency
🔗 Analysis chain
Verify configure options against package requirements
The configure options look correct, but let's verify that all required dependencies are properly specified in meta.yaml.
Let me check the actual content of meta.yaml to better understand the dependencies and their naming conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all libraries mentioned in configure options have corresponding entries in meta.yaml
echo "Checking meta.yaml for required dependencies..."
if [ -f "recipes/libmaus2/meta.yaml" ]; then
required_libs=("snappy" "io_lib" "libdeflate" "libsecrecy" "nettle" "lzma" "gmp")
for lib in "${required_libs[@]}"; do
if ! grep -q "${lib}" recipes/libmaus2/meta.yaml; then
echo "Warning: ${lib} is used in configure but might be missing in meta.yaml"
fi
done
fi
Length of output: 1090
Script:
#!/bin/bash
# Display the content of meta.yaml to check dependency specifications
cat recipes/libmaus2/meta.yaml
Length of output: 1765
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.circleci/config.yml (3)
Line range hint
150-250: Consider refactoring common build configurationThe bulk_build job contains duplicated configuration code that could be extracted into a shared command:
- Git user configuration
- Build environment setup
- Build command configuration
This would improve maintainability and reduce the risk of configuration drift between jobs.
Consider creating a shared command in CircleCI:
commands: setup_build_env: steps: - run: name: Configure Git command: | git config user.name BiocondaBot git config user.email [email protected] - run: name: Setup Build Environment command: | wget https://raw.githubusercontent.com/bioconda/bioconda-common/bulk/{common,install-and-set-up-conda,configure-conda}.sh sudo mkdir -p /opt/ sudo chmod o+rwx /opt bash install-and-set-up-conda.sh
Line range hint
250-300: Add error handling for upload failuresThe nightly build job should include proper error handling and notifications for upload failures to ensure failed uploads are tracked and can be retried.
Add error handling:
bioconda-utils build recipes config.yml \ --docker --mulled-test --docker-base-image "quay.io/bioconda/bioconda-utils-build-env-cos7-$(arch):${BIOCONDA_UTILS_TAG#v}" \ --anaconda-upload \ - --prelint --exclude bioconda-repodata-patches + --prelint --exclude bioconda-repodata-patches || { + echo "::error::Build or upload failed" + # Store failed packages list for retry + bioconda-utils build ... --dry-run --failed-builds-only > failed-uploads.txt + exit 1 +}
Line range hint
301-400: Document the biocontainers upload limitationThe comment "For now, do not upload ARM images to biocontainers" should be documented more formally, preferably in a configuration variable or documentation.
Consider adding this to the repository's documentation or creating a configuration variable:
experimental: arm_biocontainers_upload: false # ARM images upload to biocontainers is not yet supported
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(1 hunks)
🔇 Additional comments (1)
.circleci/config.yml (1)
12-12: Verify the necessity of increased resource allocation
The resource class upgrade from arm.medium to arm.large will increase CI costs. Please ensure this change is necessary for building libmaus2 v2.0.183.
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
|
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>.