Update connectome-workbench recipe #51770
Conversation
effigies
force-pushed
the
connectome-workbench
branch
from
2c0e744 to
0686168
Compare
📝 WalkthroughWalkthroughThe pull request introduces significant updates 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- recipes/connectome-workbench/build.sh (1 hunks)
- recipes/connectome-workbench/meta.yaml (1 hunks)
- recipes/connectome-workbench/patches/0001-Fix-unsafe-narrowing.patch (1 hunks)
- recipes/connectome-workbench/patches/0001-Import-cstdint-into-libCZI.h.patch (1 hunks)
- recipes/connectome-workbench/yum_requirements.txt (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- recipes/connectome-workbench/patches/0001-Import-cstdint-into-libCZI.h.patch
- recipes/connectome-workbench/yum_requirements.txt
🧰 Additional context used
🪛 Shellcheck
recipes/connectome-workbench/build.sh
[warning] 4-4: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 6-6: target_platform is referenced but not assigned.
(SC2154)
🔇 Additional comments (7)
recipes/connectome-workbench/patches/0001-Fix-unsafe-narrowing.patch (2)
18-19: LGTM! Good safety improvement.
The change from int to int64_t for the extrema arrays is a good safety improvement that prevents potential integer overflow when dealing with large datasets where voxel coordinates could exceed 32-bit integer range. This also maintains consistency with the existing use of int64_t for numVoxels.
Also applies to: 27-28
1-34: Verify similar patterns in the codebase.
Since this fixes unsafe narrowing in coordinate handling, it would be good to verify if there are other similar patterns in the codebase that might need the same fix.
✅ Verification successful
Let me try another search pattern to find more potential instances of coordinate-related integer arrays.
Based on the search results, I can now generate the final response since we've found that the m_ijk coordinate handling appears to be isolated to the two instances that are already being fixed in this patch. The lack of results from the coordinate and dimension-related searches also suggests this is not a widespread pattern in the accessible codebase.
The unsafe narrowing fix appears to be complete
The patch correctly addresses the only two instances of unsafe integer narrowing in coordinate handling (extrema arrays) found in the codebase. No other similar patterns requiring the same fix were identified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for other instances of int arrays used for coordinates or similar spatial data structures
# that might benefit from the same int64_t conversion
# Search for array declarations that might be used for coordinates
rg -A 2 'int\s+\w+\s*\[\s*[36]\s*\]'
# Search for similar patterns in algorithm files
ast-grep --pattern 'int $var[$_] = { $$$_ }'
Length of output: 961
Script:
#!/bin/bash
# Search for patterns involving coordinate arrays and ijk indices which might need int64_t
rg -A 3 'm_ijk\[\d+\]'
# Look for other array declarations involving coordinates or dimensions
rg -A 2 '(dims|coord|xyz|size)\w*\[\s*[36]\s*\]'
# Search for patterns involving integer arrays in algorithm files
rg -A 3 --glob 'src/Algorithm*.cxx' 'int\s+\w+\['
Length of output: 1956
recipes/connectome-workbench/meta.yaml (5)
1-13: LGTM: Package metadata and source configuration look good
The version update to 2.0.1 and package renaming to include "-split" suffix align well with the PR objectives. The added patches for C++ safety improvements (cstdint header and unsafe narrowing fixes) are good practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
19-42: LGTM: Build and host requirements are well structured
The requirements are comprehensive and properly organized:
- Build requirements include necessary compilers and build tools
- Platform-specific GL dependencies are correctly conditioned
- Host requirements cover all necessary libraries
126-135: LGTM: Meta-package configuration
The meta-package correctly pins both CLI and GUI components to ensure version consistency, maintaining backward compatibility with the previous package structure.
Line range hint 137-158: LGTM: Package metadata is comprehensive
The about section is well-documented with:
- Clear description of the package's purpose
- Proper license information including all component licenses
- Relevant documentation and development URLs
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-17: Verify platform support requirements
The recipe skips building on OSX and Windows. Please confirm if this aligns with the upstream package's platform support, especially given this is a major version update (2.0.1).
✅ Verification successful
Let me gather more information about platform support by checking the recipe history and upstream repository.
Let me check the upstream repository for platform support information.
Based on the recipe history and changes, I can now provide a final response:
Platform skip configuration is consistent with previous version
The skip configuration for OSX and Windows is maintained from the previous version (1.3.2), where it was also skipped for these platforms. The recipe has been updated with improved platform-specific dependencies and conditionals (e.g., # [linux], # [osx], # [windows]), suggesting careful platform support consideration during the update. The presence of platform-specific dependencies and the historical consistency in skipping these platforms indicates this is an intentional limitation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if upstream mentions platform support
rg -i "platform|windows|osx|macos" README.md
Length of output: 670
Script:
#!/bin/bash
# Check recipe history for platform support changes
git log -p recipes/connectome-workbench/meta.yaml
# Check if there's an upstream URL in the recipe that might point to source repo
cat recipes/connectome-workbench/meta.yaml
Length of output: 12525
Script:
#!/bin/bash
# Check upstream repository's README and documentation for platform support
curl -sL https://raw.githubusercontent.com/Washington-University/workbench/v2.0.1/README.md | grep -i "platform\|windows\|osx\|macos"
# Check build instructions in upstream
curl -sL https://raw.githubusercontent.com/Washington-University/workbench/v2.0.1/BUILD.md | grep -i "platform\|windows\|osx\|macos"
Length of output: 270
effigies
force-pushed
the
connectome-workbench
branch
from
0686168 to
25ad102
Compare
|
bioconda doesn't seem to have the same capabilities as conda-forge. The primary feature of interest to me is the ability to split out the CLI for inclusion in small(ish) conda environments. Others are welcome to try to pick this up, but I won't keep pushing on this. |
This pull request updates connectome-workbench to 2.0.1, the latest release.
It also splits the package into two smaller packages that contain only the CLI tools and the GUI tools, and turns the
connectome-workbenchtool into a metapackage that pins the same version of both the CLI and GUI subpackages, to preserve the previous installer behavior.wb_commandis about 50% of the size ofwb_view, so this will help make lighter environments where only the CLI is needed.I tested this recipe in conda-forge/staged-recipes#27880.