Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

centrifuge-core: add linux-aarch64 build #51517

Merged
merged 2 commits into from
Oct 21, 2024
Merged

centrifuge-core: add linux-aarch64 build #51517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
559e3d8
centrifuge-core: add linux-aarch64 build
martin-g Oct 21, 2024
de109ea
Use specific compiler flags for ARM64
martin-g Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion recipes/centrifuge-core/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
#!/bin/bash

set -xe

export LDFLAGS="-L$PREFIX/lib"
export CPATH=${PREFIX}/include

mkdir -p $PREFIX/bin

case $(uname -m) in
aarch64 | arm64)
ARCH_FLAGS=(POPCNT_CAPABILITY=0 "RELEASE_FLAGS=${CXXFLAGS} -fsigned-char")
;;
*)
ARCH_FLAGS=("RELEASE_FLAGS=${CXXFLAGS}")
;;
esac

sed "/^GCC/d;/^CC =/d;/^CPP =/d;/^CXX =/d" < Makefile > Makefile.new
mv Makefile.new Makefile
cat Makefile
make CC=$CC CXX=$CXX RELEASE_FLAGS="$CXXFLAGS"
make -j"${CPU_COUNT}" CC=$CC CXX=$CXX "${ARCH_FLAGS[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update make command to use ARCH_FLAGS array.

Once you've implemented the change to make ARCH_FLAGS an array, please update the make command to properly expand the array:

-make -j"${CPU_COUNT}" CC=$CC CXX=$CXX ${ARCH_FLAGS}
+make -j"${CPU_COUNT}" CC=$CC CXX=$CXX "${ARCH_FLAGS[@]}"

This change ensures that the array elements of ARCH_FLAGS are properly expanded and passed to the make command.

📝 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.

Suggested change
make -j"${CPU_COUNT}" CC=$CC CXX=$CXX "${ARCH_FLAGS[@]}"
make -j"${CPU_COUNT}" CC=$CC CXX=$CXX "${ARCH_FLAGS[@]}"

make install prefix=$PREFIX

12 changes: 9 additions & 3 deletions recipes/centrifuge-core/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ package:
version: {{ version }}

build:
number: 0
number: 1
skip: true # [osx]
run_exports:
- {{ pin_subpackage("centrifuge-core", max_pin="x.x") }}
Expand Down Expand Up @@ -34,11 +34,17 @@ test:
- centrifuge --help

about:
home: https://github.com/DaehwanKimLab/centrifuge
license: GPL3
home: http://www.ccb.jhu.edu/software/centrifuge
license: GPL-3.0-only
license_file: LICENSE
license_family: GPL3
summary: 'Classifier for metagenomic sequences. Does not include evaluation scripts'
dev_url: https://github.com/DaehwanKimLab/centrifuge
doc_url: https://genome.cshlp.org/content/26/12/1721

extra:
additional-platforms:
- linux-aarch64
Comment on lines +46 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Issue Detected: Incomplete aarch64 Handling in build.sh

While linux-aarch64 has been added to the additional-platforms in meta.yaml, the build.sh script does not appear to include specific environment configurations, dependency installations, or build commands for aarch64. This may result in build failures or incomplete support for the linux-aarch64 architecture.

  • File: recipes/centrifuge-core/build.sh
  • Action Needed: Implement aarch64-specific environment variables, dependency installations, and build commands to ensure proper support.
🔗 Analysis chain

LGTM: Addition of linux-aarch64 support.

The inclusion of linux-aarch64 in the 'additional-platforms' list aligns with the PR objective of adding support for this architecture.

To ensure complete implementation, let's verify that the build.sh script properly handles the aarch64 architecture:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that build.sh handles aarch64 architecture

# Test: Check if build.sh contains aarch64-specific logic
rg -i 'aarch64|arm64' recipes/centrifuge-core/build.sh

Length of output: 77

Script:

#!/bin/bash
# Description: Verify comprehensive handling of aarch64 in build.sh

# Check for aarch64-specific environment variables
rg -i 'export.*aarch64' recipes/centrifuge-core/build.sh

# Check for aarch64 in dependency installations
rg -i 'aarch64' recipes/centrifuge-core/build.sh | grep -i 'install\|depend'

# Check for aarch64 in build commands
rg -i 'aarch64' recipes/centrifuge-core/build.sh | grep -i 'build\|compile'

Length of output: 211

Script:

#!/bin/bash
# Description: Verify comprehensive handling of linux-aarch64 in build.sh

# Check for linux-aarch64-specific environment variables
rg -i 'export.*linux-aarch64' recipes/centrifuge-core/build.sh

# Check for linux-aarch64 in dependency installations
rg -i 'linux-aarch64' recipes/centrifuge-core/build.sh | grep -i 'install\|depend'

# Check for linux-aarch64 in build commands
rg -i 'linux-aarch64' recipes/centrifuge-core/build.sh | grep -i 'build\|compile'

# Additionally, verify any references to 'linux-aarch64' in conditional statements
rg -i 'linux-aarch64' recipes/centrifuge-core/build.sh | grep -i 'if\|case'

Length of output: 305

identifiers:
- biotools:Centrifuge
- doi:10.1101/gr.210641.116