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

update d4tools and d4binding #48077

Merged
merged 9 commits into from
Oct 23, 2024
Merged

update d4tools and d4binding #48077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8f2ec10
update build
ramprasadn May 25, 2024
44c6ba1
fix lint
ramprasadn May 25, 2024
6cc327a
update build
ramprasadn May 25, 2024
0a488a1
update path
ramprasadn May 25, 2024
6650695
Merge branch 'master' into d4binding
mencian Oct 23, 2024
6755d94
bump d4tools build number
mencian Oct 23, 2024
2d51e68
bump d4binding to 0.3.11
mencian Oct 23, 2024
1934ecd
clean up recipe
mencian Oct 23, 2024
387ff98
clean up recipe
mencian Oct 23, 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
3 changes: 0 additions & 3 deletions build-fail-blacklist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,9 +772,6 @@ recipes/perl-gd
# sha mismatch when downloading source
recipes/fmlrc2

# tries to download zlib itself, which fails
recipes/d4binding

# no matching package named `quickersort` found
recipes/mudskipper

Expand Down
6 changes: 4 additions & 2 deletions recipes/d4binding/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ set -x
# TODO: Remove the following export when pinning is updated and we use
# {{ compiler('rust') }} in the recipe.
export \
CARGO_NET_GIT_FETCH_WITH_CLI=true \
CARGO_HOME="${BUILD_PREFIX}/.cargo"
CARGO_NET_GIT_FETCH_WITH_CLI=true \
CARGO_HOME="${BUILD_PREFIX}/.cargo"

cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance robustness of the build_htslib.sh copy operation.

The current copy operation lacks error handling and directory verification. Consider adding these safety checks to prevent build failures.

Here's a more robust implementation:

-cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh
+if [ ! -f "$RECIPE_DIR/build_htslib.sh" ]; then
+    echo "Error: build_htslib.sh not found in recipe directory"
+    exit 1
+fi
+mkdir -p d4-hts
+cp "$RECIPE_DIR/build_htslib.sh" d4-hts/build_htslib.sh
+chmod +x d4-hts/build_htslib.sh
📝 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
cp $RECIPE_DIR/build_htslib.sh d4-hts/build_htslib.sh
if [ ! -f "$RECIPE_DIR/build_htslib.sh" ]; then
echo "Error: build_htslib.sh not found in recipe directory"
exit 1
fi
mkdir -p d4-hts
cp "$RECIPE_DIR/build_htslib.sh" d4-hts/build_htslib.sh
chmod +x d4-hts/build_htslib.sh


cd d4binding
./install.sh
63 changes: 63 additions & 0 deletions recipes/d4binding/build_htslib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#!/bin/bash

if [ "${DOCS_RS}" = "1" ]
then
exit 0
fi

set -ex
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance shell script safety settings.

Consider adding additional safety flags to prevent common shell script issues:

-set -ex
+set -euxo pipefail

This change will:

  • -u: Exit on undefined variables
  • -o pipefail: Ensure pipeline failures are caught
📝 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
set -ex
set -euxo pipefail


pushd ${OUT_DIR}
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

Validate required environment variables.

The script assumes OUT_DIR is set but doesn't validate it. Add validation to prevent unclear errors:

+if [ -z "${OUT_DIR}" ]; then
+    echo "ERROR: OUT_DIR environment variable is not set"
+    exit 1
+fi
+
 pushd ${OUT_DIR}
📝 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
pushd ${OUT_DIR}
if [ -z "${OUT_DIR}" ]; then
echo "ERROR: OUT_DIR environment variable is not set"
exit 1
fi
pushd ${OUT_DIR}


HTSLIB_VERSION=${1}

rm -rf ${OUT_DIR}/htslib

git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git
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

Use HTTPS instead of HTTP for git clone.

Using HTTP for git operations is less secure. Switch to HTTPS:

-git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git
+git clone -b ${HTSLIB_VERSION} https://github.com/samtools/htslib.git
📝 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
git clone -b ${HTSLIB_VERSION} http://github.com/samtools/htslib.git
git clone -b ${HTSLIB_VERSION} https://github.com/samtools/htslib.git


cd htslib

cat > config.h << CONFIG_H
#define HAVE_LIBBZ2 1
#define HAVE_DRAND48 1
CONFIG_H

perl -i -pe 's/hfile_libcurl\.o//g' Makefile

function is_musl() {
if [ ! -z $(echo $TARGET | grep musl) ]; then
return 0
else
return 1
fi
}
Comment on lines +27 to +33
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

Fix shell quoting in musl detection.

