Update piscem to 0.10.4 #51799
Update piscem to 0.10.4 #51799
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Possibly related PRs
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 (
|
|
@bgruening : Any idea on why the OSX-64 build alone may be failing? It passes fine on our repo's on CI. It's strange that aarch-osx passes (and linux x86-64) but not OSX-64. |
try to fix too-old SDK on OSX x86
![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 (2)
recipes/piscem/build.sh (2)
13-13: LGTM! Consider documenting the SDK version choice.Setting
MACOSX_SDK_VERSIONto 10.15 aligns well with the deployment target and provides good compatibility.Consider adding a comment explaining why 10.15 was chosen as the SDK version, which would help future maintainers understand the rationale.
+ # Use macOS 10.15 SDK for better compatibility across different macOS versions export MACOSX_SDK_VERSION=10.15
7-13: Consider documenting these macOS fixes in the Bioconda documentation.These macOS-specific fixes (SDK version and aligned allocation) might be valuable patterns for other packages facing similar issues. Consider adding them to the Bioconda documentation or troubleshooting guide.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/piscem/build.sh(1 hunks)
🔇 Additional comments (1)
recipes/piscem/build.sh (1)
7-11: LGTM! The x86_64 macOS specific handling looks appropriate.
This change appears to address the OSX-64 build failure by adding the -D_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC flag, which is a known solution for aligned allocation issues in older macOS SDKs.
Let's verify if this is the only instance where we need this fix:
✅ Verification successful
The x86_64 macOS specific fix is unique and appropriate
The -D_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC flag addition in piscem's build script is specific to addressing aligned allocation issues in older macOS SDKs. While other recipes do have x86_64-specific handling, they address different concerns:
- centrifuger: adds SSE4.2 optimization
- t1k and trust4: handle samtools dependency differently
The fix is correctly scoped to piscem and doesn't need to be propagated to other recipes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other recipes that might need similar SDK fixes
rg -l "LIBCPP_HAS_NO_C11_ALIGNED_ALLOC" recipes/
Length of output: 72
Script:
#!/bin/bash
# Let's check if there are other recipes with similar x86_64 macOS specific handling
rg -l "uname -m.*x86_64" recipes/
# Also check for any other recipes that might have macOS SDK version handling
rg -l "MACOSX_SDK_VERSION" recipes/
Length of output: 521
Script:
#!/bin/bash
# Let's check the context of x86_64 handling in other recipes that might have similar build issues
rg "uname -m.*x86_64" recipes/trust4/build.sh recipes/t1k/build.sh recipes/centrifuger/build.sh -A 3 -B 3
# Also check if any of these recipes have similar compiler flag adjustments
rg "CFLAGS|CXXFLAGS" recipes/trust4/build.sh recipes/t1k/build.sh recipes/centrifuger/build.sh
Length of output: 1964
|
Thanks @rob-p! |
|
Sure thing! For the record, the issue seems to be that the If there is a suggest of how to address this more centrally, let me know (or if there is a way to force a newer version of OSX that I overlooked). Thanks, |
|
I am not an expert on how cargo orchestrates the compilation process, but from a quick look at the recipe it does not seem like you are passing conda's sysroot to the build system. My guess is that cargo is picking up the runner SDK instead of the one installed by conda. Bioconda recently switched from Azure to GH Actions, which may have caused a change in the SDK used by the runner. This could possibly explain why the problem started occurring just now. In the C++/CMake world you can pass the appropriate sysroot like so: if [[ ${HOST} =~ .*darwin.* ]]; then
cmake -DCMAKE_OSX_SYSROOT="${CONDA_BUILD_SYSROOT}" ...
else
cmake -DCMAKE_TOOLCHAIN_FILE="${RECIPE_DIR}/cross-linux.cmake" ...
fiWhere # this one is important
set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_PLATFORM Linux)
#this one not so much
set(CMAKE_SYSTEM_VERSION 1)
# specify the cross compiler
set(CMAKE_C_COMPILER $ENV{CC})
set(CMAKE_CXX_COMPILER $ENV{CXX})
# where is the target environment
set(CMAKE_FIND_ROOT_PATH $ENV{PREFIX} $ENV{BUILD_PREFIX}/$ENV{HOST}/sysroot)
# search for programs in the build host directories
set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
# for libraries and headers in the target directories
set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
# god-awful hack because it seems to not run correct tests to determine this:
set(__CHAR_UNSIGNED___EXITCODE 1)Note that on macOS x86 this may not be enough if you depend on somewhat modern C/C++ features. See here for some guidance on how to proceed. I am not sure how these steps can be adapted to fit cargo's way of doing things, but I hope some of this will be useful to you/other people stumbling on this issue. |
|
Thanks @robomics --- the offending code is actually in a C++ submodule (managed with CMake) that is being build by cargo under the hood to produce a statically linked library. Thus, your CMake solution may very well work if I implement it in the underlying submodule (which is also code that we have developed in the lab / control). We do rely very heavily on C++17 (at a minimum), and so that is a hard requirement for us for build. I just wish there was an easier way to tell conda / bioconda to only provide a newer SDK by default at the conda / CI level (rather than having to pass a bunch of specific variables to the individual build systems). Our CMake and To build C++ code, Rust uses I also noticed the switch from Azure to GitHub CI, and it didn't escape me that this coincided with the problem. However, I also thought it was particularly strange that it didn't affect the aarch64 build for OSX, but only the x86 build. |
|
Thanks for the clarifications! I think it should be possible to deal with all/most of bioconda peculiarities using CMake toolchain files (which does not require modifying the You should be able to adapt the toolchain file from my previous post and then pass it down to all CMake invocations by defining the CMAKE_TOOLCHAIN_FILE env variable in the build script. All of this should not be an issue on macOS arm64 (at least when it comes to C++17 support) because the oldest OS/SDK with arm support is v11.0. |
Update
piscem: 0.10.3 → 0.10.4recipes/piscem(click to view/edit other files)@COMBINE-labThis pull request was automatically generated (see docs).