The current implementation is vulnerable to word splitting. Quote the grep result:

 function is_musl() {
-	if [ ! -z $(echo $TARGET | grep musl) ]; then 
+	if [ ! -z "$(echo "$TARGET" | grep musl)" ]; then 
 		return 0
 	else
 		return 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.

Suggested change
function is_musl() {
if [ ! -z $(echo $TARGET | grep musl) ]; then
return 0
else
return 1
fi
}
function is_musl() {
if [ ! -z "$(echo "$TARGET" | grep musl)" ]; then
return 0
else
return 1
fi
}
🧰 Tools
🪛 Shellcheck

[warning] 28-28: Quote this to prevent word splitting.

(SC2046)


is_musl && perl -i -pe 's/gcc/musl-gcc/g' Makefile


if [ "x${ZLIB_SRC}" != "x" ]
then
tar xz ${ZLIB_SRC}
else
curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz
fi
Comment on lines +38 to +43
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

Enhance download safety for zlib.

The current implementation lacks important safety measures:

 else
-	curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz
+	ZLIB_SHA256="629380c90a77892b5c0119a3be631f21e391faa5e93c7b5a5e2ad35d84f7f178"
+	curl --retry 3 --retry-delay 3 -L 'https://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' -o zlib.tar.gz
+	echo "${ZLIB_SHA256}  zlib.tar.gz" | sha256sum -c
+	tar xzf zlib.tar.gz
+	rm zlib.tar.gz
 fi

Changes:

  • Added SHA256 checksum verification
  • Added retry logic for network resilience
  • Switched to HTTPS
  • Added explicit download instead of piping to tar
📝 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
if [ "x${ZLIB_SRC}" != "x" ]
then
tar xz ${ZLIB_SRC}
else
curl -L 'http://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' | tar xz
fi
if [ "x${ZLIB_SRC}" != "x" ]
then
tar xz ${ZLIB_SRC}
else
ZLIB_SHA256="629380c90a77892b5c0119a3be631f21e391faa5e93c7b5a5e2ad35d84f7f178"
curl --retry 3 --retry-delay 3 -L 'https://github.com/madler/zlib/archive/refs/tags/v1.2.11.tar.gz' -o zlib.tar.gz
echo "${ZLIB_SHA256} zlib.tar.gz" | sha256sum -c
tar xzf zlib.tar.gz
rm zlib.tar.gz
fi

cd zlib-1.2.11
is_musl && CC=musl-gcc ./configure || ./configure
make libz.a
cp libz.a ..
cd ..

#The original file in the repo is lacking -L in the curl command so it doen't work
curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz
cd bzip2-1.0.8
Comment on lines +50 to +52
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

Fix typo in comment and enhance bzip2 download safety.

The comment has a typo and the download lacks safety measures:

-#The original file in the repo is lacking -L in the curl command so it doen't work
-curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz
+# The original file in the repo is lacking -L in the curl command so it doesn't work
+BZIP2_SHA256="ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269"
+curl --retry 3 --retry-delay 3 -L 'https://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz' -o bzip2.tar.gz
+echo "${BZIP2_SHA256}  bzip2.tar.gz" | sha256sum -c
+tar xzf bzip2.tar.gz
+rm bzip2.tar.gz
📝 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
#The original file in the repo is lacking -L in the curl command so it doen't work
curl -L http://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz | tar xz
cd bzip2-1.0.8
# The original file in the repo is lacking -L in the curl command so it doesn't work
BZIP2_SHA256="ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269"
curl --retry 3 --retry-delay 3 -L 'https://sourceware.org/pub/bzip2/bzip2-1.0.8.tar.gz' -o bzip2.tar.gz
echo "${BZIP2_SHA256} bzip2.tar.gz" | sha256sum -c
tar xzf bzip2.tar.gz
rm bzip2.tar.gz
cd bzip2-1.0.8

is_musl && perl -i -pe 's/gcc/musl-gcc/g' Makefile
is_musl || perl -i -pe 's/CFLAGS=/CFLAGS=-fPIC /g' Makefile
make
cp libbz2.a ..
cd ..

perl -i -pe 's/CPPFLAGS =/CPPFLAGS = -Izlib-1.2.11 -Ibzip2-1.0.8/g' Makefile

is_musl || perl -i -pe 's/CFLAGS *=/CFLAGS = -fPIC/g' Makefile

make -j8 lib-static
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize parallel compilation.

Instead of hardcoding -j8, use the available CPU cores:

-make -j8 lib-static
+nproc_bin="$(command -v nproc)"
+if [ -n "$nproc_bin" ]; then
+    make -j"$($nproc_bin)" lib-static
+else
+    make -j8 lib-static
+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.

Suggested change
make -j8 lib-static
nproc_bin="$(command -v nproc)"
if [ -n "$nproc_bin" ]; then
make -j"$($nproc_bin)" lib-static
else
make -j8 lib-static
fi

22 changes: 14 additions & 8 deletions recipes/d4binding/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,44 @@
{% set version = "0.3.4" %}
{% set name = "d4binding" %}
{% set version = "0.3.11" %}

package:
name: d4binding
name: {{ name }}
version: {{ version }}

build:
number: 1
number: 0
run_exports:
- {{ pin_subpackage("d4binding", max_pin="x.x") }}

source:
# TODO: use formal release number when 0.3.5 is released
url: https://github.com/38/d4-format/archive/refs/tags/v{{ version }}-3.tar.gz
sha256: 05e1a410b77c34d7d188e69ef607eaa9f0babcc9817c558362203fc3f1f2219c
url: https://github.com/38/d4-format/archive/refs/tags/v{{ version }}.tar.gz
sha256: 376e61c93cfe2efc15f5e74b75214e065e278146555e67b8769818bf49594726

requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- rust >=1.30
- gcc #[not osx]
- gcc # [not osx]
- clangdev
- pkg-config
- make
- cmake
run:
- starcode

test:
commands:
- "true"

about:
home: https://github.com/38/d4-format
license: MIT
license_family: MIT
summary: |
The C/C++ binding for the D4 file format
The C/C++ binding for the D4 file format.
dev_url: https://github.com/38/d4-format

extra:
recipe-maintainers:
- haohou
Expand Down
7 changes: 4 additions & 3 deletions recipes/d4tools/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
{% set name = "d4tools" %}
{% set version = "0.3.11" %}

package:
name: d4tools
name: {{ name }}
version: {{ version }}

source:
url: https://github.com/38/d4-format/archive/refs/tags/v{{ version }}.tar.gz
sha256: 376e61c93cfe2efc15f5e74b75214e065e278146555e67b8769818bf49594726

build:
number: 0
number: 1
run_exports:
- {{ pin_subpackage('d4tools', max_pin="x.x") }}

Expand All @@ -35,7 +36,7 @@ about:
license: MIT
license_family: MIT
summary: |
The D4 command line utility program
The D4 command line utility program.
dev_url: https://github.com/38/d4-format

extra:
Expand Down